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] Make MessengerPass less strict when auto-register handlers #29525

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@nicholasruunu
Copy link

nicholasruunu commented Dec 8, 2018

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

This allows you to auto-register handlers that have more than one argument, which is useful when having custom middleware to pass more parameters.

#symfonyconhackday2018

@ogizanagi

This comment has been minimized.

Copy link
Member

ogizanagi commented Dec 8, 2018

Well, usually handlers should be pure business/application logic and envelope stamps mostly technical stuff about the transport or cross cutting concerns from middleware. If the data you need from stamps is relevant to the handler, you should think twice if it should not be part of the message itself. If it can't and can only be added as a stamp, is it really the handler's responsibility to be aware of this and act accordingly?

But actually on my side, I'm not against this PR right now. I even thought to suggest this before but was waiting for real demands. What I expressed above only is my own experience with buses within CQRS applications. Not canonical. And bringing more flexibility here might be interesting without implying much overhead.

@nicholasruunu

This comment has been minimized.

Copy link

nicholasruunu commented Dec 8, 2018

@ogizanagi Yeah, I don't know how common this would be but our need comes from using PubNub and having to apply the channel to respond to when sending another message from the handler.

Having the channel be part of the message is a solution but that would really mix infrastructure concerns with our business logic.

@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Dec 8, 2018

But having the envelope be part of the message is also mixing infrastructure (sf messenger) with domain logic 🤔

It sounds like a middleware concern IMHO.

@nicholasruunu

This comment has been minimized.

Copy link

nicholasruunu commented Dec 9, 2018

@ro0NL It's true, if it's possible to have information from one message to another in the middleware then let me know. Also, handlers you can change, messages you really shouldn't change too much.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 13, 2018

I agree with @ro0NL: handlers are designed to get only the message and not the envelope.
If you need metadata related to the transport layer, you should write a middleware instead - or a sender.

@nicholasruunu

This comment has been minimized.

Copy link

nicholasruunu commented Dec 13, 2018

@nicolas-grekas Yeah, we did solve it with a custom middleware, but it's not really a nice experience atm, but doable.

One problem still exist with auto-register handlers with more than one invoke parameters. I think the message-pass should look like what I did in this PR.

@@ -207,8 +207,8 @@ private function guessHandledClasses(\ReflectionClass $handlerClass, string $ser
}
$parameters = $method->getParameters();
if (1 !== \count($parameters)) {
throw new RuntimeException(sprintf('Invalid handler service "%s": method "%s::__invoke()" must have exactly one argument corresponding to the message it handles.', $serviceId, $handlerClass->getName()));
if (0 === \count($parameters)) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 13, 2018

Member

maybe use getNumberOfRequiredParameters?

This comment has been minimized.

@nicholasruunu

nicholasruunu Jan 10, 2019

I changed it, although not sure if it helps against (Message $message = null) since you could have (Message $message = null, Envelope $envelope)?

@nicholasruunu nicholasruunu changed the title [Messenger] Pass envelope as second argument to handlers [Messenger] Make MessangerPass less strict when auto-register handlers Jan 10, 2019

@nicholasruunu nicholasruunu changed the title [Messenger] Make MessangerPass less strict when auto-register handlers [Messenger] Make MessengerPass less strict when auto-register handlers Jan 10, 2019

@nicholasruunu

This comment has been minimized.

Copy link

nicholasruunu commented Jan 10, 2019

I updated the PR to only deal with auto-registering in MessengerPass.

@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Jan 16, 2019

the changes seem reasonable to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment