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 handled & sent stamps#29166

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:messenger_handled_sent_stamps
Nov 15, 2018
Merged

[Messenger] Add handled & sent stamps#29166

nicolas-grekas merged 1 commit intosymfony:masterfromogizanagi:messenger_handled_sent_stamps
Nov 15, 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/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 theconsole text descriptor format for now.
  • Sender areSenderInterface 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 fromHandledStamp::fromCallable()


From previous conversations:

What about not adding HandledStamp onnull 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.

@ogizanagiogizanagi mentioned this pull requestNov 13, 2018
nicolas-grekas added a commit that referenced this pull requestNov 13, 2018
This PR was merged into the 4.2-dev branch.Discussion----------[Messenger] Fix typos| Q             | A| ------------- | ---| Branch?       | 4.2 <!-- see below -->| Bug fix?      | no| New feature?  | no <!-- 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        | N/AJust small typos extracted from#29166 in case it doesn't make it to 4.2.Commits-------7e763f9 [Messenger] Fix typos
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.

(OK for 4.2 on my side to release asap the new semantics of keys for handlers/senders locators)

Copy link
Member

@chalasrchalasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👍 for 4.2

@Tobion
Copy link
Contributor

Even if we don't add a query bus#29167 (comment), adding tracking for handled/sent should not hurt to add to the core and allows people to add any custom logic on top of it.

@nicolas-grekas
Copy link
Member

Thank you@ogizanagi.

@nicolas-grekasnicolas-grekas merged commit2f5acf7 intosymfony:masterNov 15, 2018
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 deleted the messenger_handled_sent_stamps branchNovember 15, 2018 12:17
@fabpotfabpot mentioned this pull requestNov 16, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@chalasrchalasrchalasr approved these changes

@srozesrozeAwaiting requested review from sroze

+2 more reviewers

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

6 participants

@ogizanagi@Tobion@nicolas-grekas@ro0NL@chalasr@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp