Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] make senders and handlers subscribing to parent interfaces receive *all* matching messages, wildcard included#29010
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
b295a6c toac70731Comparesrc/Symfony/Component/Messenger/Transport/Sender/SendersLocatorInterface.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
ac70731 to71d5cacComparenicolas-grekas commentedOct 29, 2018
@topbin thanks, fixed |
ro0NL left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
71d5cac to1ea0880Comparenicolas-grekas commentedOct 29, 2018
fixed |
ro0NL commentedOct 29, 2018
|
1ea0880 to54edf57Comparenicolas-grekas commentedOct 29, 2018
fixed - and also fixed the message in ControllerTrait. |
54edf57 to55337ebCompareweaverryan commentedOct 29, 2018
This is the approach I was absolutely hoping for. It's predicatable, but type-based. Awesome! |
skalpa commentedOct 29, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Wouldn't it be worth removing the handlers' It made sense when the handler with the highest priority used towin, but a lot less now. IMHO, users should not expect handlers to be executed in a predictable order. If one needs |
nicolas-grekas commentedOct 29, 2018
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 commentedOct 29, 2018
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. |
src/Symfony/Bundle/FrameworkBundle/Controller/ControllerTrait.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Tobion commentedOct 29, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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. |
55337eb toe2ba16dComparenicolas-grekas commentedOct 29, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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. |
e2ba16d to7d69b41Comparenicolas-grekas commentedOct 30, 2018
PR rebased, ready to merge. |
7d69b41 tob224cc3Compare| if (($definition =$container->findDefinition($messengerMiddlewareId))->isAbstract()) { | ||
| $childDefinition =newChildDefinition($messengerMiddlewareId); | ||
| $count =\count($definition->getArguments()); | ||
| foreach (array_values($arguments ??array())as$key =>$argument) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
all this logic should be removed: ChildDefinition already have their merging logic. Overwriting an existing parent argument is done by usingindex_0 as a key. Let's make things simpler by reusing existing knowledge.
| )); | ||
| $this->assertSame(array($sender),iterator_to_array($locator->getSenders(DummyMessage::class))); | ||
| $this->assertSame(array(),iterator_to_array($locator->getSenders(SecondMessage::class))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it's technically not safe to calliterator_to_array on the result as it's return type isiterable which can be an array. I usually have a helper function for cases like thisiterableToArray
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
that's a test case: wedo expect an iterator. Even if it's implicit, at least it protects against changing the return value to an array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Then that should be a new assertion. A static code analyzer would complain about this code.
| $messagePriority =$messageClass[1]; | ||
| $messageClass =$messageClass[0]; | ||
| if (\is_array($message)) { | ||
| list($message,$priority) =$message; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd suggest doing so in another PR - here it's unrelated (and I feel like this compiler pass needs to be improved anyway)
| returnarray($class =>$class,'*' =>'*'); | ||
| } | ||
| returnarray($class =>$class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The array key does not seem to be used or useful. So you could put anarray_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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
return array_values(array($class => $class) + ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
| * 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
An upper case would be lovely here.
sroze left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fantastic. Let's defo make that in 4.2.
b224cc3 to8aa3696Compare…s receive *all* matching messages, wildcard included
8aa3696 to1e7af4dComparesroze commentedOct 31, 2018
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 removedThe 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 conventionalAs 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 commentedOct 31, 2018
Awesome, thank you all for your help! |
bendavies commentedOct 31, 2018
lots of cool changes to messenger - please don't forget about docs! |
Uh oh!
There was an error while loading.Please reload this page.
Embeds#29006 for now.From the CHANGELOG:
HandlerLocatorInterface::resolve()has been removed, useHandlersLocator::getHandlers()insteadSenderLocatorInterface::getSenderForMessage()has been removed, useSendersLocator::getSenders()insteadChainHandlerandChainSenderclasses have been removedContainerHandlerLocator,AbstractHandlerLocator,SenderLocatorandAbstractSenderLocatorclasses have been removedThe new
HandlersLocatorInterfaceandSendersLocatorInterfaceinterfaces returniterableof 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:
messenger.sendertag was useless, it's removedAs 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.