WIP Allow tagging handlers per bus

[benglass]

Addresses #19

There are a number of different possible ways that this feature could be implemented. Currently the bundle places all tagged handlers into a single instance of ContainerBasedHandlerLocator and relies on the user to register this handler as a middleware of their bus (or use the default tactician.middleware.command_handler service which includes it).

The approach I took was to leave that as-is for any handlers which are explicitly tagged as belonging to a specific bus.

However if the user tags a handler for a specific bus by using the "bus" key in the tag definition then this code does the following:

1. Do not add the handler to the default locator
2. Create a new service called tactician.handler.locator.symfony.bus_name with only those handlers tagged explicitly for that bus

This means that if you want as soon as you tag a handler for a specific bus, that bus will now ONLY include handlers explicitly tagged for it.

This also means that the user needs to be sure they configure the bus with an instance of CommandHandlerMiddleware since they can't use the default tactician.middleware.command_handler

There are some other possible approaches improvements I thought of, I wanted to post this WIP code and get feedback before I proceed any further. Here are some other ideas

1. Create a copy of the tactician.middleware.command_handler for each bus that has handlers tagged for it explicitly. This would be identical to the default command_handler but with a different locator. The user would still need to attach the right tactician.middleware.command_handler.BUS_NAME as the final middleware.
2. Automatically push the command_handler onto the middleware for the bus (deviates from the way this bundle works right now where you have to include the command handler as the final middleware)
3. If the user configures multiple buses we could throw an exception if they dont tag all their handler services (probably not a good idea)

@rosstuck @boekkooi Let me know your thoughts, It seems like at least the first item would be necessary to make this feature easy to use (automatically create a copy of the default command_handler middleware with the custom locator if the user tags any of their handler services)

[boekkooi]

Hi @benglass,

Nice start but I would go a little more redical on this (if @rosstuck agrees).
This is still very similar to what you are doing not but a little less conservative.

First I would say that no bus in the tag name means the default_bus.
To be able to know the default bus I would add a tactician.default_command_bus parameter in the container that is set to the default_bus.

Second I would create the handler and locator for each bus:

• tactician.commandbus.<name>.handler.locator
• tactician.commandbus.<name>.middleware.command_handler

This will give a user a clear understanding of what his bus maybe using because tactician.commandbus. is already created.

Third I would remove the following services and set them as aliases to the default bus:

• tactician.commandbus => tactician.commandbus.%tactician.default_command_bus%
• tactician.handler.locator.symfony => tactician.commandbus.%tactician.default_command_bus%.handler.locator
• tactician.middleware.command_handler => tactician.commandbus.%tactician.default_command_bus%.middleware.command_handler

This is mostly to avoid BC breaks.

What are your thought @rosstuck?

Great work and thank for doing this @benglass

[rosstuck]

Hey @benglass, this looks good and thanks for the quick turnaround! 😄

Addressing your original points:

1. Makes sense
2. This is tricky. Theoretically, the execution middleware is always the last unless someone's replaced it with a custom variant. We could make that configurable but swapping the handler locator around makes it more complicated. It would be more explicit to do as @boekkooi suggests and make folks wire them up directly, but I'm worried that's a bit convoluted for most folks. Perhaps there's two config modes to be had here? I'm undecided and open to suggestions on this one.
3. Yeah, forcing folks to tag with the bus would probably be unnecessary and a bit of a BC break. Also, honestly, I feel the configuration in the bundle per handler right now is a bit overly verbose and I'd like to find a way to reduce this on the default path.

The only other feedback I might throw out is perhaps it's possible for the bus value to optionally be an array of multiple bus names as well but that's a minor thing.

@boekkooi The proposal makes sense and I like the idea of generating the services, though do we need folks to wire them by hand? There probably won't be autocomplete for them because they're generated, so it's a bit annoying to get right. There IS already a default bus, it's just implicit but the alias approach makes sense.

Overall, I like this PR and want to merge it. I feel like the issues we're discussing have less to do with this specific PR and maybe underlying things about the way the bundle is structured and perhaps we should break those out into separate issues?

Will be offline the rest of the day, probably, but totally open to suggestions. Thanks again!

[benglass]

@boekkooi I like your suggestions and considered going more in this direction

I think any change to whether handler middleware is automatically added to the buses is a fairly significant BC change and as @rosstuck should probably be moved to a separate discussion.

I think keeping the current model of requiring the user to add the middleware themselves combined with auto-generation of the locator and command_handler services is the way to go. It avoids BC breaks while providing a more flexible tagging system.

• Use config value default_bus to determine which bus is the default if no bus is tagged
• Create a locator service for each bus tactician.commandbus.name.handler.locator
• Create a command handler service for each bus tactician.commandbus.name.middleware.command_handler
• Alias tactician.commandbus => tactician.commandbus.default_bus (This was already being done in the extension)
• Alias tactician.handler.locator.symfony => tactician.commandbus.default_bus.handler.locator
• Alias tactician.middleware.command_handler => tactician.commandbus.default_bus.middleware.command_handler

Regarding allowing the bus key to be an array, this is more inline with the default behavior right now but I am curious if we can think of a use case where users would want to put the same handler for the same command on multiple buses. I know this is how it works right now (registering a handler registers it on all buses) but my understanding of this was more that the bundle was expecting you would never want multiple handlers for the same command. That was just my interpretation. It seems like having the same handler for the same command on multiple buses could be confusing. Thoughts?

[benglass]

@boekkooi Regarding setting a parameter so we know which bus is the default, this may not be necessary because we can do $container->getExtensionConfig('tactician') in the compilr pass. Can you think of another reason to set it as a parameter?

[benglass]

Since all handler locators are now created in the compiler pass, this functionality has moved there

[benglass]

@boekkooi @rosstuck I took another pass at this. We are now creating bus-specific versions of the handler locator ad command handler middleware and registering the default versions as aliases to the old service names.

[boekkooi]

Would the following code not be better?

$busId = isset($attributes['bus']) ? $attributes['bus'] : $defaultBusId;

$this->abortIfInvalidBusId($busId, $container);

[benglass]

@boekkooi Good call, easy to miss stuff like this with refactorings. Updated.

[boekkooi]

@benglass Great work! @rosstuck 👍 for merging this 😄

[rosstuck]

Thanks for the hustle, fellas! I'll review tonight but it looks good.

Regarding the test boilerplate, maybe take a look at https://github.com/matthiasnoback/SymfonyDependencyInjectionTest but I haven't tried yet.

[benglass]

@rosstuck @boekkooi I should note I have not actually tested this in a working symfony app so let me go ahead and do that so I can confirm it actually works (as opposed to the tests all passing). I'll post when I can confirm its working.

Regarding the test boilerplate it seems like it would be nice to just test that the world is in the expected state after running the compiler pass rather than testing the internal implementation of the function (ensuring specific methods are called on the container). I don't have strong opinions on this but it felt like the way the tests mirrored the internals of the method made them brittle (I had to change every test any time I changed the compiler pass) and were testing things that arent necessarily what we care about. We care that certain services are created rather than that certain methods are called.

It does seem like using something like https://github.com/matthiasnoback/SymfonyDependencyInjectionTest#testing-a-compiler-pass would make testing the compiler pass easier and make the tests more descriptive of the desired behavior

[rosstuck]

This can be a class constant, bundle should only be 5.5+

[rosstuck]

+1 on class constant :)

[rosstuck]

Added a few comments, please let me know what you think about this. Beyond that, the only other thing needing updating is docs!

[benglass]

Reverted this to only check if bus is explicitly specified since otherwise it comes from the list of buses in the config which can't be wrong

[benglass]

@rosstuck Your comments are now addresses to prevent a BC break although I can't think of a case where you'd want 2 command buses with the same handlers for the same commands. Doesnt mean someone isnt using it this way though and probably best to avoid BC.

One thing I thought of is we are currently validating that if you attached the default command handler middleware that its last in the middleware stack. This logic doesnt account for the custom per-bus command handler middleware ids so it would catch if you attached a bus-specific command handler middleware but did not make it last. We could possibly fix this by matching on the string

tactician.commandbus.*.middleware.command_handler

Let me know if you think this is necessary

[rosstuck]

@benglass Thanks! I agree about the handlers, though I've learned to not assume unfortunately. :-/

With regards to the final check, perhaps it would be better to check by class name to see if it's an instance of the execution bus, but I'm not sure we can reliably do that with, say, the queueing strategy version. Therefore, perhaps it's better to deprecate the check entirely if we can't make it work and try to address it heavily in the (forthcoming) docs?

[tyx]

My issue #27 seems duplicated with this one in fact.

Why this one is not merged ? I could help instead of starting from scratch ?

[rosstuck]

Think it petered out on the last few review things and the docs. If you'd like to pick it up from there or @benglass hasn't been chased off by how negligent we've been, more than happy to go forward.

[tyx]

Ok so we are talking about :

• Remove the check on the command_handler middleware here https://github.com/thephpleague/tactician-bundle/blob/master/DependencyInjection/Configuration.php#L39
• Adding some doc about that need
• Handle method_inflector per bus

Bonus:

• Add behat test ? To be sure all stuff work with Symfony ;)

[rosstuck]

Completed and closed in favor of #19, since @tyx wrapped it up. Thanks enormously to you both. :)