[Messenger] Envelope-aware middleware is never called with a message #27841

Cydonia7 · 2018-07-04T09:58:45Z

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
Fixed tickets	no
License	MIT
Doc PR	no

Messenger 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.

sroze

Can you rebase on 4.1 and change your PR's base as well?

sroze · 2018-07-04T12:04:07Z

src/Symfony/Component/Messenger/Asynchronous/Middleware/SendMessageMiddleware.php

@@ -36,12 +35,11 @@ public function __construct(SenderLocatorInterface $senderLocator, array $messag
    /**
     * {@inheritdoc}
     */
-    public function handle($message, callable $next)
+    public function handle($envelope, callable $next)


Can you add a @param Envelope $envelope above the {@inheritdoc}? I think that this would be better to explicit it's an Envelope.

Cydonia7 · 2018-07-04T12:41:01Z

@sroze Comments addressed. I think failures on Appveyor are not related to this PR.

nicolas-grekas · 2018-07-04T15:41:08Z

src/Symfony/Component/Messenger/Asynchronous/Middleware/SendMessageMiddleware.php

@@ -34,14 +34,15 @@ public function __construct(SenderLocatorInterface $senderLocator, array $messag
    }

    /**
+     * @param Envelope $envelope


Since the component is experimental, we can turn this to a real type hint on master at least?

There is a PR for this (#27322) but I don't think that it's a good idea. At all.

nicolas-grekas · 2018-07-04T15:41:12Z

src/Symfony/Component/Messenger/Asynchronous/Middleware/SendMessageMiddleware.php

    {
-        $envelope = Envelope::wrap($message);


Could this introduce any BC break?

Nop, because EnvelopeAwareInterface, the contract is that it gets an Envelope.

ogizanagi · 2018-07-04T22:40:55Z

Just to be sure: this is not fixing a bug at all, just removing support of a message in EnvelopeAwareInterface middlewares, right?

Our bus implementation ensures it always receives the Envelope, it's true, but nothing in the design really allows a stronger enforcement here. See symfony/symfony-docs#9757 (comment) discussion and #27322 already mentioned above.
(Note I'm not against this PR, but still proves to me #27322 would be a better design).

Also, the ValidationMiddleware could benefit from similar changes.

sroze · 2018-07-08T09:32:25Z

Just to be sure: this is not fixing a bug at all, just removing support of a message in EnvelopeAwareInterface middlewares, right?

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 EnvelopeAwareInterface. My 👍 is for this change, so we clarify that this handling of EnvelopeAwareInterface is part of the MessageBusInterface (we could add a comment to it).

@Cydonia7 As per @ogizanagi's comment, can you update the ValidationMiddleware file as well?

ogizanagi · 2018-07-08T20:51:29Z

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 Envelope::wrap as in b3a6768 as the only reason it exists in core was for this (I'll cherry-pick the commit later).

Cydonia7 · 2018-07-09T11:44:47Z

@ogizanagi @sroze I've rebased on master and adapted the ValidationMiddleware in the same way.

fabpot · 2018-07-19T06:53:30Z

@ogizanagi Can you review this PR please?

ogizanagi

That's ok with latest changes 👍
But we miss a mention for the BC break in the UPGRADE file.

Cydonia7 · 2018-07-19T08:18:55Z

@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 · 2018-07-19T08:23:11Z

@Cydonia7 it means it goes to the next release. So 4.2.

sroze · 2018-07-19T08:24:33Z

UPGRADE-4.2.md

+Messenger
+---------
+
+ * The `Symfony\Component\Messenger\Middleware\ValidationMiddleware` and `Symfony\Component\Messenger\Asynchronous\Middleware\SendMessageMiddleware` do not support messages in their `handle` method anymore.


We need more than this. I suggest adding something like:

...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).

Thanks for the review, I just added it.

sroze · 2018-07-19T09:11:07Z

UPGRADE-4.2.md

+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).


Add around theEnvelope::wrap` example.

Also, I think that I'd phrase it like:

The handle method of the X and Y middleware now requires an Envelope object to be given (because they implement the EnvelopeAwareInterface. 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 use Envelope::wrap($message) to create an envelope for your message.

It looks a lot better like that.

sroze · 2018-07-19T10:41:07Z

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  | Deprecations? | no | Tests pass? | yes | Fixed tickets | no | License | MIT | Doc PR | no Messenger 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

carsonbot added Status: Needs Review Messenger Bug labels Jul 4, 2018

sroze suggested changes Jul 4, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jul 4, 2018

Cydonia7 changed the base branch from master to 4.1 July 4, 2018 12:20

nicolas-grekas added this to the 4.1 milestone Jul 4, 2018

nicolas-grekas added Status: Needs Review and removed Status: Needs Work labels Jul 4, 2018

nicolas-grekas reviewed Jul 4, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jul 4, 2018

sroze approved these changes Jul 4, 2018

View reviewed changes

nicolas-grekas approved these changes Jul 4, 2018

View reviewed changes

ogizanagi added the Status: Waiting feedback label Jul 5, 2018

ogizanagi removed the Bug label Jul 8, 2018

ogizanagi removed the Status: Waiting feedback label Jul 8, 2018

Cydonia7 requested review from dunglas and lyrixx as code owners July 9, 2018 11:42

carsonbot added Status: Needs Review and removed Status: Reviewed labels Jul 9, 2018

Cydonia7 changed the base branch from 4.1 to master July 9, 2018 11:42

fabpot removed this from the 4.1 milestone Jul 19, 2018

fabpot added this to the next milestone Jul 19, 2018

ogizanagi added the BC Break label Jul 19, 2018

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jul 19, 2018

ogizanagi approved these changes Jul 19, 2018

View reviewed changes

sroze reviewed Jul 19, 2018

View reviewed changes

[Messenger] Envelope-aware middleware is never called with a message

9488e2a

sroze merged commit 9488e2a into symfony:master Jul 19, 2018

Cydonia7 deleted the envelope-aware-middleware branch July 19, 2018 11:27

sroze mentioned this pull request Jul 20, 2018

[Messenger] Envelope as first class citizen in middleware too #27322

Closed

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Messenger] Envelope-aware middleware is never called with a message #27841

[Messenger] Envelope-aware middleware is never called with a message #27841

Cydonia7 commented Jul 4, 2018 •

edited by ogizanagi

Loading

sroze left a comment

sroze Jul 4, 2018

Cydonia7 commented Jul 4, 2018 •

edited

Loading

nicolas-grekas Jul 4, 2018

sroze Jul 4, 2018

nicolas-grekas Jul 4, 2018

sroze Jul 4, 2018

ogizanagi commented Jul 4, 2018

sroze commented Jul 8, 2018

ogizanagi commented Jul 8, 2018 •

edited

Loading

Cydonia7 commented Jul 9, 2018

fabpot commented Jul 19, 2018

ogizanagi left a comment

Cydonia7 commented Jul 19, 2018

sroze commented Jul 19, 2018

sroze Jul 19, 2018

Cydonia7 Jul 19, 2018

sroze Jul 19, 2018

Cydonia7 Jul 19, 2018

sroze commented Jul 19, 2018

[Messenger] Envelope-aware middleware is never called with a message #27841

[Messenger] Envelope-aware middleware is never called with a message #27841

Conversation

Cydonia7 commented Jul 4, 2018 • edited by ogizanagi Loading

sroze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cydonia7 commented Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi commented Jul 4, 2018

sroze commented Jul 8, 2018

ogizanagi commented Jul 8, 2018 • edited Loading

Cydonia7 commented Jul 9, 2018

fabpot commented Jul 19, 2018

ogizanagi left a comment

Choose a reason for hiding this comment

Cydonia7 commented Jul 19, 2018

sroze commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sroze commented Jul 19, 2018

Cydonia7 commented Jul 4, 2018 •

edited by ogizanagi

Loading

Cydonia7 commented Jul 4, 2018 •

edited

Loading

ogizanagi commented Jul 8, 2018 •

edited

Loading