[Messenger] make senders and handlers subscribing to parent interfaces receive all matching messages, wildcard included #29010

nicolas-grekas · 2018-10-28T21:05:00Z

Q	A
Branch?	4.2
Bug fix?	no
New feature?	yes
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

~~Embeds #29006 for now.~~

From the CHANGELOG:

senders and handlers subscribing to parent interfaces now receive all matching messages, wildcard included
HandlerLocatorInterface::resolve() has been removed, use HandlersLocator::getHandlers() instead
SenderLocatorInterface::getSenderForMessage() has been removed, use SendersLocator::getSenders() instead
The ChainHandler and ChainSender classes have been removed
The ContainerHandlerLocator, AbstractHandlerLocator, SenderLocator and AbstractSenderLocator classes have been removed

The new HandlersLocatorInterface and SendersLocatorInterface interfaces return iterable of corresponding handlers/senders. This allows simplifying a lot the DI configuration and standalone usage.

Inheritance-based configuration is now stable: when a sender or a handler is bound to SomeMessageInterface, it will always get all messages of that kind. This fixes the unstable nature of the previous logic, where only the first matching type bound to a message would match, making routing/handling unpredictable (note that the new behavior is also how Laravel does it.)

Some cleanups found meanwhile:

the messenger.sender tag was useless, it's removed
the reponsibility of the "send-and-handle" decision has been moved to the locator, where it belongs
thanks to type+argument autowiring aliases, we don't need to force defining a default bus - gaining nice errors when a named argument has a typo
some services have been renamed to make them more conventional

As far as priorities are concerned, the priority number applies in the scope of the type bound to it: senders/handlers that are bound to more specific types are always called before less specific ones, no matter the defined priority.

src/Symfony/Component/Messenger/Transport/Sender/SendersLocatorInterface.php

src/Symfony/Component/Messenger/Handler/HandlersLocator.php

nicolas-grekas · 2018-10-29T07:52:25Z

@topbin thanks, fixed

ro0NL

thanks to type+argument autowiring aliases, we don't need to force defining a default bus

so potentially MessageBusInterface has no alias? currently the "message_bus" is still forced to exists few line down low.

nicolas-grekas · 2018-10-29T09:37:10Z

currently the "message_bus" is still forced to exists few line down low

fixed

ro0NL · 2018-10-29T09:59:43Z

MessengerPass wont run if there's no message_bus service, its' not needed anymore IIUC (fixes #28959 i guess :))

nicolas-grekas · 2018-10-29T11:02:28Z

MessengerPass wont run if there's no message_bus service

fixed - and also fixed the message in ControllerTrait.

weaverryan · 2018-10-29T14:38:01Z

This is the approach I was absolutely hoping for. It's predicatable, but type-based. Awesome!

skalpa · 2018-10-29T15:26:52Z

Wouldn't it be worth removing the handlers' priority altogether ? As it is now it's pretty confusing as it only sorts handlers that subscribe to the same type.

It made sense when the handler with the highest priority used to win, but a lot less now. IMHO, users should not expect handlers to be executed in a predictable order. If one needs BarHandler to be executed after FooHandler, then they shouldn't both subscribe to the same message type: FooHandler should dispatch an additional message that BarHandler will subscribe to.

nicolas-grekas · 2018-10-29T15:54:07Z

If a subscriber can declare its priority, it can also declare a more specific type to register. So that's not a downgrade in terms of possibilities. Then, within one type, it can be useful to have priorities (otherwise why would we have added them?) so I think it's OK as is.

skalpa · 2018-10-29T16:08:35Z

Honestly, I wouldn't know why priorities were added to start with ;-). Anyway, I'm fine with that, and this behavior can always be documented.
Btw, all your latest PRs on this component have been brilliant, thanks a lot.

src/Symfony/Bundle/FrameworkBundle/Controller/ControllerTrait.php

src/Symfony/Component/Messenger/CHANGELOG.md

Tobion · 2018-10-29T20:59:00Z

IMHO, users should not expect handlers to be executed in a predictable order. If one needs BarHandler to be executed after FooHandler, then they shouldn't both subscribe to the same message type

Fully agree with @skalpa . You can also make it one handler that does the right thing internally. Also since priortiy now has a different meaning than before, I would suggest to remove it.

nicolas-grekas · 2018-10-29T21:05:11Z

For message buses, priorities can be useless, but the component should also work for query buses. In this case, priorities could be important to have. I expect people to use flat message types most of the time also.

nicolas-grekas · 2018-10-30T14:29:15Z

PR rebased, ready to merge.

nicolas-grekas · 2018-10-30T21:27:59Z

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

            }

            if (($definition = $container->findDefinition($messengerMiddlewareId))->isAbstract()) {
                $childDefinition = new ChildDefinition($messengerMiddlewareId);
-                $count = \count($definition->getArguments());
-                foreach (array_values($arguments ?? array()) as $key => $argument) {


all this logic should be removed: ChildDefinition already have their merging logic. Overwriting an existing parent argument is done by using index_0 as a key. Let's make things simpler by reusing existing knowledge.

Tobion · 2018-10-30T23:56:41Z

src/Symfony/Component/Messenger/Tests/Transport/Sender/SendersLocatorTest.php

+        ));
+
+        $this->assertSame(array($sender), iterator_to_array($locator->getSenders(DummyMessage::class)));
+        $this->assertSame(array(), iterator_to_array($locator->getSenders(SecondMessage::class)));


