Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] Envelope as first class citizen in middleware too#27322
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
yceruto left a comment
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.
This looks better to me, thank you!
| publicfunctiontestItAlsoCallsTheNextMiddlewareBasedOnTheMessageClass() | ||
| { | ||
| $message =newDummyMessage('Hey'); | ||
| $envelope =Envelope::wrap(newDummyMessage('Hey')); |
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.
It feels likenew Envelope(...) makes more sense here if you're sure that the message isn't another envelope instance, it also seems natural to me, IMHO. (same for other tests)
ogizanagiMay 20, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
You're right. I think I'll remove this named construct as well as it doesn't really serve a purpose anymore, appart the fluent api without extra()
sroze commentedMay 21, 2018
Let's move this to 4.2; time for 4.1-beta2 :) |
hhamon commentedMay 21, 2018
Postponing to 4.2 means having a BC break. |
jvasseur commentedMay 21, 2018
@hhamon BC breaks are OK since the component is experimental |
ogizanagi commentedMay 21, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Right, I’ve added the BC break label already. I should have submitted this sooner, my bad. The component is experimental so we could just document the upgrade path in 4.2 but I agree this is far from being ideal to require such a change for end-users and lib maintainers. |
sroze commentedJun 30, 2018
I still believe that adding this is wrong: in many cases that I have, I don't care about the |
ogizanagi commentedJun 30, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
If that's the only fear you have about this, I'd suggest to add an abstract |
sroze commentedJul 20, 2018
Following#27841 being merged, I'm closing this one :) |
ogizanagi commentedJul 20, 2018
#27841 being merged does not change anything regarding this PR actually. |
…leware handlers (nicolas-grekas)This PR was merged into the 4.2-dev branch.Discussion----------[Messenger] make Envelope first class citizen for middleware handlers| Q | A| ------------- | ---| Branch? | 4.2| Bug fix? | no| New feature? | no| BC breaks? | yes| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -This PR sits on top of#28909 so that only the 2nd commit should be reviewed for now, until I rebase it.This idea has already been proposed and rejected in#27322.Yet, now that I've reviewed in depth the component, I politely but strongly suggest to reconsider.Middleware handlers are the central piece of the routing layer.When `dispatch()` accepts `object|Envelope`, it's because it sits on the outer boundary of the component. `handle()` on the contrary plugs inside its core routing logic, so that it's very legitimate to require them to deal with `Envelope` objects. Yes, some middleware care only about the message inside. That's fine calling `getMessage()` for them.Right now, we built a complex magic layer that acts as a band-aid *just* to save this call to `getMessage()`. For middleware that want the envelope, we require an additional interface that magically *changes* the expected argument. That's very fragile design: it breaks the `L` in `SOLID`...Looking at the diff stat, this is most natural.Commits-------ae46a43 [Messenger] make Envelope first class citizen for middleware handlers
Uh oh!
There was an error while loading.Please reload this page.
Nothing really new here, as I already suggested doing so in the original PR introducing the
Envelopeobject, but we wanted first to give a try about keeping the ability to just get the message in middleware.Now, I'm still convinced making the Envelope a first-class citizen for everything inside the bus would homogenize and simplify things for everyone and is a better design than the
EnvelopeAwareInterface, while keeping the message at both ends of the bus (i.e:MessageBusInterface::dispatchand handlers).Cons:
$envelope->getMessage()in a middleware not using theEnvelopeEnvelopeseems not really relevant in some parts of a CQRS / layered architecture app (...or is it?)Pros:
Envelopeanywhere inside the busMiddlewareInterface::handle(Envelope $envelope)typehint.Not cheating with the non-restrictive
objecttypehint which was never intended for this feature.EnvelopeAwareInterfaceas there is no more core usagee.g:
Envelope::getMessageFor($target)util (ActivationMiddlewareDecorator,TraceableMiddleware, ...). Currently it means such decorators must absolutely implementEnvelopeAwareInterfaceand care about this. Also see[Messenger] Add the Envelope in the documentation symfony-docs#9757 (comment)My opinion is the naming and the concepts introduced by the
EnvelopeVO are small and abstract (envelope, message, items) enough to even fit in the CQRS world or in a middleware in myApplicationnamespace (when such a middleware is relevant).(cheers from Spain)