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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Messenger] Allow to scope handlers per bus #27275

Merged
merged 2 commits into from May 20, 2018

Conversation

Projects
None yet
6 participants
@ogizanagi
Member

ogizanagi commented May 15, 2018

Q A
Branch? 4.1
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets N/A
License MIT
Doc PR todo

screenshot 2018-05-15 a 18 34 11

This prevents from dispatching a message in the wrong bus. It works especially great with PSR-4 discovery in DI:

command_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 }
(or in some layered architecture)
command_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 }

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.

@ro0NL

thx for adding the test :)

foreach ($handlers as $handler) {
$tableRows[] = array(sprintf(' handled by %s', $handler));
}
$mappings = $this->mapping;

This comment has been minimized.

@ro0NL

ro0NL May 16, 2018

Contributor

perhaps rename the property to $mappings as well (plural)

}
$mappings = $this->mapping;
if ($bus = $input->getArgument('bus')) {
$mappings = array($bus => $mappings[$bus]);

This comment has been minimized.

@ro0NL

ro0NL May 16, 2018

Contributor

missing isset? should throw/error for unknown buses no?

}, $handlersByMessage));
$debugCommandMapping = $handlersByBusAndMessage;
foreach ($buses as $bus) {
if (!isset($debugCommandMapping[$bus])) {

This comment has been minimized.

@ro0NL

ro0NL May 16, 2018

Contributor

see previous suggestion, i think this should be checked for at runtime (as well)

This comment has been minimized.

@ogizanagi

ogizanagi May 16, 2018

Member

Added check at runtime in debug command

foreach ($container->findTaggedServiceIds($this->handlerTag, true) as $serviceId => $tags) {
foreach ($tags as $tag) {
if (isset($tag['bus']) && !\in_array($tag['bus'], $buses, true)) {

This comment has been minimized.

@ro0NL

ro0NL May 16, 2018

Contributor

for being service ids, in theory it could resolve aliases. Allowing bus: message_bus instead of bus: 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.

This comment has been minimized.

@ogizanagi

ogizanagi May 16, 2018

Member

No strong opinion on this. Let's see what others think :)
Perhaps not in this PR anyway in order to not add more complexity.

This comment has been minimized.

@ro0NL

ro0NL May 16, 2018

Contributor

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 🤔

This comment has been minimized.

@sroze

sroze May 17, 2018

Member

You can already use bus: command_bus for example if you define your bus that way:

framework:
    messenger:
        buses:
            command_bus: ~
@sroze

Again, really great idea.

->setHelp(<<<'EOF'
The <info>%command.name%</info> command displays all messages that can be
dispatched using the message bus:
dispatched using the message buses:

This comment has been minimized.

@sroze

sroze May 17, 2018

Member

Would it make sense to add the bus example in the help?

$mapping = $this->mapping;
if ($bus = $input->getArgument('bus')) {
if (!isset($mapping[$bus])) {
throw new RuntimeException(sprintf('Invalid bus "%s". Known buses are %s.', $bus, implode(', ', array_keys($this->mapping))));

This comment has been minimized.

@sroze

sroze May 17, 2018

Member

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));

This comment has been minimized.

@sroze

sroze May 17, 2018

Member

What about rephrasing that one while we are here?

No handled message found in bus "%s".
return;
}
$buses = array();

This comment has been minimized.

@sroze

sroze May 17, 2018

Member

$busIds?

foreach ($container->findTaggedServiceIds($this->handlerTag, true) as $serviceId => $tags) {
foreach ($tags as $tag) {
if (isset($tag['bus']) && !\in_array($tag['bus'], $buses, true)) {

This comment has been minimized.

@sroze

sroze May 17, 2018

Member

You can already use bus: 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 ($tags as $tag) {
if (isset($tag['bus']) && !\in_array($tag['bus'], $buses, true)) {
throw new RuntimeException(sprintf('Unknown bus "%s" on service "%s" tag "%s" with "bus" attribute. Known buses are "%s"', $tag['bus'], $serviceId, $this->handlerTag, implode(', ', $buses)));

This comment has been minimized.

@sroze

sroze May 17, 2018

Member

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)
}
private function registerHandlers(ContainerBuilder $container)
private function registerHandlers(ContainerBuilder $container, array $buses)

This comment has been minimized.

@sroze

sroze May 17, 2018

Member

$busIds so it's clearer

$handlersLocatorMapping['handler.'.$message] = new Reference($serviceId);
$handlersLocatorMappingByBus = array();
foreach ($buses as $bus) {
$handlersLocatorMappingByBus[$bus] = array();

This comment has been minimized.

@sroze

sroze May 17, 2018

Member
$handlersLocatorMappingByBus = array_combine($busIds, array_fill(0, \count($busIds), array());

Not sure if it's more efficient though 😄

This comment has been minimized.

@ogizanagi

ogizanagi May 17, 2018

Member

Yes, I didn’t do it because I think it’s less explicit 😅

$handlerResolver->replaceArgument(0, ServiceLocatorTagPass::register($container, $handlersLocatorMapping));
foreach ($buses as $bus) {
$container->register($resolverName = "$bus.messenger.handler_resolver", ContainerHandlerLocator::class)
->setArgument(0, ServiceLocatorTagPass::register($container, $handlersLocatorMappingByBus[$bus]))

This comment has been minimized.

@sroze

sroze May 17, 2018

Member

Actually, I don't believe that you need to initialise the $handlersLocatorMappingByBus[$busId]s if you just do a $handlersLocatorMappingByBus[$busId] ?? array()

return new Reference($handlerId);
}, $handlersIds)));
$chainHandler->setPrivate(true);
$serviceId = hash('sha1', $bus.$message);

This comment has been minimized.

@sroze

sroze May 17, 2018

Member

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 use ContainerBuilder::hash instead of hash directly?

@ro0NL

ro0NL approved these changes May 19, 2018

@sroze

sroze approved these changes May 19, 2018

putenv('COLUMNS='.(119 + strlen(PHP_EOL)));
}
public function tearDown()

This comment has been minimized.

@chalasr

chalasr May 19, 2018

Member

protected, same for setup

@sroze

This comment has been minimized.

Member

sroze commented May 20, 2018

Thank you @ogizanagi.

@sroze sroze merged commit fd810cd into symfony:4.1 May 20, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

sroze added a commit that referenced this pull request May 20, 2018

feature #27275 [Messenger] Allow to scope handlers per bus (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     <!-- see https://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:

```yaml
command_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>

```yaml
command_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 functions
9d658e9 [Messenger] Allow to scope handlers per bus

@ogizanagi ogizanagi deleted the ogizanagi:messenger/handler_per_bus branch May 20, 2018

@fabpot fabpot referenced this pull request May 21, 2018

Merged

Release v4.1.0-BETA2 #27331

sroze added a commit that referenced this pull request Aug 28, 2018

feature #28275 [Messenger] Only subscribe to a given bus from the Mes…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment