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] 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 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
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Tobion commentedNov 15, 2018
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 commentedNov 15, 2018
This PR replaces#28716 I assume |
nicolas-grekas 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.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
ogizanagi commentedNov 15, 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.
@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: privatefunctionquery(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. |
nicolas-grekas commentedNov 15, 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.
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 <!-- seehttps://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/10661Based on#29159This 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 handlerIMHO, `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
ogizanagi commentedNov 15, 2018
Trait it is... (Agree for the interface, but I would have provided the base class anyway. Expect to see this happen in userland.) |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| /** @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 ifnull is returned by the handler,$handledStamps[0] exists as aHandledStamp instance.
So better revert toisset?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas 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.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Tobion commentedNov 15, 2018
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. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
ro0NL commentedNov 16, 2018
That does not a apply to a event- and/or command-bus. Only a query bus, |
nicolas-grekas commentedNov 16, 2018
nicolas-grekas commentedNov 16, 2018
Or maybe something more neutral like |
ro0NL commentedNov 16, 2018
I think there 2 concepts to grasp;
The first does not enforce the latter, however both apply to a query bus at least. |
ogizanagi commentedNov 16, 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.
@ro0NL : We do not enforce a result value. We enforce there is a handler. Which applies to bothsynchronous 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 |
ro0NL commentedNov 16, 2018
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. |
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.
Definitely 👍. Looks great to me.
sroze commentedNov 18, 2018
The name If we really want to get rid of the "query bus" notion, then we should focus on the expected behaviour: returning results: |
nicolas-grekas commentedNov 18, 2018
Naming things :)
|
nicolas-grekas commentedNov 20, 2018
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 <!-- seehttps://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/10662Commits-------6ba4e8a [Messenger] Add a trait for synchronous query & command buses
gquemener commentedMay 25, 2020
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. |
gquemener commentedMay 25, 2020
The idea I have in mind is to add a |
gquemener commentedMay 25, 2020
Another solution would be to configure the middleware with another (new) implementation of |
gquemener commentedMay 25, 2020
Another option would be to do it at compilation time in the MessengerPass 🤔 |
chalasr commentedMay 25, 2020
@gquemener Please consider opening a new issue to propose your idea and discuss about it. |
gquemener commentedMay 25, 2020
@chalasr here it is. Thanks for your reply! |
Uh oh!
There was an error while loading.Please reload this page.