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] Define multiple buses from the `framework.messenger.buses` configuration #26864

Merged
merged 1 commit into from Apr 25, 2018

Conversation

Projects
None yet
8 participants
@sroze
Member

sroze commented Apr 8, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #26652
License MIT
Doc PR symfony/symfony-docs#9617

Not everybody will benefit from having only one bus, especially with the CQRS-like usages. While keeping the extremely use of use of the single bus, this PR has the following:

  • Create multiple buses from the YAML configuration
  • Tag middleware only a specific buses
  • Register middlewares from the YAML configuration

Even if it's visible in the PR's tests, here's how it will look like, for a completely full-customised version:

framework:
    messenger:
        default_bus: commands
        buses:
            commands: ~
            events:
                middlewares:
                    - validation
                    - route_messages
                    - "Your\\Middleware\\Service"
                    - call_message_handler

A few things to note:

  1. The YAML configuration creates messenger.bus.[name] services for the buses.
  2. The YAML configuration for middleware just adds tags to the corresponding middlewares.
  3. If the middleware definition does not exists, it creates it. (without any magic on the arguments though, if it isn't auto-wirable, well... "your problem")
  4. In the PR, there is this "TolerateNoHandler" middleware that is a great example for event buses
@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Apr 8, 2018

IMHO, the framework.messenger.buses.[bus_name].middlewares node should be kept simple, with no priority handling (only the order matters). Otherwise, even if we'll have the debug command to help, it just brings confusion between services registered through the config and the ones through DI tags. It's far easier to read the list of middlewares applied on a bus on a single place.

I wonder if we shouldn't even remove the DI tag or just keep it for the default bus if framework.messenger.buses.[default_bus_name].middlewares is not set.

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Apr 8, 2018

I tend to prefer DI config over semantic middleware config, thus keep the tag.

Concerning attributes i'd say bus: string|null which either applies to the given or default bus. No big fan of the current explode trick compared to multiple tags.

Last but not least, i think 1 (default) bus should be configured minimum.

Alternatively it could wire pre-defined middlewares on a per-case basis, e.g.

framework:
    messenger:
        buses:
            events:
                tolerate_no_handler: true
@sroze

This comment has been minimized.

Member

sroze commented Apr 8, 2018

IMHO, the framework.messenger.buses.[bus_name].middlewares node should be kept simple, with no priority handling (only the order matters).

That's how I started it. The problem with this approach is how to place a middleware between/before/after the ones that are configured with the tags (and need priorities). I don't see any harm in being able to configure the priority, especially that when you don't, the configuration syntax is super easy. (i.e. by default, it's all middlewares priority 0, in the order in which you've put them in the configuration).

Concerning attributes i'd say bus: string|null which either applies to the given or default bus. No big fan of the current explode trick compared to multiple tags.

It's actually convenient to do so within the code base to simplify the handling of either a middleware is only applied to a given set of buses or all of them, hence me doing this way here. I don't mind changing it but I don't have a strong argument in doing so tbh.

Last but not least, i think st least 1 (default) bus should be configured.

That's the case if you don't configure your owns. If you do, then I'd argue it's now up to you :)

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Apr 8, 2018

That's the case if you don't configure your owns.

Right, my bad. I missed the default bus entry in config :)

I don't have a strong argument in doing so tbh

IMHO it's more pure :) but no strong opinion either.

About framework.messenger.buses.[bus_name].middlewares

You have to configure a service anyway, having to specify it here again is double bookkeeping, also this is not the place to infer new service definitions or so. IMHO :)

Having a single way of doing things is nice, and given you need to define the service i'd say tag it as a middleware as well, if needed.

SO we should either favor the tag or [bus_name].middlewares: [list of service ids].

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Apr 8, 2018

The problem with this approach is how to place a middleware between/before/after the ones that are configured with the tags (and need priorities).

That's why I'd suggest removing the tag 😄
The harm is not about having to handle a priority attribute in the config, but about allowing to configure things from 2 different places. I wouldn't allow both DI and semantic config for configuring middlewares.
I don't see much benefits to the DI tag for middlewares, while the semantic config is expressive and already allows to handle the stack order. That's how work the tactician-bundle. Never had to complain about it.

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Apr 8, 2018

A tag seems more flexible though :) considering third parties who want to register a middleware. Or put different what about the default middlewares currently?

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Apr 8, 2018

considering third parties who want to register a middleware.

That's the point to me: it's only a userland config. No third party implied. A third party could just help registering a default service to use, or provide a factory.

Or put different what about the default middlewares currently?

They can just be configured as default values for the middlewares node.
If the user want to add a middleware before or after, they'll have to be explicit about it.

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Apr 8, 2018

Well.. i was considering exactly that, register/tag a middleware to capture any message dispatched and pass it to a service.

