Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekasnicolas-grekas commentedOct 18, 2018
edited
Loading

QA
Branch?4.2
Bug fix?no
New feature?no
BC breaks?yes
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
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.
Whendispatch() 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 withEnvelope objects. 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 togetMessage(). For middleware that want the envelope, we require an additional interface that magicallychanges the expected argument. That's very fragile design: it breaks theL inSOLID...

Looking at the diff stat, this is most natural.

dmaicher, fbourigault, Koc, and Tobion reacted with thumbs up emojiogizanagi, yceruto, and skalpa reacted with heart emoji
Copy link
Contributor

@ogizanagiogizanagi left a comment
edited
Loading

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.

@nicolas-grekasnicolas-grekasforce-pushed themessenger-envelope-middleware branch 3 times, most recently fromfbb0d2a to881e039CompareOctober 20, 2018 18:56
@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedOct 20, 2018
edited
Loading

Should we remove Envelope::wrap()

yes! same forEnvelope->withMessage() as it's unused anymore. Done.

@nicolas-grekasnicolas-grekasforce-pushed themessenger-envelope-middleware branch from881e039 toae46a43CompareOctober 21, 2018 12:55
@nicolas-grekas
Copy link
MemberAuthor

(rebased, ready)

Copy link
Contributor

@srozesroze left a 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
Copy link
Contributor

Thank you@nicolas-grekas.

@srozesroze merged commitae46a43 intosymfony:masterOct 21, 2018
sroze added a commit that referenced this pull requestOct 21, 2018
…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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Where it used?

Copy link
MemberAuthor

@nicolas-grekasnicolas-grekasOct 21, 2018
edited
Loading

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.

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

Reviewers

@weaverryanweaverryanweaverryan approved these changes

+3 more reviewers

@KocKocKoc left review comments

@srozesrozesroze approved these changes

@ogizanagiogizanagiogizanagi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

6 participants

@nicolas-grekas@sroze@weaverryan@Koc@ogizanagi@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp