Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
nicolas-grekas merged 1 commit intosymfony:masterfromogizanagi:query_bus
Nov 20, 2018
Merged

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

nicolas-grekas merged 1 commit intosymfony:masterfromogizanagi:query_bus
Nov 20, 2018

Conversation

@ogizanagi
Copy link
Contributor

@ogizanagiogizanagi commentedNov 10, 2018
edited
Loading

QA
Branch?4.2
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsN/A
LicenseMIT
Doc PRsymfony/symfony-docs/issues/10662

@Tobion
Copy link
Contributor

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
Copy link
Contributor

This PR replaces#28716 I assume

Copy link
Member

@nicolas-grekasnicolas-grekas left a 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.

@ogizanagi
Copy link
ContributorAuthor

ogizanagi commentedNov 15, 2018
edited
Loading

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

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.

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.

jvasseur, sstok, and gquemener reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedNov 15, 2018
edited
Loading

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.

ro0NL reacted with thumbs up emoji

nicolas-grekas added a commit that referenced this pull requestNov 15, 2018
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
@ogizanagiogizanagi changed the title[Messenger] Add a query bus[Messenger] Add a QueryBusTraitNov 15, 2018
@ogizanagi
Copy link
ContributorAuthor

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])) {

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

Copy link
ContributorAuthor

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?

Copy link
Member

@nicolas-grekasnicolas-grekas left a 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.

@Tobion
Copy link
Contributor

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.

yceruto and sstok reacted with thumbs up emoji

@ro0NL
Copy link
Contributor

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,

jvasseur reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

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

@nicolas-grekas
Copy link
Member

Or maybe something more neutral likeOneHandlerTrait?

@ro0NL
Copy link
Contributor

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.

jvasseur and sstok reacted with thumbs up emoji

@ogizanagi
Copy link
ContributorAuthor

ogizanagi commentedNov 16, 2018
edited
Loading

@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: theCommandBusTrait name suggestion is more about the generic term referring to the command pattern. Not command/query bus. WouldSyncCommandBusTrait be any better?

@ro0NL
Copy link
Contributor

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 calledMessageDispatchingTrait:https://github.com/msgphp/msgphp/blob/master/src/Domain/Message/MessageDispatchingTrait.php#L12

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

Copy link
Contributor

@srozesroze left a 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
Copy link
Contributor

The nameQueryBusTrait 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?

jvasseur reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

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

  • usedispatch() to get an envelop back and have things potentially async
  • usehandle() from the trait to get synchronous behavior and a result back
ogizanagi and sstok reacted with thumbs up emojiKoc reacted with heart emoji

@ogizanagiogizanagi changed the title[Messenger] Add a QueryBusTrait[Messenger] Add a trait for synchronous query & command busesNov 19, 2018
@nicolas-grekas
Copy link
Member

Thank you@ogizanagi.

@nicolas-grekasnicolas-grekas merged commit6ba4e8a intosymfony:masterNov 20, 2018
nicolas-grekas added a commit that referenced this pull requestNov 20, 2018
…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
@ogizanagiogizanagi deleted the query_bus branchNovember 20, 2018 18:25
@stofstof modified the milestones:next,4.2Nov 23, 2018
This was referencedNov 26, 2018
@gquemener
Copy link
Contributor

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 usingprooph/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 theHandleMessageMiddleware 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
Copy link
Contributor

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

@gquemener
Copy link
Contributor

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

@gquemener
Copy link
Contributor

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

@chalasr
Copy link
Member

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

@gquemener
Copy link
Contributor

@chalasr here it is. Thanks for your reply!

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@chalasrchalasrchalasr left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+2 more reviewers

@srozesrozesroze approved these changes

@ro0NLro0NLro0NL approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

9 participants

@ogizanagi@Tobion@nicolas-grekas@ro0NL@sroze@gquemener@chalasr@stof@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp