NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

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

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

Jump to bottom

Closed

pamil wants to merge1 commit intosymfony:masterfrompamil:messenger-command-bus-middleware

Closed

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

pamil wants to merge1 commit intosymfony:masterfrompamil:messenger-command-bus-middleware

Conversation

Copy link

Contributor

pamil commentedOct 3, 2018•
edited
Loading

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?

pamil requested a review fromsroze as acode owner

October 3, 2018 19:45

carsonbot added Status: Needs Review Messenger Feature labels

Oct 3, 2018

pamil force-pushed themessenger-command-bus-middleware branch from7088a74 tod0a42d2Compare

October 3, 2018 19:47

[Messenger] Add middleware allowing just for a single handler

144ae61

pamil force-pushed themessenger-command-bus-middleware branch fromd0a42d2 to144ae61Compare

October 3, 2018 19:48

nicolas-grekas reviewed

Oct 4, 2018

View reviewed changes

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

		{
		$handler = $this->messageHandlerResolver->resolve($message);

		if ($handler instanceof ChainHandler) {

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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?

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, this is coupled with theFrameworkBundle 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? 🤔

Copy link

ContributorAuthor

pamilOct 6, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 intoFrameworkBundle 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 reviewed

Oct 7, 2018

View reviewed changes

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

Copy link

Contributor

srozeOct 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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?

Copy link

Contributor

ogizanagi commentedOct 8, 2018

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 ?

nicolas-grekas added this to thenext milestone

Oct 14, 2018

Copy link

Contributor

sroze commentedOct 21, 2018

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 theMessengerPass to enforce it when registering messages. I'm 👍-ing it!

Copy link

Contributor

sroze commentedOct 31, 2018•
edited by nicolas-grekas
Loading

Note that nowChainHandler has been removed :) (see#29010)

Copy link

Contributor

Tobion commentedNov 1, 2018•
edited
Loading

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 theMessengerPass 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 in9cd88b0

publicfunction__construct(HandlersLocatorInterface$handlersLocator,bool$allowNoHandlers =false)

Copy link

Contributor

ogizanagi commentedNov 1, 2018

@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 onMessengerPass::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).