[Messenger] Add middleware allowing just for a single handler #28716

pamil · 2018-10-03T19:45:43Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	not yet

It's a quite common middleware, required for command buses.

PS. How do you manage to have autocompletion for PHPUnit classes when writing tests in Symfony since there is no PHPUnit in dependencies?

nicolas-grekas · 2018-10-04T08:16:20Z

src/Symfony/Component/Messenger/Middleware/AllowSingleHandlerMiddleware.php

+    {
+        $handler = $this->messageHandlerResolver->resolve($message);
+
+        if ($handler instanceof ChainHandler) {


I feel like this is tight coupling to a specific implementation: any other handler could implement a chained behavior, isn't it?
Why do we want to restrict the possibilities here?

Yes, this is coupled with the FrameworkBundle implementation. Unfortunately, I don't have any idea on how to make it better (I don't think that sharing the YAML configuration is better either). @nicolas-grekas do you have an idea? 🤔

Yeah, I also don't think this is the best solution, but I couldn't think of any other way (apart from introducing such a constraint into FrameworkBundle itself).

We want to restrict it only to one handler to prevent any issues with having two handlers handling the same command (which could happen by accident, especially if using autowiring and autoconfiguration).

sroze · 2018-10-07T10:13:16Z

src/Symfony/Component/Messenger/Middleware/AllowSingleHandlerMiddleware.php

+        $handler = $this->messageHandlerResolver->resolve($message);
+
+        if ($handler instanceof ChainHandler) {
+            throw new MoreThanOneHandlerForMessageException(sprintf('More than one handler for message "%s".', \get_class($message)));


The other "problem" (or challenge) with the current implementation is that we can't display which are the implementations. It would massively help the developer, isn't it?

ogizanagi · 2018-10-08T18:51:43Z

I wonder if running this check at runtime for each message dispatched really makes sense. It's not runtime dependent at all (hence, not really valid as a middleware), so perhaps we should rather just make it a bus option ?

sroze · 2018-10-21T13:06:31Z

I wonder if running this check at runtime for each message dispatched really makes sense. It's not runtime dependent at all (hence, not really valid as a middleware), so perhaps we should rather just make it a bus option ?

Sounds reasonable actually. An option and the bus and the MessengerPass to enforce it when registering messages. I'm 👍-ing it!

sroze · 2018-10-31T07:56:02Z

Note that now ChainHandler has been removed :) (see #29010)

Tobion · 2018-11-01T13:56:12Z

I wonder if running this check at runtime for each message dispatched really makes sense. It's not runtime dependent at all (hence, not really valid as a middleware), so perhaps we should rather just make it a bus option ?

Sounds reasonable actually. An option and the bus and the MessengerPass to enforce it when registering messages. I'm 👍-ing it!

I don't think this is really possible.

You would need to detect that handlers register for overlapping class hierarchies with all the parents and interfaces and wildcard * being taken into account. Does not seem feasible.
It still depends on which messages are dispatched. If I don't dispatch a message at all that could cause several handlers to be executed (maybe it's unused), then there is no problem at all. But you cannot know that at container compilation time.

It would be easy to implement as a new argument in

symfony/src/Symfony/Component/Messenger/Middleware/HandleMessageMiddleware.php

Line 28 in 9cd88b0

    
           public function __construct(HandlersLocatorInterface $handlersLocator, bool $allowNoHandlers = false)

ogizanagi · 2018-11-01T16:55:53Z

@Tobion : Sure, this comment is outdated and was referring to another era where the ChainHandler existed and everything was guessed at compilation-time. Perhaps you're right :)

You would need to detect that handlers register for overlapping class hierarchies with all the parents and interfaces and wildcard * being taken into account. Does not seem feasible.

Well, if you want to add this check, (I guess) you're likely to use simple objects without inheritance nor interface in your bus. This check at compilation-time based on MessengerPass::guessHandledClasses might still be valuable.

It still depends on which messages are dispatched. If I don't dispatch a message at all that could cause several handlers to be executed (maybe it's unused), then there is no problem at all. But you cannot know that at container compilation time.

If it's not used, then it should have been removed? This check isn't for a particular message, but for the whole bus. (I mean you're not asking if there is an issue with a specific message but if there isn't one in the whole bus which may be mis-configured).

nicolas-grekas · 2018-11-15T13:26:17Z

Closing in favor of #29167, thanks for the PR.

pamil requested a review from sroze as a code owner October 3, 2018 19:45

carsonbot added Status: Needs Review Messenger Feature labels Oct 3, 2018

pamil force-pushed the messenger-command-bus-middleware branch from 7088a74 to d0a42d2 Compare October 3, 2018 19:47

[Messenger] Add middleware allowing just for a single handler

144ae61

pamil force-pushed the messenger-command-bus-middleware branch from d0a42d2 to 144ae61 Compare October 3, 2018 19:48

nicolas-grekas reviewed Oct 4, 2018

View reviewed changes

sroze reviewed Oct 7, 2018

View reviewed changes

nicolas-grekas added this to the next milestone Oct 14, 2018

Tobion mentioned this pull request Nov 15, 2018

[Messenger] Add a trait for synchronous query & command buses #29167

Merged

nicolas-grekas closed this Nov 15, 2018

stof removed this from the next milestone Nov 23, 2018

pamil deleted the messenger-command-bus-middleware branch May 25, 2020 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Messenger] Add middleware allowing just for a single handler #28716

[Messenger] Add middleware allowing just for a single handler #28716

pamil commented Oct 3, 2018 •

edited

Loading

nicolas-grekas Oct 4, 2018

sroze Oct 6, 2018

pamil Oct 6, 2018 •

edited

Loading

sroze Oct 7, 2018

ogizanagi commented Oct 8, 2018

sroze commented Oct 21, 2018

sroze commented Oct 31, 2018 •

edited by nicolas-grekas

Loading

Tobion commented Nov 1, 2018 •

edited

Loading

ogizanagi commented Nov 1, 2018

nicolas-grekas commented Nov 15, 2018

[Messenger] Add middleware allowing just for a single handler #28716

[Messenger] Add middleware allowing just for a single handler #28716

Conversation

pamil commented Oct 3, 2018 • edited Loading

nicolas-grekas Oct 4, 2018

Choose a reason for hiding this comment

sroze Oct 6, 2018

Choose a reason for hiding this comment

pamil Oct 6, 2018 • edited Loading

Choose a reason for hiding this comment

sroze Oct 7, 2018

Choose a reason for hiding this comment

ogizanagi commented Oct 8, 2018

sroze commented Oct 21, 2018

sroze commented Oct 31, 2018 • edited by nicolas-grekas Loading

Tobion commented Nov 1, 2018 • edited Loading

ogizanagi commented Nov 1, 2018

nicolas-grekas commented Nov 15, 2018

pamil commented Oct 3, 2018 •

edited

Loading

pamil Oct 6, 2018 •

edited

Loading

sroze commented Oct 31, 2018 •

edited by nicolas-grekas

Loading

Tobion commented Nov 1, 2018 •

edited

Loading