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] Allow to scope handlers per bus#27275
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
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.
thx for adding the test :)
| foreach ($handlersas$handler) { | ||
| $tableRows[] =array(sprintf(' handled by %s',$handler)); | ||
| } | ||
| $mappings =$this->mapping; |
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.
perhaps rename the property to$mappings as well (plural)
| } | ||
| $mappings =$this->mapping; | ||
| if ($bus =$input->getArgument('bus')) { | ||
| $mappings =array($bus =>$mappings[$bus]); |
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.
missingisset? should throw/error for unknown buses no?
| },$handlersByMessage)); | ||
| $debugCommandMapping =$handlersByBusAndMessage; | ||
| foreach ($busesas$bus) { | ||
| if (!isset($debugCommandMapping[$bus])) { |
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.
see previous suggestion, i think this should be checked for at runtime (as well)
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.
Added check at runtime in debug command
| foreach ($container->findTaggedServiceIds($this->handlerTag,true)as$serviceId =>$tags) { | ||
| foreach ($tagsas$tag) { | ||
| if (isset($tag['bus']) && !\in_array($tag['bus'],$buses,true)) { |
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.
for being service ids, in theory it could resolve aliases. Allowingbus: message_bus instead ofbus: messenger.bus.default.
Personally i'd find that useful for handlers registered by 3rd party bundles, allowing them to use a fixed alias id during registration and let the user provide the alias service.
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.
No strong opinion on this. Let's see what others think :)
Perhaps not in this PR anyway in order to not add more complexity.
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.
In my case im still hesitating between auto registration and documenting the user config. Things like this and removing the middleware tag makes me lean to the latter 🤔
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.
You can already usebus: command_bus for example if you define your bus that way:
framework:messenger:buses:command_bus:~
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.
Again, really great idea.
| ->setHelp(<<<'EOF' | ||
| The <info>%command.name%</info> command displays all messages that can be | ||
| dispatched using the messagebus: | ||
| dispatched using the messagebuses: |
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.
Would it make sense to add thebus example in the help?
| $mapping =$this->mapping; | ||
| if ($bus =$input->getArgument('bus')) { | ||
| if (!isset($mapping[$bus])) { | ||
| thrownewRuntimeException(sprintf('Invalid bus "%s". Known buses are %s.',$bus,implode(',',array_keys($this->mapping)))); |
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.
Bus "%s" does not exist. Know ...
| $io->newLine(); | ||
| $io->table(array(),$tableRows); | ||
| }else { | ||
| $io->warning(sprintf('No messages were found that have valid handlers in bus "%s".',$bus)); |
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.
What about rephrasing that one while we are here?
No handled message found in bus "%s".| return; | ||
| } | ||
| $buses =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.
$busIds?
| foreach ($container->findTaggedServiceIds($this->handlerTag,true)as$serviceId =>$tags) { | ||
| foreach ($tagsas$tag) { | ||
| if (isset($tag['bus']) && !\in_array($tag['bus'],$buses,true)) { |
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.
You can already usebus: command_bus for example if you define your bus that way:
framework:messenger:buses:command_bus:~
| foreach ($container->findTaggedServiceIds($this->handlerTag,true)as$serviceId =>$tags) { | ||
| foreach ($tagsas$tag) { | ||
| if (isset($tag['bus']) && !\in_array($tag['bus'],$buses,true)) { | ||
| thrownewRuntimeException(sprintf('Unknown bus "%s" on service "%s" tag "%s" with "bus" attribute. Known buses are "%s"',$tag['bus'],$serviceId,$this->handlerTag,implode(',',$buses))); |
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 suggest we use the same format than the other exceptions, something like:
Invalid handler service "%s": bus "%s" does not exist (known ones are: %s)| } | ||
| privatefunctionregisterHandlers(ContainerBuilder$container) | ||
| privatefunctionregisterHandlers(ContainerBuilder$container,array$buses) |
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.
$busIds so it's clearer
| $handlersLocatorMapping['handler.'.$message] =newReference($serviceId); | ||
| $handlersLocatorMappingByBus =array(); | ||
| foreach ($busesas$bus) { | ||
| $handlersLocatorMappingByBus[$bus] =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.
$handlersLocatorMappingByBus =array_combine($busIds,array_fill(0,\count($busIds),array());
Not sure if it's more efficient though 😄
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.
Yes, I didn’t do it because I think it’s less explicit 😅
| $handlerResolver->replaceArgument(0, ServiceLocatorTagPass::register($container,$handlersLocatorMapping)); | ||
| foreach ($busesas$bus) { | ||
| $container->register($resolverName ="$bus.messenger.handler_resolver", ContainerHandlerLocator::class) | ||
| ->setArgument(0, ServiceLocatorTagPass::register($container,$handlersLocatorMappingByBus[$bus])) |
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.
Actually, I don't believe that you need to initialise the$handlersLocatorMappingByBus[$busId]s if you just do a$handlersLocatorMappingByBus[$busId] ?? array()
| returnnewReference($handlerId); | ||
| },$handlersIds))); | ||
| $chainHandler->setPrivate(true); | ||
| $serviceId =hash('sha1',$bus.$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.
I know it was done like that but... it needs to be changed. Can you prefix it the name of the service with.messenger.chain_handler. and useContainerBuilder::hash instead ofhash directly?
| putenv('COLUMNS='.(119 +strlen(PHP_EOL))); | ||
| } | ||
| publicfunctiontearDown() |
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.
protected, same for setup
sroze commentedMay 20, 2018
Thank you@ogizanagi. |
…, sroze)This PR was merged into the 4.1 branch.Discussion----------[Messenger] Allow to scope handlers per bus| Q | A| ------------- | ---| Branch? | 4.1 <!-- see below -->| Bug fix? | no| New feature? | yes <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | N/A <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | todo<img width="970" alt="screenshot 2018-05-15 a 18 34 11" src="https://user-images.githubusercontent.com/2211145/40070551-b36d1f50-586e-11e8-8a44-c6a1503312dd.PNG">This prevents from dispatching a message in the wrong bus. It works especially great with PSR-4 discovery in DI:```yamlcommand_handlers: namespace: App\Message\ resource: '%kernel.project_dir%/src/Message/*CommandHandler.php' tags: - { name: messenger.message_handler, bus: command_bus }query_handlers: namespace: App\Message\ resource: '%kernel.project_dir%/src/Message/*QueryHandler.php' tags: - { name: messenger.message_handler, bus: query_bus }```<details><summary>(or in some layered architecture)</summary>```yamlcommand_handlers: namespace: App\Application\ resource: '%kernel.project_dir%/src/Application/**/Handler/*CommandHandler.php' tags: - { name: messenger.message_handler, bus: command_bus }query_handlers: namespace: App\Application\ resource: '%kernel.project_dir%/src/Application/**/Handler/*QueryHandler.php' tags: - { name: messenger.message_handler, bus: query_bus }```</details>---It also updates (and add tests for) the `DebugCommand`, so we can inspect easily where is supposed to be handled each message.It's totally optional, and if the bus isn't provided, then it stays available in every bus.Commits-------fd810cd Uses `protected` for test functions9d658e9 [Messenger] Allow to scope handlers per bus
…sageSubscriber (sroze)This PR was merged into the 4.2-dev branch.Discussion----------[Messenger] Only subscribe to a given bus from the MessageSubscriber| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | ø#27275 introduced the ability to listen to only a few buses from the handler tag. This adds that ability directly from the message subscriber.It has also highlighted to me that most of the configuration can be done using `yield` (like the example I've added in this PR's tests) and that we could remove the support for other ways (especially the obscure `return [['method', -10]]` syntax) but I believe this should be done **in another pull-request** (that I'm happy to do after this one).Commits-------f60e409 Only subscribe to a given bus from the MessageSubscriber
Uh oh!
There was an error while loading.Please reload this page.
This prevents from dispatching a message in the wrong bus. It works especially great with PSR-4 discovery in DI:
(or in some layered architecture)
It also updates (and add tests for) the
DebugCommand, so we can inspect easily where is supposed to be handled each message.It's totally optional, and if the bus isn't provided, then it stays available in every bus.