it's technically not safe to call iterator_to_array on the result as it's return type is iterable which can be an array. I usually have a helper function for cases like this iterableToArray

that's a test case: we do expect an iterator. Even if it's implicit, at least it protects against changing the return value to an array.

Then that should be a new assertion. A static code analyzer would complain about this code.

Tobion · 2018-10-31T00:14:40Z

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

-                        $messagePriority = $messageClass[1];
-                        $messageClass = $messageClass[0];
+                    if (\is_array($message)) {
+                        list($message, $priority) = $message;


this case is not documented and seems to be a relict of the old subscriber return values. considering all the bc breaks, this could be removed as well I guess.

I'd suggest doing so in another PR - here it's unrelated (and I feel like this compiler pass needs to be improved anyway)

Tobion · 2018-10-31T00:38:41Z

src/Symfony/Component/Messenger/Handler/HandlersLocator.php

+            return array($class => $class, '*' => '*');
+        }
+
+        return array($class => $class)


The array key does not seem to be used or useful. So you could put an array_values around the whole array union. This would clarify the code to me as it makes clear the array key is only relevant for the array union operation.

return array_values(array($class => $class) + ...)

An array_values() call would add memory and CPU overhead for no reason. Yes, we don't use the keys. That's not an issue to me - arrays always have keys anyway. And actually having the key be the values makes it impossible to return twice the same value, which is part of the contract of this function. I'd keep as is.

src/Symfony/Component/Messenger/Handler/HandlersLocator.php

sroze · 2018-10-30T19:07:08Z

src/Symfony/Component/Messenger/CHANGELOG.md

@@ -6,38 +6,34 @@ CHANGELOG

 * The component is not experimental anymore
 * All the changes below are BC BREAKS
+ * senders and handlers subscribing to parent interfaces now receive *all* matching messages, wildcard included


An upper case would be lovely here.

sroze

Fantastic. Let's defo make that in 4.2.

…s receive *all* matching messages, wildcard included

sroze · 2018-10-31T08:00:42Z

It took time, but here we go, this is in now. Thank you very much @nicolas-grekas.

