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] made dispatch() and handle() return void#28909
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
SpacePossum commentedOct 17, 2018
👍 shipit :) |
f81ead5 to53cecc4CompareKoc commentedOct 17, 2018
Does |
nicolas-grekas commentedOct 17, 2018
Yes: it's used when several handlers are configured for a message, using the |
fabpot 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.
Looks very sensible to me
jvasseur commentedOct 17, 2018
I'm 👎 here, the messenger component can be used for things like query buses where being able to return a result makes sense. Setting the result on the query would force to transform a code like this into which would makes it really annoying to use. |
fbourigault commentedOct 17, 2018
This change would exclude using the messenger component as query bus and command pattern bus. |
Koc commentedOct 18, 2018
@nicolas-grekas can you please open also PR into docs repository and describe how to properly configure query bus? I hope it removes all the questions.
Would be nice have some example. |
nicolas-grekas commentedOct 18, 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.
Basically these two alternatives, which both allow specifying the expected type: Or better: It may not be as simple to write as DX will be improved by allowing both readers and the IDE to know what they can expect (and suggest for autocompletion.) |
fbourigault commentedOct 18, 2018
Wouldn't this fail because any message may at some point be serialized? |
nicolas-grekas commentedOct 18, 2018
@fbourigault if you expect any results back from a query bus, it means the bus is synchronous. Only asynchronous messages are serialized. |
jvasseur commentedOct 18, 2018
This doesn't work if you are in a controller and need the result to return a response which is a common use case. |
nicolas-grekas commentedOct 18, 2018
@jvasseur sure, but as you wrote yourself, you know how to do otherwise. |
nicolas-grekas commentedOct 18, 2018
One thing that we could return is the passed object if that makes sense to others: |
fbourigault commentedOct 18, 2018
I know but it's so easy to switch from sync to async that things would break so easily which is not so good for DX. Anyway, you convinced me and I prefer having a robust component that can't return than having one that may return but is more fragile. Maybe in the future, we will be able to ship a robust bus which can return a result but mixing both non-returning and returning buses in the same interface is too dangerous. |
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas commentedOct 18, 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.
@fbourigault it's impossible to switch from sync to async if you expect any results back. |
nicolas-grekas commentedOct 18, 2018
@makasim how can you do that in PHP? I'm not aware of any way to do that without using non-standard extensions... |
nicolas-grekas commentedOct 18, 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.
@makasim I don't understand what you're talking about in the context of this PR. |
ogizanagi commentedOct 18, 2018
@nicolas-grekas : The component allows to handle both sync & async a single command with |
nicolas-grekas commentedOct 18, 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.
@ogizanagi sure, and this PR doesn't prevent this use case. It just enforces doing it in a way that is friendly to the type system and to software design. That's our responsibility as framework authors. |
ogizanagi commentedOct 18, 2018
But would just prevent naively using |
nicolas-grekas commentedOct 18, 2018
At some point, people are responsible for the way they configure their buses. |
nicolas-grekas commentedOct 18, 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.
Side note: I think EventDispatcher is equally fine for creating command/query buses. The central innovations that Messenger adds to the Symfony stack are buses with transports. |
53cecc4 tod81d907Comparenicolas-grekas commentedOct 18, 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.
Personally, I would even go one step further and require messages to be cloneable - and clone them before dispatching to middlewares... (this could be implemented as a middleware :) ) |
Koc commentedOct 18, 2018
closes#28758 |
weaverryan commentedOct 19, 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.
This makes a lot of sense to me. Nicolas listed may reasons (#28909 (comment)) why the return value is problematic in general for this component. SimpleBus's implementation returns void for these exact reasons (https://github.com/SimpleBus/MessageBus/issues/52). Should we support make sure QueryBus implementations are possible? Of course! And there are some examples in this issue of how we can continue to do it. Itdoes increase the lines/characters of code slightly. But you also get proper return type / auto-completion - e.g. 👍 Cheers! |
d81d907 tof942ffcComparenicolas-grekas commentedOct 20, 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.
While discussing this on Slack with@weaverryan, we figured out that it's very easy to wrap the privatefunctionquery(MyQueryInterface$query):MyReturnType{$this->bus->dispatch($query);return$query->getResult();} and then use |
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 good to me.
For the records, I've been back and forth on this one. I finally decided to admit that thismixed return value is more of a workaround than something else. It does not give any typing capabilities which is very much error prone. As mentioned already in the pull-request, a query bus implementation is reasonably easy to achieve with a "pass-through" object (i.e.$bus->dispatch($query = new MyQuery()) +$query->setResult(/* ... */)).
sroze commentedOct 21, 2018
Thank you@nicolas-grekas. |
…nicolas-grekas)This PR was merged into the 4.2-dev branch.Discussion----------[Messenger] made dispatch() and handle() return void| Q | A| ------------- | ---| Branch? | 4.2| Bug fix? | no| New feature? | no| BC breaks? | yes| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Middlewares and dispatchers should not return any value. Forwarding back the results from handlers breaks the scope of the component. The canonical example of some bad design this can lead to is `ChainHandler`: how can the caller know that there is one in the chain and that it should expect an array as a return value? More generally, how can a caller know what to expect back from a call to dispatch()? I think we should not allow such broken designs.Instead, we should favor east-oriented design: if one needs a command bus, one would have to dispatch a proper command object - and if a result is expected back, it should be done via a setter on the dispatched command - or better, a callback set on the command object. This way we play *by the rules* of the type-system, not against.Commits-------f942ffc [Messenger] made dispatch() and handle() return void
…leware handlers (nicolas-grekas)This PR was merged into the 4.2-dev branch.Discussion----------[Messenger] make Envelope first class citizen for middleware handlers| Q | A| ------------- | ---| Branch? | 4.2| Bug fix? | no| New feature? | no| BC breaks? | yes| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -This PR sits on top of#28909 so that only the 2nd commit should be reviewed for now, until I rebase it.This idea has already been proposed and rejected in#27322.Yet, now that I've reviewed in depth the component, I politely but strongly suggest to reconsider.Middleware handlers are the central piece of the routing layer.When `dispatch()` accepts `object|Envelope`, it's because it sits on the outer boundary of the component. `handle()` on the contrary plugs inside its core routing logic, so that it's very legitimate to require them to deal with `Envelope` objects. Yes, some middleware care only about the message inside. That's fine calling `getMessage()` for them.Right now, we built a complex magic layer that acts as a band-aid *just* to save this call to `getMessage()`. For middleware that want the envelope, we require an additional interface that magically *changes* the expected argument. That's very fragile design: it breaks the `L` in `SOLID`...Looking at the diff stat, this is most natural.Commits-------ae46a43 [Messenger] make Envelope first class citizen for middleware handlers
…ds return Envelope (nicolas-grekas)This PR was merged into the 4.2-dev branch.Discussion----------[Messenger] make dispatch(), handle() and send() methods return Envelope| Q | A| ------------- | ---| Branch? | 4.2| Bug fix? | no| New feature? | yes| BC breaks? | no (already broken ;) )| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Follow up of#28909. We can do better than return `void`: let's return `Envelope`!This means middleware and senders should also return `Envelope`, so that we can typically read back the stamps that were added during the sending process (eg to get the Kafka queue id).ping@dunglas as we discussed that first on Slack, and@sroze as we confirmed interest IRL today.(User handlers don't know anything about envelopes so they still should return `void` - use senders or middleware if you need to populate/read envelopes.)Commits-------4b0e015 [Messenger] make dispatch(), handle() and send() methods return Envelope
Uh oh!
There was an error while loading.Please reload this page.
Middlewares and dispatchers should not return any value. Forwarding back the results from handlers breaks the scope of the component. The canonical example of some bad design this can lead to is
ChainHandler: how can the caller know that there is one in the chain and that it should expect an array as a return value? More generally, how can a caller know what to expect back from a call to dispatch()? I think we should not allow such broken designs.Instead, we should favor east-oriented design: if one needs a command bus, one would have to dispatch a proper command object - and if a result is expected back, it should be done via a setter on the dispatched command - or better, a callback set on the command object. This way we playby the rules of the type-system, not against.