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

[Messenger] Add handled & sent stamps #29166

Merged
merged 1 commit into from Nov 15, 2018

Conversation

Projects
None yet
6 participants
@ogizanagi
Member

ogizanagi commented Nov 10, 2018

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/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 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.

@ogizanagi ogizanagi force-pushed the ogizanagi:messenger_handled_sent_stamps branch 4 times, most recently from 047c355 to 19f77cc Nov 10, 2018

@ogizanagi ogizanagi force-pushed the ogizanagi:messenger_handled_sent_stamps branch from 19f77cc to 6cd29ca Nov 11, 2018

@ogizanagi ogizanagi force-pushed the ogizanagi:messenger_handled_sent_stamps branch from 6cd29ca to 8d09d8b Nov 12, 2018

@ogizanagi ogizanagi force-pushed the ogizanagi:messenger_handled_sent_stamps branch 2 times, most recently from 3b4dba6 to 583d772 Nov 12, 2018

@ogizanagi ogizanagi force-pushed the ogizanagi:messenger_handled_sent_stamps branch 3 times, most recently from 56c3e00 to 6105ad4 Nov 12, 2018

@ro0NL

ro0NL approved these changes Nov 13, 2018

@ogizanagi ogizanagi referenced this pull request Nov 13, 2018

Merged

[Messenger] Fix typos #29210

nicolas-grekas added a commit that referenced this pull request Nov 13, 2018

minor #29210 [Messenger] Fix typos (ogizanagi)
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     <!-- 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        | N/A

Just small typos extracted from #29166 in case it doesn't make it to 4.2.

Commits
-------

7e763f9 [Messenger] Fix typos

@ogizanagi ogizanagi force-pushed the ogizanagi:messenger_handled_sent_stamps branch from 6105ad4 to fe9b17a Nov 14, 2018

@ogizanagi ogizanagi force-pushed the ogizanagi:messenger_handled_sent_stamps branch from fe9b17a to f41c161 Nov 14, 2018

@nicolas-grekas

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

@chalasr

👍 for 4.2

@ogizanagi ogizanagi force-pushed the ogizanagi:messenger_handled_sent_stamps branch from f41c161 to 1ee99f8 Nov 14, 2018

@Tobion

Tobion approved these changes Nov 15, 2018

@Tobion

This comment has been minimized.

Member

Tobion commented Nov 15, 2018

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.

@ogizanagi ogizanagi force-pushed the ogizanagi:messenger_handled_sent_stamps branch from 1ee99f8 to 7990c39 Nov 15, 2018

@ogizanagi ogizanagi force-pushed the ogizanagi:messenger_handled_sent_stamps branch 2 times, most recently from f096214 to f034c5a Nov 15, 2018

@ro0NL

ro0NL approved these changes Nov 15, 2018

@ogizanagi ogizanagi force-pushed the ogizanagi:messenger_handled_sent_stamps branch from f034c5a to 2f5acf7 Nov 15, 2018

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Nov 15, 2018

Thank you @ogizanagi.

@nicolas-grekas nicolas-grekas merged commit 2f5acf7 into symfony:master Nov 15, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Nov 15, 2018

feature #29166 [Messenger] Add handled & sent stamps (ogizanagi)
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

@ogizanagi ogizanagi deleted the ogizanagi:messenger_handled_sent_stamps branch Nov 15, 2018

@fabpot fabpot referenced this pull request Nov 16, 2018

Merged

Release v4.2.0-BETA2 #29237

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