Support for Messenger HandleTrait return types #404

bnowak · 2024-08-13T09:30:24Z

The goal is to cover #207.

That's my very first PR in phpstan-symfony plugin, so any feedback/tips/advices are very welcome :)

ondrejmirtes

Instead of DynamicMethodReturnTypeExtension you can use https://apiref.phpstan.org/1.11.x/PHPStan.Type.ExpressionTypeResolverExtension.html which is like that but lets you override any expression type.

bnowak · 2024-10-17T14:35:26Z

@ondrejmirtes Thanks for the tip about ExpressionTypeResolverExtension! It resolved my blocker issue about not supported traits in DynamicMethodReturnTypeExtension.

It took me some time, however finally I've got ready to review version of this PR :) Please take a look in you free time :)

Thank you very much!

bnowak · 2024-10-17T14:48:26Z

As static analyzing should works now fine with the result of HandleTrait::handle, but only in classes which have statically typed query passed - like in this SF-adjusted example:

// src/Action/ListItems.php
namespace App\Action;

use App\Message\ListItemsQuery;
use App\MessageHandler\ListItemsQueryResult;
use Symfony\Component\Messenger\HandleTrait;
use Symfony\Component\Messenger\MessageBusInterface;

class ListItems
{
    use HandleTrait;

    public function __construct(
        private MessageBusInterface $messageBus,
    ) {
    }

    public function __invoke(): void
    {
        $result = $this->handle(new ListItemsQuery(/* ... */));
        // $result should be automatically recognized as `ListItemsQueryResult` now by phpstan via SF configuration

        // Do something with the result
        // ...
    }
}

However, my target goal was to cover QueryBus classes with dynamic return types based on dynamic query, something like:

// src/MessageBus/QueryBus.php
namespace App\MessageBus;

use Symfony\Component\Messenger\Envelope;
use Symfony\Component\Messenger\HandleTrait;
use Symfony\Component\Messenger\MessageBusInterface;

class QueryBus
{
    use HandleTrait;

    public function __construct(
        private MessageBusInterface $messageBus,
    ) {
    }

    // wherever that method is called, phpstan should recognize return type based on passed query
    public function query($query)
    {
        return $this->handle($query);
    }
}

Maybe it's trivial and doable with already existing php-doc annotation or more complex to write some next extension for that, but I don't know how to connect the dots and reuse covered typing of HandleTrait::handle which is already in place. I'm happy to address that in separate PR (if will be needed to achieve that) :)
Any thoughts/ideas are more than welcome :)
/cc @ondrejmirtes

src/Symfony/MessageMap.php

src/Symfony/MessageMapFactory.php

src/Type/Symfony/MessengerHandleTraitReturnTypeExtension.php

…eExtension

…HandledMessages

bnowak · 2024-11-20T13:32:12Z

@ondrejmirtes any chances to review that, please? 🙏

bnowak · 2024-12-18T07:28:12Z

@ondrejmirtes I can see that version 2.0.0 is released now. Is 1.x abandoned from now? Should I rebase that PR to 2.x branch? Any chances to review and/or merge it please? 🙂

ondrejmirtes

No need to rebase this, we could release this in 1.x.

ondrejmirtes · 2024-12-18T08:36:08Z

.gitignore

@@ -1,5 +1,6 @@
 /tests/tmp
 /build-cs
 /vendor
+/.idea


Remove this please. This belongs to a global .gitignore.

ondrejmirtes · 2024-12-18T08:36:32Z

src/Symfony/MessageMapFactory.php

+
+				/** @var array{handles?: class-string, method?: string} $tagAttributes */
+				$tagAttributes = $tag->getAttributes();
+				$reflectionClass = $this->reflectionProvider->getClass($serviceClass);


This is unsafe. Ask for hasClass first.

ondrejmirtes · 2024-12-18T08:36:43Z

src/Symfony/MessageMapFactory.php

+				}
+
+				foreach ($handles as $messageClassName => $options) {
+					$methodReflection = $reflectionClass->getNativeMethod($options['method'] ?? self::DEFAULT_HANDLER_METHOD);


This is unsafe. Ask for hasNativeMethod first.

ondrejmirtes · 2024-12-18T08:37:22Z

src/Symfony/MessageMapFactory.php

+	private function guessHandledMessages(ClassReflection $reflectionClass): iterable
+	{
+		if ($reflectionClass->implementsInterface(MessageSubscriberInterface::class)) {
+			foreach ($reflectionClass->getName()::getHandledMessages() as $index => $value) {


Save the class name into a variable first, then call the method.

ondrejmirtes · 2024-12-18T08:37:41Z

src/Symfony/MessageMapFactory.php

+		return new MessageMap($messageMap);
+	}
+
+	/** @return array<class-string, array<string, string>> */


This does not return an array. It's a generator.

right, changed to iterable :)

ondrejmirtes · 2024-12-18T08:40:23Z

src/Symfony/MessageMapFactory.php

+				}
+
+				/** @var array{handles?: class-string, method?: string} $tagAttributes */
+				$tagAttributes = $tag->getAttributes();


Can we add a @return stub for this method instead of @var?

This shape is dynamic and rely on what tag we're using from SF config. In this case I'm ensuring above that we're handling only messenger.message_handler, so we know what shape it should have.

As far I know stubs are static only, so unfortunately we cannot use them here.

Do you have other idea or it could stay as it is?

ondrejmirtes · 2024-12-18T08:40:56Z

src/Symfony/MessageMapFactory.php

+	 * @phpstan-assert-if-true class-string $index
+	 * @phpstan-assert-if-true array<string, mixed> $value
+	 * @phpstan-assert-if-false int $index
+	 * @phpstan-assert-if-false class-string $value


All of these asserts need =: https://phpstan.org/writing-php-code/narrowing-types#equality-assertions

ondrejmirtes · 2024-12-18T08:41:21Z

src/Type/Symfony/MessengerHandleTraitReturnTypeExtension.php

+		if ($this->isSupported($expr, $scope)) {
+			$arg = $expr->getArgs()[0]->value;
+			/** @var class-string[] $argClassNames */
+			$argClassNames = $scope->getType($arg)->getObjectClassNames();


Don't add this @var.

ondrejmirtes · 2024-12-18T08:41:43Z

src/Type/Symfony/MessengerHandleTraitReturnTypeExtension.php

+	}
+
+	/**
+	 * @phpstan-assert-if-true MethodCall $expr


This needs =.

ondrejmirtes · 2024-12-18T08:43:32Z

src/Type/Symfony/MessengerHandleTraitReturnTypeExtension.php

+
+			return $declaringClassReflection->getName() === self::TRAIT_NAME;
+		} catch (ReflectionException $e) {
+			return false;


Instead of catching exception, check if the method exists first.

bnowak · 2024-12-23T08:07:44Z

@ondrejmirtes Thanks for review. I addressed all your comments, please take a look again when you have time :)

Also, could you take a look on #404 (comment) and answer that if you have an idea how to solve it please? That's my initial goal to resolve by this PR :)

ondrejmirtes · 2024-12-23T16:39:09Z

src/Symfony/MessageMapFactory.php

+					} else {
+						yield $value => ['method' => self::DEFAULT_HANDLER_METHOD];
+					}
+				} catch (ShouldNotHappenException $e) {


Never catch ShouldNotHappenException.

ondrejmirtes · 2024-12-23T16:39:55Z

src/Type/Symfony/MessengerHandleTraitReturnTypeExtension.php

+	public function getType(Expr $expr, Scope $scope): ?Type
+	{
+		if ($this->isSupported($expr, $scope)) {
+			$arg = $expr->getArgs()[0]->value;


$expr->getArgs()[0] might not exist. Check the existence first.

bnowak force-pushed the messenger-handle-trait-support branch from 5d47f63 to e3790e0 Compare August 14, 2024 05:53

ondrejmirtes requested changes Aug 19, 2024

View reviewed changes

bnowak force-pushed the messenger-handle-trait-support branch from e7063fd to 7b4c17e Compare October 15, 2024 06:23

bnowak marked this pull request as ready for review October 17, 2024 14:31

bnowak requested a review from ondrejmirtes October 17, 2024 14:48

bnowak mentioned this pull request Oct 17, 2024

Support for Symfony HandleTrait #207

Open

KrzysztofCzerwinski reviewed Oct 18, 2024

View reviewed changes

bnowak requested a review from KrzysztofCzerwinski October 18, 2024 10:50

KrzysztofCzerwinski approved these changes Oct 18, 2024

View reviewed changes

ondrejmirtes force-pushed the 1.4.x branch from 0f833b0 to 270c2ee Compare October 30, 2024 12:07

Bartłomiej Nowak added 12 commits November 7, 2024 13:30

base version of handling query result

4c95d32

coding standards fix

5985362

phpstan fix

bd75cb6

phpstan fix 2

9768de5

add /.idea path to .gitignore

0c8cb34

use ExpressionTypeResolverExtension instead of DynamicMethodReturnTyp…

224c478

…eExtension

adjust MessageMap to contain only supported cases

9438e5e

covered more test-cases

8a44be8

get rid of todo comments in \PHPStan\Symfony\MessageMapFactory::guess…

46e0cc8

…HandledMessages

addressed rest of todo comments

5cba4ba

code review 1

f7bf1e0

code review 2 removed mixed type which doesn't exist in <PHP8

d62ec03

bnowak force-pushed the messenger-handle-trait-support branch from b94aa51 to d62ec03 Compare November 7, 2024 12:30

ondrejmirtes requested changes Dec 18, 2024

View reviewed changes

code review fixes

89c7534

ondrejmirtes requested changes Dec 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for Messenger HandleTrait return types #404

Support for Messenger HandleTrait return types #404

bnowak commented Aug 13, 2024

ondrejmirtes left a comment

bnowak commented Oct 17, 2024 •

edited

Loading

bnowak commented Oct 17, 2024 •

edited

Loading

bnowak commented Nov 20, 2024

bnowak commented Dec 18, 2024

ondrejmirtes left a comment

ondrejmirtes Dec 18, 2024

ondrejmirtes Dec 18, 2024

ondrejmirtes Dec 18, 2024

ondrejmirtes Dec 18, 2024

ondrejmirtes Dec 18, 2024

bnowak Dec 23, 2024

ondrejmirtes Dec 18, 2024

bnowak Dec 23, 2024

ondrejmirtes Dec 18, 2024

ondrejmirtes Dec 18, 2024

ondrejmirtes Dec 18, 2024

ondrejmirtes Dec 18, 2024

bnowak commented Dec 23, 2024

ondrejmirtes Dec 23, 2024

ondrejmirtes Dec 23, 2024

Support for Messenger HandleTrait return types #404

Are you sure you want to change the base?

Support for Messenger HandleTrait return types #404

Conversation

bnowak commented Aug 13, 2024

ondrejmirtes left a comment

Choose a reason for hiding this comment

bnowak commented Oct 17, 2024 • edited Loading

bnowak commented Oct 17, 2024 • edited Loading

bnowak commented Nov 20, 2024

bnowak commented Dec 18, 2024

ondrejmirtes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnowak commented Dec 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnowak commented Oct 17, 2024 •

edited

Loading

bnowak commented Oct 17, 2024 •

edited

Loading