[Messenger] Add a trait for synchronous query & command buses

[ogizanagi]

Q	A
Branch?	4.2
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	N/A
License	MIT
Doc PR	symfony/symfony-docs/issues/10662

[Tobion]

While the implementation looks clean and simple, I'm not confident that a query bus should be added at all. The problem that @nicolas-grekas already mentioned in #28909 (comment) still applies. This does not allow to use type declarations for the return value. And there are better alternatives already.

Also I would argue that queries should not be using the command bus pattern at all. Use the repository pattern for this. This is also why SimpleBus does not include a query bus.

[Tobion]

This PR replaces #28716 I assume

[nicolas-grekas]

What about shipping a trait instead of an interface + class? We would then encourage ppl to declare return types when using it. The method added to the trait would be private.

[ogizanagi]

@nicolas-grekas 's comment still applies, yes....about autocompletion. While I would usually agree with you, here the return type wouldn't even be of any help in most cases, because the result is often simply sent to the template or a serializer.

This does not allow to use type declarations for the return value

Return type is enforced by the handler. Which we ensure is registered, and single. So a mismatch in config/code may still happen, but at some point users are responsible of their config and the issue would be easily spotted.

Also I would argue that queries should not be using the command bus pattern at all. Use the repository pattern for this.

The query bus isn't a replacement of the repository pattern but is actually complementary to it.
It also allows:

• cross-cutting concerns through middleware (logging, cache, storing metrics, query validation, security, ...)
• centralizing application use-cases in one place, in a consistent way between reads & writes. It's awesome for discoverability, really helpful for anyone new in the team to understand the application.

Query handlers are also the place where data is aggregated into DTOs matching the use-case.
All of this favor best practices & designing sustainable, understandable, enjoyable apps.

If one really needs the return type, this is perfectly legit to implement in their action or any caller:

private function query(MyQuery $query): MyQueryResult
{
    return $this->queryBus->query($query);
}

And if we want to enforce return type to prevent any misconfig issue, we could add an optional fqcn argument to assert the result is what we expect.

What about shipping a trait instead of an interface + class? We would then encourage ppl to declare return types when using it. The method added to the trait would be private.

This is not needed to me regarding above explanations.

[nicolas-grekas]

What is not needed is the interface and the class ;)
Let's save us creating an abstraction (the interface) we can avoid, especially when it's not that SOLID.

[ogizanagi]

Trait it is...

(Agree for the interface, but I would have provided the base class anyway. Expect to see this happen in userland.)

[nicolas-grekas]

should use "count" to account for nulls

[ogizanagi]

Hmm. Actually not. Even if null is returned by the handler, $handledStamps[0] exists as a HandledStamp instance.
So better revert to isset?

[nicolas-grekas]

I would have provided the base class anyway

a class is still a type, which is what is costly in terms of maintenance / SOLID. A trait is a pure behavior so mostly free from such considerations.

[Tobion]

I agree the interface is not needed. But the query bus as a class is much cleaner than as a trait. Traits are for code reuse in single-inheritance languages. A class like a controller that requires a query bus, depends on it and it does not behave like one.

[ro0NL]

a message bus instance to return a single synchronous message handling result

That does not a apply to a event- and/or command-bus. Only a query bus,

[nicolas-grekas]

In #28716, @pamil uses the term command bus for his need. What's a command bus vs a query bus?

[nicolas-grekas]

Or maybe something more neutral like OneHandlerTrait?

[ro0NL]

I think there 2 concepts to grasp;

• enforce a single handler
• enforce a result value

The first does not enforce the latter, however both apply to a query bus at least.

[ogizanagi]

@ro0NL : We do not enforce a result value. We enforce there is a handler. Which applies to both synchronous command & query buses. Using this trait for a command bus would be fine. Either you choose to use a return value or not, then, is up to you.

Note: the CommandBusTrait name suggestion is more about the generic term referring to the command pattern. Not command/query bus. Would SyncCommandBusTrait be any better?

[ro0NL]

We do not enforce a result value

True, my bad.

suggestion is more about the generic term referring to the command pattern. Not command/query bus

i find that confusing :)

I think something more generic could apply yes, e.g. i have something called MessageDispatchingTrait: https://github.com/msgphp/msgphp/blob/master/src/Domain/Message/MessageDispatchingTrait.php#L12

It could provide multiple APIs, e.g. dispatch() + dispatchOne()

[sroze]

Definitely 👍. Looks great to me.

[sroze]

The name QueryBusTrait seems reasonable to me as the query bus is typically what describes a bus returning a result, so it sounds good.

If we really want to get rid of the "query bus" notion, then we should focus on the expected behaviour: returning results: DispatchAndGetResultTrait?

[nicolas-grekas]

Naming things :)
My best proposal would be using just HandleTrait + handle():

• use dispatch() to get an envelop back and have things potentially async
• use handle() from the trait to get synchronous behavior and a result back

[nicolas-grekas]

Thank you @ogizanagi.

[gquemener]

Sorry to dig up the issue so long after the discussion, but I feel that a single handler is a common strategy when using a command bus.

I've been using prooph/service-bus (currently deprecated) for the past years and that's something they provide (https://github.com/prooph/service-bus/blob/master/src/Plugin/Router/SingleHandlerRouter.php).

I'm surprised that the only way the Messenger component offers this feature is through a trait that allows to check that the message was not handled by more than one handler. It feels that this is too late and the damages are already done.

I'd like to work on a PR that tweaks the HandleMessageMiddleware to detect this problem before any handler is called (keeping it BC at the same time).

I've read the discussions and couldn't find any moment where this option was discussed. Please forgive me if it is the case.

[gquemener]

The idea I have in mind is to add a allowMultipleHandlers flag to the middleware, and check how many handlers are located for a given message whenever it's enabled.

[gquemener]

Another solution would be to configure the middleware with another (new) implementation of HandlersLocatorInterface that throws exception when more than 1 handler is located.

[gquemener]

Another option would be to do it at compilation time in the MessengerPass 🤔

[chalasr]

@gquemener Please consider opening a new issue to propose your idea and discuss about it.

[gquemener]

@chalasr here it is. Thanks for your reply!