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 MessengerPass less strict when auto-register handlers#29525

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

Conversation

@nicholasruunu
Copy link
Contributor

@nicholasruununicholasruunu commentedDec 8, 2018
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
LicenseMIT
Doc PRnot needed

This allows you to auto-register handlers that have more than one argument, which is useful when having custom middleware to pass more parameters.

#symfonyconhackday2018

@ogizanagi
Copy link
Contributor

ogizanagi commentedDec 8, 2018
edited
Loading

Well, usually handlers should be pure business/application logic and envelope stamps mostly technical stuff about the transport or cross cutting concerns from middleware. If the data you need from stamps is relevant to the handler, you should think twice if it should not be part of the message itself. If it can't and can only be added as a stamp, is it really the handler's responsibility to be aware of this and act accordingly?

But actually on my side, I'm not against this PR right now. I even thought to suggest this before but was waiting for real demands. What I expressed above only is my own experience with buses within CQRS applications. Not canonical. And bringing more flexibility here might be interesting without implying much overhead.

@nicholasruunu
Copy link
ContributorAuthor

@ogizanagi Yeah, I don't know how common this would be but our need comes from using PubNub and having to apply the channel to respond to when sending another message from the handler.

Having the channel be part of the message is a solution but that would really mix infrastructure concerns with our business logic.

@ro0NL
Copy link
Contributor

But having the envelope be part of the message is also mixing infrastructure (sf messenger) with domain logic 🤔

It sounds like a middleware concern IMHO.

sstok reacted with thumbs up emoji

@nicholasruunu
Copy link
ContributorAuthor

nicholasruunu commentedDec 9, 2018
edited
Loading

@ro0NL It's true, if it's possible to have information from one message to another in the middleware then let me know. Also, handlers you can change, messages you really shouldn't change too much.

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedDec 13, 2018
edited
Loading

I agree with@ro0NL: handlers are designed to get only the message and not the envelope.
If you need metadata related to the transport layer, you should write a middleware instead - or a sender.

@nicholasruunu
Copy link
ContributorAuthor

@nicolas-grekas Yeah, we did solve it with a custom middleware, but it's not really a nice experience atm, but doable.

One problem still exist with auto-register handlers with more than one invoke parameters. I think the message-pass should look like what I did in this PR.

$parameters =$method->getParameters();
if (1 !==\count($parameters)) {
thrownewRuntimeException(sprintf('Invalid handler service "%s": method "%s::__invoke()"must have exactly oneargumentcorresponding to the message it handles.',$serviceId,$handlerClass->getName()));
if (0 ===\count($parameters)) {

Choose a reason for hiding this comment

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

maybe use getNumberOfRequiredParameters?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I changed it, although not sure if it helps against(Message $message = null) since you could have(Message $message = null, Envelope $envelope)?

@nicholasruununicholasruunu changed the title[Messenger] Pass envelope as second argument to handlers[Messenger] Make MessangerPass less strict when auto-register handlersJan 10, 2019
@nicholasruununicholasruunu changed the title[Messenger] Make MessangerPass less strict when auto-register handlers[Messenger] Make MessengerPass less strict when auto-register handlersJan 10, 2019
@nicholasruunu
Copy link
ContributorAuthor

I updated the PR to only deal with auto-registering in MessengerPass.

@xabbuh
Copy link
Member

the changes seem reasonable to me

@nicholasruununicholasruunuforce-pushed thefeature/pass-envelope-to-handler branch from303051a to49ab2cdCompareMarch 31, 2019 14:45
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.

Sounds reasonable to me as well 👍

@sroze
Copy link
Contributor

Thank you@nicholasruunu.

@srozesroze merged commit49ab2cd intosymfony:masterApr 6, 2019
sroze added a commit that referenced this pull requestApr 6, 2019
…ister handlers (nicholasruunu)This PR was merged into the 4.3-dev branch.Discussion----------[Messenger] Make MessengerPass less strict when auto-register handlers| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| License       | MIT| Doc PR        | not neededThis allows you to auto-register handlers that have more than one argument, which is useful when having custom middleware to pass more parameters.#symfonyconhackday2018Commits-------49ab2cd Make MessengerPass less strict when auto-register handlers
@nicholasruununicholasruunu deleted the feature/pass-envelope-to-handler branchApril 6, 2019 12:15
@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

+1 more reviewer

@srozesrozesroze approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

8 participants

@nicholasruunu@ogizanagi@ro0NL@nicolas-grekas@xabbuh@sroze@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp