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-aware middleware is never called with a message#27841
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
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.
Can you rebase on 4.1 and change your PR's base as well?
| * {@inheritdoc} | ||
| */ | ||
| publicfunctionhandle($message,callable$next) | ||
| publicfunctionhandle($envelope,callable$next) |
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.
Can you add a@param Envelope $envelope above the{@inheritdoc}? I think that this would be better to explicit it's an Envelope.
Cydonia7 commentedJul 4, 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.
@sroze Comments addressed. I think failures on Appveyor are not related to this PR. |
| } | ||
| /** | ||
| * @param Envelope $envelope |
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.
Since the component is experimental, we can turn this to a real type hint on master at least?
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.
There is a PR for this (#27322) but I don't think that it's a good idea. At all.
| publicfunctionhandle($message,callable$next) | ||
| publicfunctionhandle($envelope,callable$next) | ||
| { | ||
| $envelope = Envelope::wrap($message); |
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.
Could this introduce any BC break?
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.
Nop, becauseEnvelopeAwareInterface, the contract is that it gets anEnvelope.
ogizanagi commentedJul 4, 2018
Just to be sure: this is not fixing a bug at all, just removing support of a message in Our bus implementation ensures it always receives the Envelope, it's true, but nothing in the design really allows a stronger enforcement here. Seesymfony/symfony-docs#9757 (comment) discussion and#27322 already mentioned above. Also, the |
sroze commentedJul 8, 2018
You are right actually. Maybe we should keep it that way to ensure the middlewares could be used with another bus implementation that does not support the @Cydonia7 As per@ogizanagi's comment, can you update the |
ogizanagi commentedJul 8, 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.
So, this cannot be merged in 4.1 as not acceptable in a patch release to me. PR should target and be rebased on master and removal of the support of messages not wrapped in an Envelope be mentioned in the UPGRADE file. Also I'd suggest to remove |
Cydonia7 commentedJul 9, 2018
@ogizanagi@sroze I've rebased on master and adapted the |
fabpot commentedJul 19, 2018
@ogizanagi Can you review this PR please? |
ogizanagi 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.
That's ok with latest changes 👍
But we miss a mention for the BC break in the UPGRADE file.
Cydonia7 commentedJul 19, 2018
@ogizanagi I just added a note in the UPGRADE file for 4.2 @fabpot I see you've changed the milestone to 4.1 and next, does this mean this will be inside the next patch or next minor? (If it's patch, then I should move the UPGRADE note) |
sroze commentedJul 19, 2018
@Cydonia7 it means it goes to the next release. So 4.2. |
UPGRADE-4.2.md Outdated
| Messenger | ||
| --------- | ||
| * The`Symfony\Component\Messenger\Middleware\ValidationMiddleware` and`Symfony\Component\Messenger\Asynchronous\Middleware\SendMessageMiddleware` do not support messages in their`handle` method anymore. |
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.
We need more than this. I suggest adding something like:
...because they implement the
EnvelopeAwareInterface. You need to give them an instance of theEnvelopeobject, wrapping your message. You can useEnvelope::wrap($message).
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.
Thanks for the review, I just added it.
UPGRADE-4.2.md Outdated
| Messenger | ||
| --------- | ||
| * The`Symfony\Component\Messenger\Middleware\ValidationMiddleware` and`Symfony\Component\Messenger\Asynchronous\Middleware\SendMessageMiddleware` do not support messages in their`handle` method anymore because they implement the`EnvelopeAwareInterface`. You need to give them an instance of the`Envelope` object, wrapping your message. You can use Envelope::wrap($message). |
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.
Addaround theEnvelope::wrap` example.
Also, I think that I'd phrase it like:
The
handlemethod of the X and Y middleware now requires anEnvelopeobject to be given (because they implement theEnvelopeAwareInterface. When using these middleware with the provided MessageBus, you will not have to do anything. If you use the middleware any other way, you can useEnvelope::wrap($message)to create an envelope for your message.
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 looks a lot better like that.
sroze commentedJul 19, 2018
Thank you@Cydonia7. |
…th a message (Cydonia7)This PR was merged into the 4.2-dev branch.Discussion----------[Messenger] Envelope-aware middleware is never called with a message| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | yes <!-- seehttps://symfony.com/bc -->| Deprecations? | no| Tests pass? | yes| Fixed tickets | no| License | MIT| Doc PR | noMessenger components middlewares implementing `Symfony\Component\Messenger\EnvelopeAwareInterface` receive in their handle method an instance of `Symfony\Component\Messenger\Envelope`, not a message.To better reflect the expected usage, I've updated the unit tests for the message middleware and fixed the code accordingly.Commits-------9488e2a [Messenger] Envelope-aware middleware is never called with a message
Uh oh!
There was an error while loading.Please reload this page.
Messenger components middlewares implementing
Symfony\Component\Messenger\EnvelopeAwareInterfacereceive in their handle method an instance ofSymfony\Component\Messenger\Envelope, not a message.To better reflect the expected usage, I've updated the unit tests for the message middleware and fixed the code accordingly.