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] make Envelope first class citizen for middleware handlers#28914
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
[Messenger] make Envelope first class citizen for middleware handlers#28914
Uh oh!
There was an error while loading.Please reload this page.
Conversation
23527d2 to1f42392Compare1f42392 to5c8f083Compare
ogizanagi left a comment• 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.
Should we removeEnvelope::wrap() as in original PR or do you consider it still relevant? We can decide this in another PR though.
fbb0d2a to881e039Comparenicolas-grekas commentedOct 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.
yes! same for |
881e039 toae46a43Comparenicolas-grekas commentedOct 21, 2018
(rebased, ready) |
sroze 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.
You've made@ogizanagi happy. 😄
sroze commentedOct 21, 2018
Thank you@nicolas-grekas. |
…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
| /** | ||
| * @internal | ||
| */ | ||
| interface NextInterface |
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.
Where it used?
nicolas-grekasOct 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.
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.
In the docblock above, to document the signature of the callable.
Uh oh!
There was an error while loading.Please reload this page.
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()acceptsobject|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 withEnvelopeobjects. Yes, some middleware care only about the message inside. That's fine callinggetMessage()for them.Right now, we built a complex magic layer that acts as a band-aidjust to save this call to
getMessage(). For middleware that want the envelope, we require an additional interface that magicallychanges the expected argument. That's very fragile design: it breaks theLinSOLID...Looking at the diff stat, this is most natural.