…arent interfaces receive *all* matching messages, wildcard included (nicolas-grekas) This PR was merged into the 4.2-dev branch. Discussion ---------- [Messenger] make senders and handlers subscribing to parent interfaces receive *all* matching messages, wildcard included | Q | A | ------------- | --- | Branch? | 4.2 | Bug fix? | no | New feature? | yes | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - ~Embeds #29006 for now.~ From the CHANGELOG: * senders and handlers subscribing to parent interfaces now receive *all* matching messages, wildcard included * `HandlerLocatorInterface::resolve()` has been removed, use `HandlersLocator::getHandlers()` instead * `SenderLocatorInterface::getSenderForMessage()` has been removed, use `SendersLocator::getSenders()` instead * The `ChainHandler` and `ChainSender` classes have been removed * The `ContainerHandlerLocator`, `AbstractHandlerLocator`, `SenderLocator` and `AbstractSenderLocator` classes have been removed The new `HandlersLocatorInterface` and `SendersLocatorInterface` interfaces return **`iterable`** of corresponding handlers/senders. This allows simplifying a lot the DI configuration and standalone usage. Inheritance-based configuration is now stable: when a sender or a handler is bound to `SomeMessageInterface`, it will always get all messages of that kind. This fixes the unstable nature of the previous logic, where only the first matching type bound to a message would match, making routing/handling unpredictable (note that the new behavior is also how Laravel does it.) Some cleanups found meanwhile: * the `messenger.sender` tag was useless, it's removed * the reponsibility of the "send-and-handle" decision has been moved to the locator, where it belongs * thanks to type+argument autowiring aliases, we don't need to force defining a default bus - gaining nice errors when a named argument has a typo * some services have been renamed to make them more conventional As far as priorities are concerned, the priority number applies in the scope of the type bound to it: senders/handlers that are bound to more specific types are always called before less specific ones, no matter the defined priority. Commits ------- 1e7af4d [Messenger] make senders and handlers subscribing to parent interfaces receive *all* matching messages, wildcard included

nicolas-grekas · 2018-10-31T08:05:13Z

Awesome, thank you all for your help!

bendavies · 2018-10-31T09:10:21Z

lots of cool changes to messenger - please don't forget about docs!

nicolas-grekas added this to the 4.2 milestone Oct 28, 2018

nicolas-grekas requested a review from sroze as a code owner October 28, 2018 21:05

carsonbot added Status: Needs Review Messenger Feature BC Break labels Oct 28, 2018

nicolas-grekas force-pushed the messenger-no-chain branch from b295a6c to ac70731 Compare October 28, 2018 21:05

Tobion reviewed Oct 28, 2018

View reviewed changes

src/Symfony/Component/Messenger/Transport/Sender/SendersLocatorInterface.php Show resolved Hide resolved

Tobion reviewed Oct 29, 2018

View reviewed changes

src/Symfony/Component/Messenger/Handler/HandlersLocator.php Show resolved Hide resolved

nicolas-grekas force-pushed the messenger-no-chain branch from ac70731 to 71d5cac Compare October 29, 2018 07:50

ro0NL reviewed Oct 29, 2018

View reviewed changes

nicolas-grekas force-pushed the messenger-no-chain branch from 71d5cac to 1ea0880 Compare October 29, 2018 09:36

nicolas-grekas force-pushed the messenger-no-chain branch from 1ea0880 to 54edf57 Compare October 29, 2018 11:02

nicolas-grekas force-pushed the messenger-no-chain branch from 54edf57 to 55337eb Compare October 29, 2018 11:52

ogizanagi reviewed Oct 29, 2018

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Controller/ControllerTrait.php Outdated Show resolved Hide resolved

src/Symfony/Component/Messenger/CHANGELOG.md Outdated Show resolved Hide resolved

nicolas-grekas force-pushed the messenger-no-chain branch from 55337eb to e2ba16d Compare October 29, 2018 21:01

nicolas-grekas force-pushed the messenger-no-chain branch from e2ba16d to 7d69b41 Compare October 30, 2018 14:16

nicolas-grekas mentioned this pull request Oct 30, 2018

[Messenger] Add some UPGRADE entries regarding 4.2 BC breaks #29027

Merged

nicolas-grekas force-pushed the messenger-no-chain branch from 7d69b41 to b224cc3 Compare October 30, 2018 21:24

nicolas-grekas commented Oct 30, 2018

View reviewed changes

Tobion reviewed Oct 30, 2018

View reviewed changes

Tobion reviewed Oct 31, 2018

View reviewed changes

src/Symfony/Component/Messenger/Handler/HandlersLocator.php Show resolved Hide resolved

This was referenced Oct 31, 2018

[Messenger][FrameworkBundle] Fix bus name on traceable middleware #29038

Closed

[Messenger] Allow to override only a few of the abstract middleware definition #29034

Closed

sroze reviewed Oct 31, 2018

View reviewed changes

carsonbot removed the Status: Needs Review label Oct 31, 2018

sroze approved these changes Oct 31, 2018

View reviewed changes

carsonbot added the Status: Reviewed label Oct 31, 2018

nicolas-grekas force-pushed the messenger-no-chain branch from b224cc3 to 8aa3696 Compare October 31, 2018 07:45

[Messenger] make senders and handlers subscribing to parent interface…

1e7af4d

…s receive *all* matching messages, wildcard included

nicolas-grekas force-pushed the messenger-no-chain branch from 8aa3696 to 1e7af4d Compare October 31, 2018 07:48

sroze merged commit 1e7af4d into symfony:master Oct 31, 2018

sroze mentioned this pull request Oct 31, 2018

[Messenger] Add middleware allowing just for a single handler #28716

Closed

nicolas-grekas deleted the messenger-no-chain branch October 31, 2018 08:05

This was referenced Nov 3, 2018

Release v4.2.0-BETA1 #29072

Closed

Release v4.2.0-BETA1 #29073

Merged

skalpa mentioned this pull request Nov 7, 2018

[Messenger] Configure bus to have exactly one handler per message #27840

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Messenger] make senders and handlers subscribing to parent interfaces receive all matching messages, wildcard included #29010

[Messenger] make senders and handlers subscribing to parent interfaces receive all matching messages, wildcard included #29010

nicolas-grekas commented Oct 28, 2018 •

edited

Loading

nicolas-grekas commented Oct 29, 2018

ro0NL left a comment

nicolas-grekas commented Oct 29, 2018

ro0NL commented Oct 29, 2018

nicolas-grekas commented Oct 29, 2018

weaverryan commented Oct 29, 2018

skalpa commented Oct 29, 2018 •

edited

Loading

nicolas-grekas commented Oct 29, 2018

skalpa commented Oct 29, 2018

Tobion commented Oct 29, 2018 •

edited

Loading

nicolas-grekas commented Oct 29, 2018 •

edited

Loading

nicolas-grekas commented Oct 30, 2018

nicolas-grekas Oct 30, 2018

Tobion Oct 30, 2018 •

edited

Loading

nicolas-grekas Oct 31, 2018

Tobion Oct 31, 2018

Tobion Oct 31, 2018

nicolas-grekas Oct 31, 2018

Tobion Oct 31, 2018

Tobion Oct 31, 2018 •

edited

Loading

nicolas-grekas Oct 31, 2018

sroze Oct 30, 2018

sroze left a comment

sroze commented Oct 31, 2018

nicolas-grekas commented Oct 31, 2018

bendavies commented Oct 31, 2018

[Messenger] make senders and handlers subscribing to parent interfaces receive *all* matching messages, wildcard included #29010

[Messenger] make senders and handlers subscribing to parent interfaces receive *all* matching messages, wildcard included #29010

Conversation

nicolas-grekas commented Oct 28, 2018 • edited Loading

nicolas-grekas commented Oct 29, 2018

ro0NL left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Oct 29, 2018

ro0NL commented Oct 29, 2018

nicolas-grekas commented Oct 29, 2018

weaverryan commented Oct 29, 2018

skalpa commented Oct 29, 2018 • edited Loading

nicolas-grekas commented Oct 29, 2018

skalpa commented Oct 29, 2018

Tobion commented Oct 29, 2018 • edited Loading

nicolas-grekas commented Oct 29, 2018 • edited Loading

nicolas-grekas commented Oct 30, 2018

Choose a reason for hiding this comment

Tobion Oct 30, 2018 • edited Loading

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

Tobion Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sroze left a comment

Choose a reason for hiding this comment

sroze commented Oct 31, 2018

nicolas-grekas commented Oct 31, 2018

bendavies commented Oct 31, 2018

[Messenger] make senders and handlers subscribing to parent interfaces receive all matching messages, wildcard included #29010

[Messenger] make senders and handlers subscribing to parent interfaces receive all matching messages, wildcard included #29010

nicolas-grekas commented Oct 28, 2018 •

edited

Loading

skalpa commented Oct 29, 2018 •

edited

Loading

Tobion commented Oct 29, 2018 •

edited

Loading

nicolas-grekas commented Oct 29, 2018 •

edited

Loading

Tobion Oct 30, 2018 •

edited

Loading

Tobion Oct 31, 2018 •

edited

Loading