[Messenger] made dispatch() and handle() return void

[nicolas-grekas]

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.

[SpacePossum]

👍 shipit :)

[Koc]

Does ChainHandler used somewhere internally? What use cases for it?

[nicolas-grekas]

Yes: it's used when several handlers are configured for a message, using the messenger.message_handler tag.

[fabpot]

Looks very sensible to me

[jvasseur]

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

$result = $bus->dispatch(new Query());

into

$query = new Query();
$bus->dispatch($query);
$result = $query->getResult();

which would makes it really annoying to use.

[fbourigault]

This change would exclude using the messenger component as query bus and command pattern bus.

[Koc]

@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.

or better, a callback set on the command object

Would be nice have some example.

[nicolas-grekas]

Basically these two alternatives, which both allow specifying the expected type:
This first one already written above:

$query = new Query();
$bus->dispatch($query);
$result = $query->getResult();

Or better:

$bus->dispatch(new Query([$this, 'onResult']));

It may not be as simple to write as $result = $bus->dispatch(new Query());, but it allows enforcing the expected return type, which is just basic requirements for any maintainable software design. People cannot want typed properties and reject the type system at the same time.
We spent years killing public services because they rely on hard coupling with the configuration and hide deps. Here it's exactly the same drawbacks, except it is for return types instead of arguments.

DX will be improved by allowing both readers and the IDE to know what they can expect (and suggest for autocompletion.)

[fbourigault]

$bus->dispatch(new Query([$this, 'onResult']));

Wouldn't this fail because any message may at some point be serialized?

[nicolas-grekas]

@fbourigault if you expect any results back from a query bus, it means the bus is synchronous. Only asynchronous messages are serialized.

[jvasseur]

$bus->dispatch(new Query([$this, 'onResult']));

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]

@jvasseur sure, but as you wrote yourself, you know how to do otherwise.

[nicolas-grekas]

One thing that we could return is the passed object if that makes sense to others:
$bus->dispatch(new Query())->getResult();
EventDispatcher does something similar.
But since PHP doesn't have generics, the result cannot be autocompleted without extra rules for the IDE (the return type is still locally known, but cannot be expressed in PHP.)

[fbourigault]

@fbourigault if you expect any results back from a query bus, it means the bus is synchronous. Only asynchronous messages are serialized.

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.

[nicolas-grekas]

@fbourigault it's impossible to switch from sync to async if you expect any results back.
That's why I wrote that this return value blurs the line of the component in the description. If you use the result in any way, you're de facto opting out of async in the near future.

[nicolas-grekas]

@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]

@makasim I don't understand what you're talking about in the context of this PR.
My previous statement is the following:
code like $result = $bus->dispatch(new Query()); or $bus->dispatch(new Query())->getResult(); or $bus->dispatch(new Query([$this, 'onResult'])); are fundamentally incompatible with async in PHP.
If you write the code in some other way, of course, async is possible. But with these very pieces of code, getting the result is by definition synchronous (I'm not considering that serializing "$this" is a viable way to make calling "onResult" async).

[ogizanagi]

@nicolas-grekas : The component allows to handle both sync & async a single command with send_and_handle (cf https://symfony.com/doc/current/messenger.html#routing).
However, I never had a use-case for this, so I cannot provide more feedback.

[nicolas-grekas]

@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]

But would just prevent naively using [$this, 'onResult'] as the message will be serialized. At least nobody wants the Messenger Serializer to try serializing this callable. Do we care?

[nicolas-grekas]

At some point, people are responsible for the way they configure their buses.
If one wants a query bus and wires the serializer on it, that's their will.

[nicolas-grekas]

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.

[nicolas-grekas]

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]

closes #28758

[weaverryan]

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. $result = $bus->dispatch(new Query()); does not guarantee you know what $result is and you won't get auto-completion. Now you would, which is safer and a step forward for DX.

👍

Cheers!

[nicolas-grekas]

While discussing this on Slack with @weaverryan, we figured out that it's very easy to wrap the dispatch() call in a helper method:

private function query(MyQueryInterface $query): MyReturnType
{
    $this->bus->dispatch($query);
    return $query->getResult();
}

and then use $result = $this->query(new MyQuery());.
I love this because it makes everything explicit, types wise: userland are encouraged to declare their contracts and get nice autocompletion in return, and maintainers get enough guarantees to not be blocked by external configuration or behavior, allowing to use the BC promise, the deprecation policy and other processes in place to maintain/enhance the component in the future.

[sroze]

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(/* ... */)).

[sroze]

Thank you @nicolas-grekas.