-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
👍 shipit :) |
f81ead5
to
53cecc4
Compare
Does |
Yes: it's used when several handlers are configured for a message, using the |
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
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. |
This change would exclude using the messenger component as query bus and command pattern bus. |
@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. |
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.) |
Wouldn't this fail because any message may at some point be serialized? |
@fbourigault if you expect any results back from a query bus, it means the bus is synchronous. Only asynchronous messages are serialized. |
This doesn't work if you are in a controller and need the result to return a response which is a common use case. |
@jvasseur sure, but as you wrote yourself, you know how to do otherwise. |
One thing that we could return is the passed object if that makes sense to others: |
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. |
src/Symfony/Component/Messenger/DataCollector/MessengerDataCollector.php
Show resolved
Hide resolved
@fbourigault it's impossible to switch from sync to async if you expect any results back. |
@makasim how can you do that in PHP? I'm not aware of any way to do that without using non-standard extensions... |
@makasim I don't understand what you're talking about in the context of this PR. |
@nicolas-grekas : The component allows to handle both sync & async a single command with |
@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. |
But would just prevent naively using |
At some point, people are responsible for the way they configure their buses. |
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
to
d81d907
Compare
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 :) ) |
closes #28758 |
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. It does increase the lines/characters of code slightly. But you also get proper return type / auto-completion - e.g. 👍 Cheers! |
d81d907
to
f942ffc
Compare
While discussing this on Slack with @weaverryan, we figured out that it's very easy to wrap the private function query(MyQueryInterface $query): MyReturnType
{
$this->bus->dispatch($query);
return $query->getResult();
} and then use |
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 this mixed
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(/* ... */)
).
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
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.