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

Merged
merged 1 commit into from Nov 12, 2018

Conversation

Projects
None yet
7 participants
@nicolas-grekas
Member

nicolas-grekas commented Nov 9, 2018

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.

@nicolas-grekas nicolas-grekas added this to the 4.2 milestone Nov 9, 2018

@nicolas-grekas nicolas-grekas requested a review from sroze as a code owner Nov 9, 2018

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:messenger-envelop-collection branch from 41dbc7c to 81b189b Nov 9, 2018

if ($serializerStamp = $stamps[SerializerStamp::class] ?? null) {
$context = $serializerStamp->getContext() + $context;
if (isset($stamps[SerializerStamp::class])) {
$context = end($stamps[SerializerStamp::class])->getContext() + $context;

This comment has been minimized.

@ro0NL

ro0NL Nov 9, 2018

Contributor

we can actually support multiple stamps no?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 9, 2018

Member

like merging all stamps together? I don't know, so I wouldn't change that in this PR :)

This comment has been minimized.

@ro0NL

ro0NL Nov 9, 2018

Contributor

i think deciding on end() vs reset() vs merging kinda belongs to this PR

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 9, 2018

Member

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.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:messenger-envelop-collection branch 2 times, most recently from e75340b to 3407b75 Nov 9, 2018

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Nov 10, 2018

This fixes the current behavior where we replace any previous stamp with the same type, which is unexpected to me as it silently looses data and more importantly blocks interesting use cases we're going to need in the near future.

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.
DX is always nice, but does it really make sense to keep & serialize every stamps when most of them makes no sense as collections?

Couldn't stamps be mergeable by essence and a StampInterface::merge(StampInterface $newStamp): StampInterface used by Envelope::with (would either return the new stamp or a new instance with merged data, depending of the nature of the stamp)?

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?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Nov 10, 2018

does it really make sense to keep & serialize every stamps when most of them makes no sense as collections?

It does a lot to me. The comparison with HTTP headers (or with DI tags btw) is sound and meaningful.

StampInterface::merge(StampInterface $newStamp): StampInterface

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?

no way to clear the collection (appart from re-creating an envelope).

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)

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:messenger-envelop-collection branch from 3407b75 to 957f9a7 Nov 10, 2018

@ro0NL

ro0NL approved these changes Nov 10, 2018

@skalpa

This comment has been minimized.

Contributor

skalpa commented Nov 11, 2018

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:

  • Envelope::get() should return an array
  • Envelope::getLast() should be removed
  • When several stamps of the same type are present, the explicitly defined behaviour should be that consumers should attempt to take all of them into account (by attempting to merge them if that makes sense).
  • Instead of adding some kind of MergeableStampInterface::merge(), I'd keep the individual stamps in the envelope and provide a way to merge collections at a later stage, when the stamps need to be consumed. For instance, in the case of the SerializationStamp, add a public static function fromCollection(SerializationStamp ...$stamps): self

If we compare stamps to HTTP headers, this would be in line with the HTTP behaviour that considers that duplicate headers should get merged:

Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Nov 11, 2018

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

@skalpa

This comment has been minimized.

Contributor

skalpa commented Nov 11, 2018

@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: SerializationStamp and ValidationStamp, or in other words 100% of the stamps currently provided for user consumption by the component).

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.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Nov 11, 2018

I don't get the issue with the middleware example: just replace get() by getLast() and done. You shouldn't care that the previous stamp is still there: that's history nobody will care about in this case.

@skalpa

This comment has been minimized.

Contributor

skalpa commented Nov 11, 2018

That's right, sorry I missed that. I'll be able to work around it, so I'll stop bothering you ;-)
Thanks for taking the time to answer me.
👍

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:messenger-envelop-collection branch from 957f9a7 to 2e98859 Nov 12, 2018

}
/**
* @return StampInterface[] indexed by fqcn
* @return StampInterface[]|StampInterface[][] The stamps for the specified FQCN, or all stamps by their class name

This comment has been minimized.

@fabpot

fabpot Nov 12, 2018

Member

Would it be better to have 2 different methods instead of two kind of returned value?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 12, 2018

Member

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.

This comment has been minimized.

@skalpa

skalpa Nov 12, 2018

Contributor

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

@fabpot

fabpot approved these changes Nov 12, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Nov 12, 2018

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 2e98859 into symfony:master Nov 12, 2018

3 checks passed

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

fabpot added a commit that referenced this pull request Nov 12, 2018

feature #29159 [Messenger] collect all stamps added on Envelope as co…
…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

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:messenger-envelop-collection branch Nov 12, 2018

@sroze

This comment has been minimized.

Member

sroze commented Nov 12, 2018

Good idea indeed 👍

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

@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