Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] Make MessengerPass less strict when auto-register handlers#29525
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
[Messenger] Make MessengerPass less strict when auto-register handlers#29525
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ogizanagi commentedDec 8, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 commentedDec 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 commentedDec 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 commentedDec 9, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 commentedDec 13, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I agree with@ro0NL: handlers are designed to get only the message and not the envelope. |
nicholasruunu commentedDec 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. |
| $parameters =$method->getParameters(); | ||
| if (1 !==\count($parameters)) { | ||
| thrownewRuntimeException(sprintf('Invalid handler service "%s": method "%s::__invoke()"must have exactly oneargumentcorresponding to the message it handles.',$serviceId,$handlerClass->getName())); | ||
| if (0 ===\count($parameters)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
maybe use getNumberOfRequiredParameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I changed it, although not sure if it helps against(Message $message = null) since you could have(Message $message = null, Envelope $envelope)?
nicholasruunu commentedJan 10, 2019
I updated the PR to only deal with auto-registering in MessengerPass. |
xabbuh commentedJan 16, 2019
the changes seem reasonable to me |
src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
303051a to49ab2cdCompare
sroze left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sounds reasonable to me as well 👍
sroze commentedApr 6, 2019
Thank you@nicholasruunu. |
…ister handlers (nicholasruunu)This PR was merged into the 4.3-dev branch.Discussion----------[Messenger] Make MessengerPass less strict when auto-register handlers| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| License | MIT| Doc PR | not neededThis allows you to auto-register handlers that have more than one argument, which is useful when having custom middleware to pass more parameters.#symfonyconhackday2018Commits-------49ab2cd Make MessengerPass less strict when auto-register handlers
Uh oh!
There was an error while loading.Please reload this page.
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