@sroze

This comment has been minimized.

Member

sroze commented Apr 8, 2018

I think that your conversation is a good example of why we need to keep both configurations.

@ogizanagi As far as I understand, your concern is that it can introduce some complexity about knowing where/how the middlewares are configuration: I appreciate that but I don't believe this will be the case as it is basically a question of how you, as a developer, use these configuration mechanisms. Though, if we see a large proportion of issues coming that are related to that, we can later decide which one to deprecate I guess.

@romaricdrigon

This comment has been minimized.

romaricdrigon commented Apr 9, 2018

Hello all,
I would also be in favor of removing the tag. The duplicated config will be hard to trace, and can lead to side effect.
Imagine I have an application 2 buses. Now I install a bundle that provides a middleware, as a tagged service, which I want to add to only one of the bus. How to deal with such situation? How to easily unregister the middleware from one bus?

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Apr 9, 2018

At this point im 👍 for keeping both configs (DI+semantic).

Please dont remove tagging middlewares, it makes it real hard to hook in and basically defeats the feature as decorating the bus at this point is probably easier 😂

Now I install a bundle that provides a middleware, as a tagged service, which I want to add to only one of the bus. How to deal with such situation? How to easily unregister the middleware from one bus?

It's probably the bundle's intend to enable the middleware on all buses. Otherwise it should allow you to configure the bus service to use. I.e. How to easily unregister the middleware from one bus? seems highly hypothetical.

Also we never had this debate with bundles tagging e.g. kernel.event_listener etc. You would simply remove the definition if it doesnt fit then too.

@ro0NL ro0NL referenced this pull request Apr 9, 2018

Merged

Support Symfony Messenger #134

@sroze

This comment has been minimized.

Member

sroze commented Apr 9, 2018

I've rebased the pull-request so it contains the new middleware tag.

@sroze

This comment has been minimized.

Member

sroze commented Apr 9, 2018

@ogizanagi What if you could enable/disable tags from the framework.messenger.tags or something? 🤔

@sroze

This comment has been minimized.

Member

sroze commented Apr 9, 2018

If we drop tags, "bundles" could use the prependConfiguration to have some sort of automated configuration.

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Apr 9, 2018

Im really curious what others think about @ogizanagi's statement:

it's only a userland config. No third party implied

I tend to disagree with that. Why is a bus middleware "userland-config" and a kernel event listener not?

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Apr 9, 2018

So, we've talked a bit on Slack with @sroze today. Here's mostly what I shared with him about my point of view:

Here is how I see a typical configuration would look like with semantic config only and removing the DI tag:

framework:
  messenger:
    buses:
      commands:
        middlewares:
          - logging # fwb alias
          - validation # fwb alias
          # DoctrineBundle transaction middleware.
          # This demonstrates support of factories we could provide, i.e the doctrine_transaction_middleware
          # is an abstract service the FrameworkBundle would decline with following arguments
          # (here "default" being the manager name to use. Only simple scalar arguments, i.e no services references)
          - doctrine_transaction_middleware: [default]
          # Third-parties would just have to provide a service id (or factories for most custom needs) and document usage.
          # No automatic middleware registering, the user opt-in explicitly for each bus.
          - msg_php.console_message_subscriber_middleware
          # userland middleware service id
          - app.messenger.my_custom_middleware
          - message_handler # alias
      events:
        middlewares:
          - logging
          - tolerate_no_handler # fwb alias
          - message_handler

where logging, validation, tolerate_no_handler & message_handler are aliases handled by the FrameworkBundle for convenience, which also removes by the same occasion the need for the messenger.bus.middlewares.validation key from #26648.

Everything is configured in one place and explicit. Nothing behind the scene. No need for priority. No need for third-parties to register middlewares themselves (just to provide and document middleware services ids/factories).

Supporting both config & DI tag means decentralizing the middleware stack, struggling with priorities (even if the debug command would help) and allowing third-parties to register middleware automagically (which I really don't think is useful).
A parallel (limited though) could be made with Monolog's handlers stack: no DI tag is provided AFAIK to register handlers (And thank goodness, that's fortunate. That'll only bring more confusion 😄).

Even the one legit use-case I see within the framework can be solved differently: the MessengerDataCollector has to be registered automatically on debug mode. The solution without tag is simple: just decorate the HandleMessageMiddleware just like we do with validator & event dispatcher collectors for instance.

Finally, I used Tactician for more than 2 years. This is how the middlewares are registered by the bundle (but simpler, no factory support). Never had to complain and I'm not aware of
any issue asking for a DI tag. So, what about starting with this?

Anyway, if a majority still prefers keeping the DI tag, then I'd be in favor of no semantic config at all. Supporting both just seems confusing and pointless to me. :)

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Apr 9, 2018

allowing third-parties to register middleware automagically (which I really don't think is useful).

ok, more thinking about it and decorating instead sounds reasonable, if not better :) in my case it's also about capturing dispatch messages like data collector does.

Config looks good 👍 perhaps denote built-in aliases (non service ids i guess) with :logging, etc. Or put different who wins / what happens if a logging service is defined.

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Apr 9, 2018

To me, fwb known aliases would always win and are unlikely to collide with actual middlewares' ids, but yes, I guess a way to denote built-in aliases would be nice :)

@chalasr

+1 for dropping the tag, not worth the confusion it could cause IMHO.

return;
}
$busesNames = $container->getParameter('messenger.buses_names');

This comment has been minimized.

@chalasr

chalasr Apr 10, 2018

Member

the parameter should be removed once used, no need to expose user config

@@ -1448,6 +1449,26 @@ private function registerMessengerConfiguration(array $config, ContainerBuilder
$loader->load('messenger.xml');
$container->setParameter('messenger.buses_names', array_keys($config['buses']));
foreach ($config['buses'] as $name => $bus) {
$container->setDefinition($busId = 'messenger.bus.'.$name, (new Definition(MessageBus::class, array(array())))->addTag('messenger.bus')->setPublic(true));

This comment has been minimized.

@chalasr

chalasr Apr 10, 2018

Member

what about keeping them private? IMHO there is no reason to not use proper dependency injection for controllers as of 3.4

This comment has been minimized.

@chalasr

chalasr Apr 10, 2018

Member

I'd put an abstract messenger.bus definition in xml to be extended here also, helps for discoverability (e.g. with phpstorm plugin that links to definitions)

}
$container->getDefinition($middleware['service'])->addTag('messenger.bus_middleware', array(
'bus' => $name,

This comment has been minimized.

@chalasr

chalasr Apr 10, 2018

Member

should be on one line

foreach ($config['buses'] as $name => $bus) {
$container->setDefinition($busId = 'messenger.bus.'.$name, (new Definition(MessageBus::class, array(array())))->addTag('messenger.bus')->setPublic(true));
if ($name == $config['default_bus']) {

This comment has been minimized.

@chalasr

chalasr Apr 10, 2018

Member

better use strict comparison

foreach ($container->findTaggedServiceIds('messenger.bus_middleware') as $serviceId => $tags) {
foreach ($tags as $tag) {
$priority = $tag['priority'] ?? 0;
$buses = isset($tag['bus']) ? explode(',', $tag['bus']) : $busesNames;

This comment has been minimized.

@chalasr

chalasr Apr 10, 2018

Member

Not fond of the special syntax, AFAIK we are used to require adding as much tags as needed e.g.

App\SomeMiddleware:
    tags:
        - { name: "messenger.bus_middleware", bus: "query" }
        - { name: "messenger.bus_middleware", bus: "command" }
if (!$container->has($middleware['service'])) {
$container->setDefinition($middleware['service'], new Definition($middleware['service']));
}
$container->setDefinition(

This comment has been minimized.

@ro0NL

ro0NL Apr 12, 2018

Contributor

so in user-land you need a compiler pass, find all buses by tag (messenger.bus), and decorate each one.

or is it better to decorate call_message_handler once?

This comment has been minimized.

@sroze

sroze Apr 12, 2018

Member

I'd say it depends on your use-case. Both approaches would work.

@sroze

This comment has been minimized.

Member

sroze commented Apr 12, 2018

@ogizanagi @chalasr I've updated the PR. I agree with you, it's actually a better DX to have only one place for the configuration.

With the way the PR is right now, if you define you own middlewares, you override the default "logging", "call_message_handler" and "route_messages" handlers, so you need to define them. It feels like it could be a source of troubles for the users... What's your point of view on what we could/should do arround that?

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Apr 12, 2018

you override the default "logging", "call_message_handler" and "route_messages" handlers, so you need to define them.

I don't think it's an issue. The only thing we should maybe do is hint the user if the call_message_handler is not part of the list. Or automatically adding it but that would be a bit strange to treat it differently from other middlewares.

@@ -51,16 +38,14 @@
<service id="messenger.transport.default_decoder" alias="messenger.transport.serialize_message_with_type_in_headers" public="true" />
<!-- Logging & Debug -->
<service id="messenger.middleware.debug.logging" class="Symfony\Component\Messenger\Debug\LoggingMiddleware">
<service id="messenger.middleware.logging" class="Symfony\Component\Messenger\Middleware\LoggingMiddleware">

This comment has been minimized.

@ogizanagi

ogizanagi Apr 12, 2018

Member

IMHO, each middleware service definition should be abstract and a concrete instance created for each bus in order to be able to decorate them independently if needed.

This comment has been minimized.

@sroze

sroze Apr 13, 2018

Member

What's the use case of decorating a middleware?

This comment has been minimized.

@ogizanagi

ogizanagi Apr 13, 2018

Member

There could be many. The one I may suggest after this one is an ActivationMiddlewareDecorator, that will decorate a middleware to conditionally enable it. For instance, in debug mode only. That could solve #26901 differently.

->end()
->children()
->scalarNode('service')->isRequired()->end()
->integerNode('priority')->defaultValue(0)->end()

This comment has been minimized.

@ogizanagi

ogizanagi Apr 12, 2018

Member

can be removed now? 😄The service key wouldn't be necessary anymore neither so it can be simplified.

@sroze

This comment has been minimized.

Member

sroze commented Apr 17, 2018

And rebased 👍

@chalasr

1 minor comment 👍
doc issue created symfony/symfony-docs#9617

}
$container->setDefinition(
$busId.'.trace',

This comment has been minimized.

@chalasr

chalasr Apr 18, 2018

Member

the convention is debug.$id

This comment has been minimized.

@sroze

sroze Apr 18, 2018

Member

debug.traced.$id ?

This comment has been minimized.

@chalasr

chalasr Apr 18, 2018

Member

why not :)

This comment has been minimized.

@sroze

sroze Apr 18, 2018

Member

Updated to "follow the convention" 😉

return;
}
if (!$container->getParameter('kernel.debug') || !$container->hasAlias('logger')) {
$container->removeDefinition('messenger.middleware.debug.logging');
$collectorDefinition = $container->getDefinition('messenger.data_collector');

This comment has been minimized.

@chalasr

chalasr Apr 18, 2018

Member

kind of couples the pass to WebProfiler (basically Framework), I would skip if the definition does not exist

This comment has been minimized.

@sroze

sroze Apr 18, 2018

Member

Fair enough.

@Nyholm

This comment has been minimized.

Member

Nyholm commented Apr 20, 2018

I've just started to test this PR on a project. It works well. It is not easy to configure but it is flexible. With "easy" I mean: It would be nice to have:

  busses: 
      acme:
          type: command_bus

Im not sure that is a realistic because whatever a "command_bus" is for me it might not be the same for you. So Im +1 with the config.


I get an error though:

    messenger:
        default_bus: events
        buses:
            events:
                middlewares:
                    - route_messages
                    - 'Symfony\Component\Messenger\Middleware\TolerateNoHandler'
                    - call_message_handler
In ResolveChildDefinitionsPass.php line 71:
                                                                                                                          
  Service "messenger.bus.events.middleware.Symfony\Component\Messenger\Middleware\TolerateNoHandler": Parent definition   
  "Symfony\Component\Messenger\Middleware\TolerateNoHandler" does not exist.                                              
                                                                                       

If I change my config to the following, it will work:

    messenger:
        default_bus: events
        buses:
            events:
                middlewares:
                    - route_messages
                    - 'messenger.middleware.tolerate_no_handler'
                    - call_message_handler
@sroze

This comment has been minimized.

Member

sroze commented Apr 20, 2018

You are right, my example in the original pull-request description wasn't accurate anymore (updated now). The reason you had this exception is because it tried to create an instance of the middleware for this bus based on the parent definition that you define.

In the example you mentioned, you could even:

- 'messenger.middleware.tolerate_no_handler'
+ tolerate_no_handler

Regarding the buses.[name].type configuration, I like the idea but is obviously trickier to settle on what should be in which. Therefore I'd argue this shouldn't be in this PR at least 🙃 (could be a good 3rd party bundle as well).

@weaverryan

This comment has been minimized.

Member

weaverryan commented Apr 21, 2018

Hey guys!

Sorry to come late after so much work.

With the way the PR is right now, if you define you own middlewares, you override the default "logging", "call_message_handler" and "route_messages" handlers, so you need to define them. It feels like it could be a source of troubles for the users... What's your point of view on what we could/should do around that?

I actually think this is a huge problem. Imagine a user wants to add a new middleware: they setup their config, set middleware to [my_new_middleware] and suddenly things that worked before don't - my message isn't being handled or routed.

I'm not sure of the best solution - I think the argument around tags is valid, and the fact that experience with TacticianBundle has validated their approach a bit. Actually, one solution would be to be more like TacticianBundle: have zero default middleware out-of-the-box. Then, in the recipe, include the 3 middleware in messenger.yaml with comments. When a user creates a second bus, they'll copy and paste.

@@ -1052,6 +1053,21 @@ function ($a) {
->end()
->end()
->end()
->scalarNode('default_bus')->defaultValue('default')->end()

This comment has been minimized.

@weaverryan

weaverryan Apr 21, 2018

Member

Could we default this to null, then in the extension:

  1. If there is 1 extension, just use it
  2. If there are multiple, throw a clear exception that the default_bus key needs to be defined?

I could see a situation where someone sees:

The default bus named "default" is not defined. Define it or change the default bus name.

And thinking, where did default come from?

This comment has been minimized.

@sroze

sroze Apr 21, 2018

Member

Yep, I like that idea 👍

This comment has been minimized.

@sroze

sroze Apr 21, 2018

Member

Changed it this way.

@sroze

This comment has been minimized.

Member

sroze commented Apr 21, 2018

Actually, one solution would be to be more like TacticianBundle: have zero default middleware out-of-the-box.

This means we can't auto-activate the messenger when we know it's installed (i.e. the MessageBus class exists) and we would only rely on the YAML configuration. It's pretty good to be able to do this (i.e. doing some configuration via the recipe) but it's more boilerplate (and much harder to maintain, once the recipe is installed, you're done...)

What about having the followingmiddlewares per bus:

  • logging
  • validation
  • all the middlewares in the buses.[name].middlewares configuration
  • route_messages
  • call_message_handler

As a developer, you can disable the "default middlewares" with:

buses:
    [name]:
        default_middlewares: false
@sroze

This comment has been minimized.

Member

sroze commented Apr 21, 2018

I've pushed 3 commits:

  1. 0d78a0e that allows middlewares to be services (no requirement for them to be abstract)
  2. d39604e forces to have a matching default_bus only when using more than one bus. Otherwise, well, it take the name of that only bus.
  3. 8d16cda my favourite alternative to replace all the configured middlewares: we have a set of hard-coded default middlewares and you can squeeze yours in between. If you really need to (I believe this will be rare) you can disable the default middlewares with default_middlewares: false.
$defaultMiddlewares = array('before' => array('logging'), 'after' => array('route_messages', 'call_message_handler'));
if ($config['middlewares']['validation']['enabled']) {

This comment has been minimized.

@ro0NL

ro0NL Apr 21, 2018

Contributor

at this point..shouldnt we let users this configure as a regular middleware, per bus, if needed? Extra configs feels a bit too much config IMHO. Perhaps try to prepend either way like 'logging'.

This comment has been minimized.

@ro0NL

ro0NL Apr 21, 2018

Contributor

Or put different, it's a confusing config as it relies on default_middlewares also.

This comment has been minimized.

@weaverryan

weaverryan Apr 21, 2018

Member

This one special middleware handling is indeed a little bit weird. Is there any special reason when you would not want this middleware? Could it ever cause issues? My instinct is to always include it when default_middlewares is enabled. If you really don't want it for some reason, you can set that config to false (unless there's a somewhat common use-case for not wanting this middleware).

This comment has been minimized.

@ro0NL

ro0NL Apr 21, 2018

Contributor

I tend to opt-in to a validation middleware.. im not sure validating messages by default makes sense.

Also it's not like adding - validation to your middlewares (per bus) is "bad config" or so IMHO :)

This comment has been minimized.

@sroze

sroze Apr 22, 2018

Member

Is there any special reason when you would not want this middleware?

Yes, when the component is not included.

I tend to opt-in to a validation middleware.. im not sure validating messages by default makes sense.

Let's do this. So it is explicit and removes some extra configuration in there.

@weaverryan

Other than one small comment, yea, I like this approach. I can't think of any other part of the framework that has this exact problem, and so, the solution is also a bit unique. But, it's both practical (works immediately for the 90%) and extensible.

$defaultMiddlewares = array('before' => array('logging'), 'after' => array('route_messages', 'call_message_handler'));
if ($config['middlewares']['validation']['enabled']) {

This comment has been minimized.

@weaverryan

weaverryan Apr 21, 2018

Member

This one special middleware handling is indeed a little bit weird. Is there any special reason when you would not want this middleware? Could it ever cause issues? My instinct is to always include it when default_middlewares is enabled. If you really don't want it for some reason, you can set that config to false (unless there's a somewhat common use-case for not wanting this middleware).

@weaverryan

This comment has been minimized.

Member

weaverryan commented Apr 22, 2018

Ping @symfony/deciders!

I'm 👍 for this approach, and it's a big PR, so we need to get it merged in soon. The most important aspect is how middleware are added. The final decision about that is explained very well here: #26864 (comment)

@ro0NL

now we have the messenger.bus tag instead of tagging middlewares.. from a vendor perspective im leaning to wire my own bus, thus tag it. Given that i think it should work like any other bus that comes from config.

There's no framework-way of asking for an event or command bus, and i want to avoid configuration for that on my side. So i'll autowire the default bus to be used for commands, but then i still need one for events. Alternatively i could wire a one-size-fits-all bus by injecting all tagged buses, and break on first successful bus handling a message.

}
if ('messenger.middleware.validation' === $messengerMiddlewareId && !$container->has('validator')) {
throw new RuntimeException('The Validation middleware is only available when the Validator component is installed and enabled. Try running "composer require symfony/validator".');

This comment has been minimized.

@ro0NL

ro0NL Apr 22, 2018

Contributor

would be nice if we can improve such error messages at the DI service level

This comment has been minimized.

@sroze

sroze Apr 22, 2018

Member

I think it's a wider question within core. That's the best way I found to keep the great DX while using this middleware without the Validator component. I would keep it here.

This comment has been minimized.

@ro0NL

ro0NL Apr 22, 2018

Contributor

See #27004

This comment has been minimized.

@ogizanagi

ogizanagi Apr 23, 2018

Member

IMHO, this still belongs to the Fwb extension where we have all the keys to detect if validation is installed and enabled. The messenger.middleware.validation is really specific to the Fwb wiring, so the pass alone doesn't have to deal with this unless strictly necessary.

This comment has been minimized.

@sroze

sroze Apr 24, 2018

Member

I see the point that this has little to do in here. Moved to the FrameworkBundle 👍

}
if ($container->hasDefinition('messenger.data_collector')) {
$this->registerBusToCollector($container, $busId, $tags[0]);

This comment has been minimized.

@ro0NL

ro0NL Apr 22, 2018

Contributor

should we iterate $tags as well or throw in case of >1?

This comment has been minimized.

@sroze

sroze Apr 22, 2018

Member

I don't believe there is a use-case for a bus to be tagged multiple times 🤔

$container->setAlias(MessageBusInterface::class, $busId);
}
$container->setDefinition(

This comment has been minimized.

@ro0NL

ro0NL Apr 22, 2018

Contributor

should/can we process this at the compile pass level?

This comment has been minimized.

@sroze

sroze Apr 22, 2018

Member

Yes, very good idea! So we add it only when we need it 💃

@ogizanagi

👍 With only minor comments. Thanks for considering what we discussed on Slack.
I'm not found of the default middlewares vs always using explicit configuration but I guess that's a good tradeoff.

'default_middlewares' => false,
'middlewares' => array(
'route_messages',
'Symfony\\Component\\Messenger\\Middleware\\TolerateNoHandler',

This comment has been minimized.

@ogizanagi

ogizanagi Apr 23, 2018

Member

It's a bit misleading reading the tests and seing this FQCN while the actual middleware exists and has a service id that should be used (or the shortcut), not the FQCN. Is it really relevant to test a FQCN here?

This comment has been minimized.

@sroze

sroze Apr 24, 2018

Member

Good point; changed.

{
private $traceableBuses = array();
public function registerBus(string $name, TraceableMessageBus $bus)

This comment has been minimized.

@ogizanagi

ogizanagi Apr 23, 2018

Member

Why not injected in the constructor?

This comment has been minimized.

@sroze

sroze Apr 24, 2018

Member

This is to match with what already exists like the CacheDataCollector for example.

$container->getParameterBag()->remove($busMiddlewaresParameter);
}
if ($container->getParameter('kernel.debug') && $container->hasDefinition('messenger.data_collector')) {

This comment has been minimized.

@ogizanagi

ogizanagi Apr 23, 2018

Member

Is the $container->getParameter('kernel.debug') check useful, or $container->hasDefinition('messenger.data_collector') would be enough?

This comment has been minimized.

@sroze

sroze Apr 24, 2018

Member

True, we can remove the $container->getParameter('kernel.debug') now.

}
if ('messenger.middleware.validation' === $messengerMiddlewareId && !$container->has('validator')) {
throw new RuntimeException('The Validation middleware is only available when the Validator component is installed and enabled. Try running "composer require symfony/validator".');

This comment has been minimized.

@ogizanagi

ogizanagi Apr 23, 2018

Member

IMHO, this still belongs to the Fwb extension where we have all the keys to detect if validation is installed and enabled. The messenger.middleware.validation is really specific to the Fwb wiring, so the pass alone doesn't have to deal with this unless strictly necessary.

$config['default_bus'] = array_keys($config['buses'])[0];
}
$defaultMiddlewares = array('before' => array('logging'), 'after' => array('route_messages', 'call_message_handler'));

This comment has been minimized.

@ogizanagi

ogizanagi Apr 23, 2018

Member

Isn't validation supposed to be in defaultMiddlewares['before'] if the component is enabled, as described in #26864 (comment) ?

This comment has been minimized.

@ro0NL

This comment has been minimized.

@ogizanagi

ogizanagi Apr 23, 2018

Member

Alright, let's stick with it :)

@sroze

This comment has been minimized.

Member

sroze commented Apr 25, 2018

Repushed the last commit to make AppVeyor green again 💚

@sroze

This comment has been minimized.

Member

sroze commented Apr 25, 2018

Let's merge! Thank you all for your feedback 🎉

@sroze sroze merged commit e5deb84 into symfony:master Apr 25, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

sroze added a commit that referenced this pull request Apr 25, 2018

feature #26864 [Messenger] Define multiple buses from the `framework.…
…messenger.buses` configuration (sroze)

This PR was squashed before being merged into the 4.1-dev branch (closes #26864).

Discussion
----------

[Messenger] Define multiple buses from the `framework.messenger.buses` configuration

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #26652
| License       | MIT
| Doc PR        | symfony/symfony-docs#9617

Not everybody will benefit from having only one bus, especially with the CQRS-like usages. While keeping the extremely use of use of the single bus, this PR has the following:

- Create multiple buses from the YAML configuration
- Tag middleware only a specific buses
- Register middlewares from the YAML configuration

Even if it's visible in the PR's tests, here's how it will look like, for a completely full-customised version:
```yaml
framework:
    messenger:
        default_bus: commands
        buses:
            commands: ~
            events:
                middlewares:
                    - validation
                    - route_messages
                    - "Your\\Middleware\\Service"
                    - call_message_handler
```

A few things to note:
1. The YAML configuration creates `messenger.bus.[name]` services for the buses.
2. The YAML configuration for middleware just adds tags to the corresponding middlewares.
3. If the middleware definition does not exists, it creates it. (without any magic on the arguments though, if it isn't auto-wirable, well... "your problem")
4. In the PR, there is this "TolerateNoHandler" middleware that is a great example for event buses

Commits
-------

e5deb84 [Messenger] Define multiple buses from the `framework.messenger.buses` configuration

@sroze sroze deleted the sroze:multiple-buses branch May 3, 2018

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

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

feature #27128 [Messenger] Middleware factories support in config (og…
…izanagi)

This PR was squashed before being merged into the 4.1 branch (closes #27128).

Discussion
----------

[Messenger] Middleware factories support in config

| Q             | A
| ------------- | ---
| Branch?       | master <!-- 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

Following #26864, this would allow to configure easily the middlewares by using an abstract factory definition to which are provided simple arguments (just scalars, no services references).

For instance, here is how the DoctrineBundle would benefit from such a feature (also solving the wiring of the `DoctrineTransactionMiddleware` reverted in #26684):

```yaml
framework:
    messenger:
      buses:
        default:
          middleware:
            - logger
            - doctrine_transaction_middleware: ['entity_manager_name']
```

where `doctrine_transaction_middleware` would be an abstract factory definition provided by the doctrine bundle:

```yml
services:

    doctrine.orm.messenger.middleware_factory.transaction:
      class: Symfony\Bridge\Doctrine\Messenger\DoctrineTransactionMiddlewareFactory
      arguments: ['@doctrine']

    doctrine_transaction_middleware:
      class: Symfony\Bridge\Doctrine\Messenger\DoctrineTransactionMiddleware
      factory: ['@doctrine.orm.messenger.middleware_factory.transaction', 'createMiddleware']
      abstract: true
      # the default arguments to use when none provided from config.
      # i.e:
      #   middlewares:
      #     - doctrine_transaction_middleware: ~
      arguments: ['default']
```

and is interpreted as:

```yml
buses:
    default:
        middleware:
            -
                id: logger
                arguments: {  }
            -
                id: doctrine_transaction_middleware
                arguments:
                    - entity_manager_name
        default_middleware: true
```

---

<details>

<summary>Here is the whole config reference with these changes: </summary>

```yaml
# Messenger configuration
messenger:
    enabled:              true
    routing:

        # Prototype
        message_class:
            senders:              []
    serializer:
        enabled:              true
        format:               json
        context:

            # Prototype
            name:                 ~
    encoder:              messenger.transport.serializer
    decoder:              messenger.transport.serializer
    adapters:

        # Prototype
        name:
            dsn:                  ~
            options:              []
    default_bus:          null
    buses:

        # Prototype
        name:
            default_middleware:  true
            middleware:

                # Prototype
                -
                    id:                   ~ # Required
                    arguments:            []
```

</details>

Commits
-------

f5ef421 [Messenger] Middleware factories support in config

symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request May 14, 2018

feature #27128 [Messenger] Middleware factories support in config (og…
…izanagi)

This PR was squashed before being merged into the 4.1 branch (closes #27128).

Discussion
----------

[Messenger] Middleware factories support in config

| Q             | A
| ------------- | ---
| Branch?       | master <!-- 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

Following symfony/symfony#26864, this would allow to configure easily the middlewares by using an abstract factory definition to which are provided simple arguments (just scalars, no services references).

For instance, here is how the DoctrineBundle would benefit from such a feature (also solving the wiring of the `DoctrineTransactionMiddleware` reverted in symfony/symfony#26684):

```yaml
framework:
    messenger:
      buses:
        default:
          middleware:
            - logger
            - doctrine_transaction_middleware: ['entity_manager_name']
```

where `doctrine_transaction_middleware` would be an abstract factory definition provided by the doctrine bundle:

```yml
services:

    doctrine.orm.messenger.middleware_factory.transaction:
      class: Symfony\Bridge\Doctrine\Messenger\DoctrineTransactionMiddlewareFactory
      arguments: ['@doctrine']

    doctrine_transaction_middleware:
      class: Symfony\Bridge\Doctrine\Messenger\DoctrineTransactionMiddleware
      factory: ['@doctrine.orm.messenger.middleware_factory.transaction', 'createMiddleware']
      abstract: true
      # the default arguments to use when none provided from config.
      # i.e:
      #   middlewares:
      #     - doctrine_transaction_middleware: ~
      arguments: ['default']
```

and is interpreted as:

```yml
buses:
    default:
        middleware:
            -
                id: logger
                arguments: {  }
            -
                id: doctrine_transaction_middleware
                arguments:
                    - entity_manager_name
        default_middleware: true
```

---

<details>

<summary>Here is the whole config reference with these changes: </summary>

```yaml
# Messenger configuration
messenger:
    enabled:              true
    routing:

        # Prototype
        message_class:
            senders:              []
    serializer:
        enabled:              true
        format:               json
        context:

            # Prototype
            name:                 ~
    encoder:              messenger.transport.serializer
    decoder:              messenger.transport.serializer
    adapters:

        # Prototype
        name:
            dsn:                  ~
            options:              []
    default_bus:          null
    buses:

        # Prototype
        name:
            default_middleware:  true
            middleware:

                # Prototype
                -
                    id:                   ~ # Required
                    arguments:            []
```

</details>

Commits
-------

f5ef421474 [Messenger] Middleware factories support in config

symfony-splitter pushed a commit to symfony/messenger that referenced this pull request May 14, 2018

feature #27128 [Messenger] Middleware factories support in config (og…
…izanagi)

This PR was squashed before being merged into the 4.1 branch (closes #27128).

Discussion
----------

[Messenger] Middleware factories support in config

| Q             | A
| ------------- | ---
| Branch?       | master <!-- 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

Following symfony/symfony#26864, this would allow to configure easily the middlewares by using an abstract factory definition to which are provided simple arguments (just scalars, no services references).

For instance, here is how the DoctrineBundle would benefit from such a feature (also solving the wiring of the `DoctrineTransactionMiddleware` reverted in symfony/symfony#26684):

```yaml
framework:
    messenger:
      buses:
        default:
          middleware:
            - logger
            - doctrine_transaction_middleware: ['entity_manager_name']
```

where `doctrine_transaction_middleware` would be an abstract factory definition provided by the doctrine bundle:

```yml
services:

    doctrine.orm.messenger.middleware_factory.transaction:
      class: Symfony\Bridge\Doctrine\Messenger\DoctrineTransactionMiddlewareFactory
      arguments: ['@doctrine']

    doctrine_transaction_middleware:
      class: Symfony\Bridge\Doctrine\Messenger\DoctrineTransactionMiddleware
      factory: ['@doctrine.orm.messenger.middleware_factory.transaction', 'createMiddleware']
      abstract: true
      # the default arguments to use when none provided from config.
      # i.e:
      #   middlewares:
      #     - doctrine_transaction_middleware: ~
      arguments: ['default']
```

and is interpreted as:

```yml
buses:
    default:
        middleware:
            -
                id: logger
                arguments: {  }
            -
                id: doctrine_transaction_middleware
                arguments:
                    - entity_manager_name
        default_middleware: true
```

---

<details>

<summary>Here is the whole config reference with these changes: </summary>

```yaml
# Messenger configuration
messenger:
    enabled:              true
    routing:

        # Prototype
        message_class:
            senders:              []
    serializer:
        enabled:              true
        format:               json
        context:

            # Prototype
            name:                 ~
    encoder:              messenger.transport.serializer
    decoder:              messenger.transport.serializer
    adapters:

        # Prototype
        name:
            dsn:                  ~
            options:              []
    default_bus:          null
    buses:

        # Prototype
        name:
            default_middleware:  true
            middleware:

                # Prototype
                -
                    id:                   ~ # Required
                    arguments:            []
```

</details>

Commits
-------

f5ef421474 [Messenger] Middleware factories support in config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment