-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Messenger] collect all stamps added on Envelope as collections #29159
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] collect all stamps added on Envelope as collections #29159
Conversation
41dbc7c
to
81b189b
Compare
if ($serializerStamp = $stamps[SerializerStamp::class] ?? null) { | ||
$context = $serializerStamp->getContext() + $context; | ||
if (isset($stamps[SerializerStamp::class])) { | ||
$context = end($stamps[SerializerStamp::class])->getContext() + $context; |
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.
we can actually support multiple stamps no?
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.
like merging all stamps together? I don't know, so I wouldn't change that in this PR :)
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 think deciding on end() vs reset() vs merging kinda belongs to this PR
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.
about reset() vs end(): the previous behavior returned the latest stamp, so it should be end() - in need of any reason to change it I'd keep the behavior here.
src/Symfony/Component/Messenger/Transport/Serialization/Serializer.php
Outdated
Show resolved
Hide resolved
e75340b
to
3407b75
Compare
Nothing prevents anyone from getting and mutating/cloning a stamp with new data. So that's not blocking actually, but this suggestion is nice for DX. Stamps were thought as an advanced feature for flexibility but them as collections is even more specific. Couldn't stamps be mergeable by essence and a In case of a stamp making sense as a collection, there is no way to clear the collection (appart from re-creating an envelope). Do we care? |
It does a lot to me. The comparison with HTTP headers (or with DI tags btw) is sound and meaningful.
that's an interesting idea. I would do it with a separate interface that would be added to stamps that provide this method (as you said, not all of them would need that behavior). Is it worth doing when the current PR is so simple?
that's legit to me, like a real-life envelope: you cannot remove a stamp. And that's on purpose: this is information about the life of the message that shouldn't be deleted - unless you put the message in a new envelope. That's a metaphor but it makes sense to me. BTW, this hints me I'd prefer the current PR vs the mergeable interface: we really don't want to lose stamps silently IMHO (when replacing) |
3407b75
to
957f9a7
Compare
This breaks the ability to "update" existing stamps by replacing them with a new instance. Ie: $bus->dispatch(new Envelope($msg, new SerializationStamp(array('foo' => 'bar'))));
// Later in a middleware
$context = array('bar' => 'baz');
if ($envelope->get(SerializationStamp::class)) {
$context = array_replace($envelope->get(SerializationStamp::class)->getContext(), $context);
}
$envelope = $envelope->with(new SerializationStamp($context)); Also, imho, this has a lot of potential for WTF. Before the behaviour was, at least, clear: a new stamp of the same type replaces an old one. Now, stamps are added to the envelope but they may or may not be taken into account depending on whether the code that consumes them retrieves all of them or just the last one. So, I'd push this a little further to make it more predictable:
If we compare stamps to HTTP headers, this would be in line with the HTTP behaviour that considers that duplicate headers should get merged:
|
@skalpa thanks for the feedback. I read your comment and I'm really sorry but I don't agree with any of your proposals. I see an envelope and its stamps as the history of the processing of a message. The current behavior of erasing the previous message is critically reducing the usefulness of the Envelope wrapper. About mutability of the stamp, this PR doesn't change anything: getLast allows doing exactly the same as the previous get() (the reason being that it does exactly the same thing: return the last stamp). About the previous stamps, we shouldn't facilitate mutating nor merging them: history shouldn't be rewritten. Looking at #29166 then #29167 this already proves that this behavior is useful - and simple to use. LGTM as is. Thanks again. |
@nicolas-grekas I understood this, but I still have to respectfully disagree. By only seeing the envelope as "the history of the processing of a message", you are ignoring what stamps are used for in most cases at the moment: to pass information from the outside world to the components involved in the processing of the message (ie: While I understand what you're trying to do and consider it positive, if from now on stamps should only be considered as a "the history of the processing of the message", then another, distinct, mean to pass configuration to middleware/senders on a message basis should be found. I would also appreciate if you could tell me how I am supposed to fix the middleware I described above and that this PR is going to break once it gets merged, as I'm unable to find a solution by myself. |
I don't get the issue with the middleware example: just replace |
That's right, sorry I missed that. I'll be able to work around it, so I'll stop bothering you ;-) |
957f9a7
to
2e98859
Compare
} | ||
|
||
/** | ||
* @return StampInterface[] indexed by fqcn | ||
* @return StampInterface[]|StampInterface[][] The stamps for the specified FQCN, or all stamps by their class name |
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.
Would it be better to have 2 different methods instead of two kind of returned value?
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.
not sure personally: when reading, all()
or all(SomeStampp:class)
is very clear to me. Discoverability looks better also (same as EventDispatcher::getListeners()
btw) + types-wise, the interface has no method so autocompletion won't help.
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 think you're both right.
Having one method is fine, but isn't grouping them by FQCN in one case a relic from how we used to manage stamps of the same type before ?
If now stamps should also give us an history of the life of the message, it would feel more natural if all()
(without argument) did just return a StampInterface[]
with all the stamps in the order they were added (ie: not grouped by FQCN anymore).
Thank you @nicolas-grekas. |
…llections (nicolas-grekas) This PR was merged into the 4.2-dev branch. Discussion ---------- [Messenger] collect all stamps added on Envelope as collections | Q | A | ------------- | --- | Branch? | 4.2 | Bug fix? | no | New feature? | yes | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | #29156 | License | MIT | Doc PR | - Late small BC break for Messenger: * `Envelope::all()` takes a new optional `$stampFqcn` argument and returns the stamps for the specified FQCN, or all stamps by their class name * `Envelope::get()` has been renamed `Envelope::last()` This fixes the current behavior where we replace any previous stamp with the same type, which is unexpected to me as it silently loses data and more importantly blocks interesting use cases we're going to need in the near future. Basically, that's the same as HTTP headers being allowed to exist several times: most of them make no sense as collections, but some are really useful as collections. Commits ------- 2e98859 [Messenger] collect all stamps added on Envelope as collections
Good idea indeed 👍 |
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
…traceable middleware (ogizanagi) This PR was merged into the 4.2 branch. Discussion ---------- [Messenger] Flatten collection of stamps collected by the traceable middleware | Q | A | ------------- | --- | Branch? | 4.2 <!-- see below --> | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- please 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 ### before   ### after  Not sure how it qualifies, but I assume this wasn't entirely intentional when introducing the new `Envelope::all()` behavior in #29159. Note it'll affect #32680 as well once merged. Commits ------- 015fca7 [Messenger] Flatten collection of stamps collected by the traceable middleware
Late small BC break for Messenger:
Envelope::all()
takes a new optional$stampFqcn
argument and returns the stamps for the specified FQCN, or all stamps by their class nameEnvelope::get()
has been renamedEnvelope::last()
This fixes the current behavior where we replace any previous stamp with the same type, which is unexpected to me as it silently loses data and more importantly blocks interesting use cases we're going to need in the near future.
Basically, that's the same as HTTP headers being allowed to exist several times: most of them make no sense as collections, but some are really useful as collections.