Support for Messenger HandleTrait return types by bnowak · Pull Request #404 · phpstan/phpstan-symfony

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

…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

 /tests/tmp
 /build-cs
 /vendor
+/.idea


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

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

+
+				/** @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

+				}
+
+				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

+	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

+		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

+				}
+
+				/** @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

+	 * @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

+		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

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


This needs =.

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

+
+			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

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


Never catch ShouldNotHappenException.

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

+	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 · 2024-12-30T06:26:29Z

@ondrejmirtes both comments fixed. Could you re-check? Thanks, in advance :)

ondrejmirtes · 2025-01-04T13:55:41Z

Thank you. Please be available if any issues arise.

bnowak · 2025-01-04T16:23:03Z

@ondrejmirtes Thank you for merging and your help with reviewing that :)

Could you take a look on #404 (comment) comment please 🙏 I'm not sure whether I should create separate issue for that? Maybe it's able to achive with already existing features/annotations? If not, do you have an idea how to implement such feature?

ondrejmirtes · 2025-01-05T12:43:27Z

@bnowak Feel free to send a failing test so I can understand it better.

ondrejmirtes · 2025-07-10T11:11:13Z

Hi, since MessageSubscriberInterface has been deprecated/removed in favour of #[AsMessageHandler], shouldn't the code in MessageMapFactory be adjusted to also support the attribute? Thanks.

bnowak · 2025-07-10T11:19:13Z

@ondrejmirtes I'll take a look on that tomorrow. Thanks for info 😉

ondrejmirtes · 2025-07-10T11:23:38Z

Maybe it's the reason why some of the tests are failing on Symfony 7.3, maybe not: https://github.com/phpstan/phpstan-symfony/actions/runs/16193692378/job/45715050598?pr=447

bnowak · 2025-07-10T12:19:44Z

Yes it is. I started working on tests separation and skipping the case when MessageSubscriberInterface is missing (SF <7.0). Tomorrow, I'll push PR for that.

Regarding adding support for #[AsMessageHandler] - I'll handle that in separate PR, no problem. However, I think firstly before that, it would be nice to have support for SF 7.3 in tests (work which you started in https://github.com/phpstan/phpstan-symfony/pull/447/files). Then, I'd add that support.

@ondrejmirtes What do you think? Does such order make sense?

ondrejmirtes · 2025-07-10T13:53:33Z

I think AsMessageHandler already existed before Symfony 7, didn't it?

It'd make it easy to implement it here, and when my symfony-7-3 is rebased, less tests should fail.

bnowak · 2025-07-10T14:03:01Z

Ahh, you're right. I don't know why I assumed it was introduced in 7.0 😅

Tomorrow will try to handle both topics 😉

bnowak · 2025-07-11T09:37:17Z

@ondrejmirtes please review #448.

Regarding AsMessageHandler support - it's just tag for auto configuring it during kernel boot, so its result is generated container xml file definition which this plugin read from (the same as messenger handle trait feature).
Given that, it should be supported by default and additional tests are not needed IMO.

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

Closed

KrzysztofCzerwinski reviewed Oct 18, 2024

View reviewed changes

Comment thread src/Symfony/MessageMap.php

Comment thread src/Symfony/MessageMap.php Outdated

Comment thread src/Symfony/MessageMapFactory.php Outdated

Comment thread src/Symfony/MessageMapFactory.php

Comment thread src/Type/Symfony/MessengerHandleTraitReturnTypeExtension.php Outdated

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

code review fixes 2

df90a3b

ondrejmirtes merged commit dd1aaa7 into phpstan:1.4.x Jan 4, 2025

bnowak deleted the messenger-handle-trait-support branch January 17, 2025 11:46

bnowak mentioned this pull request Jan 17, 2025

Using messenger HandleTrait as QueryBus with appropriate result typing #424

Closed

bnowak mentioned this pull request Jul 10, 2025

handle optional MessageSubscriberInterface if exist #448

Merged

Conversation

bnowak commented Aug 13, 2024

Uh oh!

ondrejmirtes left a comment

Choose a reason for hiding this comment

Uh oh!

bnowak commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnowak commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bnowak commented Nov 20, 2024

Uh oh!

bnowak commented Dec 18, 2024

Uh oh!

ondrejmirtes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnowak commented Dec 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnowak commented Dec 30, 2024

Uh oh!

ondrejmirtes commented Jan 4, 2025

Uh oh!

bnowak commented Jan 4, 2025

Uh oh!

ondrejmirtes commented Jan 5, 2025

Uh oh!

ondrejmirtes commented Jul 10, 2025

Uh oh!

bnowak commented Jul 10, 2025

Uh oh!

ondrejmirtes commented Jul 10, 2025

Uh oh!

bnowak commented Jul 10, 2025

Uh oh!

ondrejmirtes commented Jul 10, 2025

Uh oh!

bnowak commented Jul 10, 2025

Uh oh!

bnowak commented Jul 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

bnowak commented Oct 17, 2024 •

edited

Loading

bnowak commented Oct 17, 2024 •

edited

Loading