-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Messenger] Add a trait for synchronous query & command buses #29167
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
ogizanagi
commented
Nov 10, 2018
•
edited
Loading
edited
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 |
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. |
This PR replaces #28716 I assume |
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.
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.
@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.
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.
The query bus isn't a replacement of the repository pattern but is actually complementary to it.
Query handlers are also the place where data is aggregated into DTOs matching the use-case. 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.
This is not needed to me regarding above explanations. |
What is not needed is the interface and the class ;) |
This PR was merged into the 4.2-dev branch. Discussion ---------- [Messenger] Add handled & sent stamps | Q | A | ------------- | --- | Branch? | 4.2 <!-- see below --> | Bug fix? | no | New feature? | yes <!-- don't forget to update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | N/A <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | symfony/symfony-docs/issues/10661 Based on #29159 This new feature marks sent and handled messages, so middleware can act upon these and use the handler(s) result(s). This is also the base of a next PR (#29167), introducing a query bus built on top of the message bus. I'm not sure yet about the best way to determine the handlers and senders names/descriptions to store in the stamps: - Handlers are callable. I've just reused the [console text descriptor](https://github.com/nicolas-grekas/symfony/blob/1c1818b87675d077808dbf7e05da84c2e1ddc9f8/src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/TextDescriptor.php#L457-L491) format for now. - ~~Sender are `SenderInterface` instances. `\get_class` is used for now, but a single message can be sent by multiple senders, including of the same class.~~ => Updated. Yielding the sender name if provided, the FQCN otherwise. ~~Instead, what about allowing to yield names from locators, and fallback on the above strategies otherwise? So we'll use transport names from the config for senders, and pre-computed compile-time handlers descriptions?~~ => Done. For handlers, computing it at compile time might not be straightforward. Let's compute it lazily from `HandledStamp::fromCallable()` --- ### From previous conversations: > What about not adding HandledStamp on `null` returned from handler IMHO, `null` still is a result. The stamps allows to identify a message as being handled regardless of the returned value, so makes sense on its own and keeping would require one less check for those wanting to consume it. > What about adding SentStamp? Makes sense to me and I think it was requested by @Nyholm before on Slack. So, included in this PR. > Should it target 4.2 or 4.3? Targeting 4.2, because of the removal of the handler result forwarding by middleware. A userland middleware could have used this result, typically a cache middleware. Which would now require extra boring code in userland. This will simplify it and allow users to create their query bus instance until 4.3. Commits ------- 2f5acf7 [Messenger] Add handled & sent stamps
Trait it is... (Agree for the interface, but I would have provided the base class anyway. Expect to see this happen in userland.) |
/** @var HandledStamp[] $handledStamps */ | ||
$handledStamps = $envelope->all(HandledStamp::class); | ||
|
||
if (!isset($handledStamps[0])) { |
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.
should use "count" to account for nulls
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.
Hmm. Actually not. Even if null
is returned by the handler, $handledStamps[0]
exists as a HandledStamp
instance.
So better revert to isset
?
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 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.
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. |
That does not a apply to a event- and/or command-bus. Only a query bus, |
Or maybe something more neutral like |
I think there 2 concepts to grasp;
The first does not enforce the latter, however both apply to a query bus at least. |
@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 |
True, my bad.
i find that confusing :) I think something more generic could apply yes, e.g. i have something called It could provide multiple APIs, e.g. |
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.
Definitely 👍. Looks great to me.
The name If we really want to get rid of the "query bus" notion, then we should focus on the expected behaviour: returning results: |
Naming things :)
|
Thank you @ogizanagi. |
…d buses (ogizanagi) This PR was merged into the 4.2-dev branch. Discussion ---------- [Messenger] Add a trait for synchronous query & command buses | Q | A | ------------- | --- | Branch? | 4.2 <!-- see below --> | Bug fix? | no | New feature? | yes <!-- don't forget to update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | N/A <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | symfony/symfony-docs/issues/10662 Commits ------- 6ba4e8a [Messenger] Add a trait for synchronous query & command buses
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 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 I've read the discussions and couldn't find any moment where this option was discussed. Please forgive me if it is the case. |
The idea I have in mind is to add a |
Another solution would be to configure the middleware with another (new) implementation of |
Another option would be to do it at compilation time in the MessengerPass 🤔 |
@gquemener Please consider opening a new issue to propose your idea and discuss about it. |
@chalasr here it is. Thanks for your reply! |