[Messenger] Compile-time errors fixes and tweaks #26672

chalasr · 2018-03-26T09:26:05Z

Q	A
Branch?	master
Bug fix?	no (master only)
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	n/a

chalasr · 2018-03-26T09:28:10Z

src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

        }

-        return $parameter->getClass()->getName();


getClass() breaks if the class does not exist (ReflectionException)

chalasr · 2018-03-26T09:29:22Z

src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php


-                    throw new RuntimeException(sprintf('The message class "%s" %s of service "%s" does not exist.', $messageClassLocation, $handles, $serviceId));


bad argument ordering in sprintf

sroze · 2018-03-26T11:13:48Z

src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

        } catch (\ReflectionException $e) {
-            throw new RuntimeException(sprintf('Service "%s" should have an `__invoke` function.', $serviceId));
+            throw new RuntimeException(sprintf('Invalid service "%s": class "%s" must have an "__invoke()" method.', $serviceId, $handlerClass->getName()));


What do you think about replacing all the "Invalid service" by "Invalid handler service" to clarify why it requires such method?

stof · 2018-03-26T15:54:37Z

src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

-        $parameter = $parameters[0];
-        if (null === $parameter->getClass()) {
-            throw new RuntimeException(sprintf('The parameter of `__invoke` function of service "%s" must type hint the message class it handles.', $serviceId));
+        if (!$parameters[0]->hasType()) {


should also report the issue if it has a builtin type as typehint (i.e. not a class)

…tead of `message.handler` (sroze) This PR was merged into the 4.1-dev branch. Discussion ---------- [Messenger] Uses the `messenger.message_handler` tag instead of `message.handler` | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no (technically, it's not even in 4.1) | Deprecations? | no | Tests pass? | yes | Fixed tickets | ø | License | MIT After a few days using it, it feels weird to have the `messenger.receiver` and `messenger.sender` tags while the one for message handlers is `message_handler`. I believe that for consistency, this would make sense to rename this tag `messenger.message_handler`. As we don't have tests for this pass yet (will be added in #26672) there is nothing else to change (but the documentation). Commits ------- bddebc4 Uses the `messenger.message_handler` tag instead of `message.handler`

sroze · 2018-03-27T07:49:36Z

src/Symfony/Component/Messenger/Tests/DependencyInjection/MessengerPassTest.php

+        $container = $this->getContainerBuilder();
+        $container
+            ->register(MissingArgumentHandler::class, MissingArgumentHandler::class)
+            ->addTag('message_handler')


This tag has changed and is now messenger.message_handler. Could you rebase this PR on master and update the tests?

chalasr · 2018-03-27T10:19:24Z

@sroze @stof comments addressed

fabpot · 2018-03-27T12:15:00Z

src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

@@ -67,12 +67,12 @@ private function registerHandlers(ContainerBuilder $container)

        foreach ($container->findTaggedServiceIds($this->handlerTag, true) as $serviceId => $tags) {
            foreach ($tags as $tag) {
-                $handles = $tag['handles'] ?? $this->guessHandledClass($container, $serviceId);
+                $handles = $tag['handles'] ?? $this->guessHandledClass($r = $container->getReflectionClass($container->getDefinition($serviceId)->getClass()), $serviceId);


The class in the definition can be a parameter like %some_class_name%, you should resolve it here (via $container->getParameterBag()->resolveValue()).

chalasr · 2018-03-29T08:02:42Z

ready

sroze · 2018-03-29T08:16:23Z

I'm happy with it. @stof & @fabpot could you amend your reviews?

jakzal · 2018-03-29T09:33:30Z

src/Symfony/Component/Messenger/Tests/DependencyInjection/MessengerPassTest.php

+    }
+
+    /**
+     * @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException


Not sure what's our stance on this, but phpunit advices to use method calls instead of annotations.

for now we use annotations everywhere, perhaps it should be rediscussed on 2.7

fabpot · 2018-03-29T13:47:17Z

Thank you @chalasr.

This PR was merged into the 4.1-dev branch. Discussion ---------- [Messenger] Compile-time errors fixes and tweaks | Q | A | ------------- | --- | Branch? | master | Bug fix? | no (master only) | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Commits ------- 2889acf [Messenger] Compile time errors fixes and tweaks

@chalasr

…messages + auto-configuration) (sroze) This PR was squashed before being merged into the 4.1-dev branch (closes #26685). Discussion ---------- [Messenger] Add a `MessageHandlerInterface` (multiple messages + auto-configuration) | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | ø | License | MIT | Doc PR | ø Based on @chalasr's PR: symfony/symfony#26672. This reduces the hassle of registering handlers: it allows the auto-configuration with a new interface `HandlerInterface`. At the same time, it allows a handler to handle multiple messages. Commits ------- 07e6bc73a3 [Messenger] Add a `MessageHandlerInterface` (multiple messages + auto-configuration)

@chalasr

…messages + auto-configuration) (sroze) This PR was squashed before being merged into the 4.1-dev branch (closes #26685). Discussion ---------- [Messenger] Add a `MessageHandlerInterface` (multiple messages + auto-configuration) | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | ø | License | MIT | Doc PR | ø Based on @chalasr's PR: symfony/symfony#26672. This reduces the hassle of registering handlers: it allows the auto-configuration with a new interface `HandlerInterface`. At the same time, it allows a handler to handle multiple messages. Commits ------- 07e6bc73a3 [Messenger] Add a `MessageHandlerInterface` (multiple messages + auto-configuration)

@chalasr

…messages + auto-configuration) (sroze) This PR was squashed before being merged into the 4.1-dev branch (closes #26685). Discussion ---------- [Messenger] Add a `MessageHandlerInterface` (multiple messages + auto-configuration) | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | ø | License | MIT | Doc PR | ø Based on @chalasr's PR: #26672. This reduces the hassle of registering handlers: it allows the auto-configuration with a new interface `HandlerInterface`. At the same time, it allows a handler to handle multiple messages. Commits ------- 07e6bc7 [Messenger] Add a `MessageHandlerInterface` (multiple messages + auto-configuration)

chalasr added the Messenger label Mar 26, 2018

chalasr added this to the 4.1 milestone Mar 26, 2018

carsonbot added the Status: Needs Review label Mar 26, 2018

chalasr force-pushed the messenger-compile-errors branch 2 times, most recently from 0d569f6 to 51c66c6 Compare March 26, 2018 09:28

chalasr commented Mar 26, 2018

View reviewed changes

chalasr force-pushed the messenger-compile-errors branch from 51c66c6 to 37bcfea Compare March 26, 2018 09:57

sroze reviewed Mar 26, 2018

View reviewed changes

chalasr force-pushed the messenger-compile-errors branch from 37bcfea to f30d857 Compare March 26, 2018 11:38

carsonbot removed the Status: Needs Review label Mar 26, 2018

stof requested changes Mar 26, 2018

View reviewed changes

carsonbot added the Status: Needs Work label Mar 26, 2018

sroze mentioned this pull request Mar 26, 2018

[Messenger] Uses the messenger.message_handler tag instead of message.handler #26676

Merged

sroze suggested changes Mar 27, 2018

View reviewed changes

chalasr force-pushed the messenger-compile-errors branch 2 times, most recently from 574ef49 to ed9ef9d Compare March 27, 2018 10:18

chalasr added Status: Needs Review and removed Status: Needs Work labels Mar 27, 2018

fabpot requested changes Mar 27, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Mar 27, 2018

[Messenger] Compile time errors fixes and tweaks

2889acf

chalasr force-pushed the messenger-compile-errors branch from ed9ef9d to 2889acf Compare March 27, 2018 12:18

sroze approved these changes Mar 27, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Mar 27, 2018

sroze mentioned this pull request Mar 27, 2018

[Messenger] Add a MessageHandlerInterface (multiple messages + auto-configuration) #26685

Merged

jakzal reviewed Mar 29, 2018

View reviewed changes

fabpot approved these changes Mar 29, 2018

View reviewed changes

fabpot merged commit 2889acf into symfony:master Mar 29, 2018

chalasr deleted the messenger-compile-errors branch March 29, 2018 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Messenger] Compile-time errors fixes and tweaks #26672

[Messenger] Compile-time errors fixes and tweaks #26672

chalasr commented Mar 26, 2018

chalasr Mar 26, 2018

chalasr Mar 26, 2018

sroze Mar 26, 2018

chalasr Mar 26, 2018

stof Mar 26, 2018

sroze Mar 27, 2018

chalasr commented Mar 27, 2018

fabpot Mar 27, 2018

chalasr Mar 27, 2018

chalasr commented Mar 29, 2018

sroze commented Mar 29, 2018

jakzal Mar 29, 2018

chalasr Mar 29, 2018

fabpot commented Mar 29, 2018


		throw new RuntimeException(sprintf('The message class "%s" %s of service "%s" does not exist.', $messageClassLocation, $handles, $serviceId));

[Messenger] Compile-time errors fixes and tweaks #26672

[Messenger] Compile-time errors fixes and tweaks #26672

Conversation

chalasr commented Mar 26, 2018

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

chalasr commented Mar 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr commented Mar 29, 2018

sroze commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Mar 29, 2018