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

ogizanagi · 2018-05-20T11:09:14Z

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

Nothing really new here, as I already suggested doing so in the original PR introducing the Envelope object, but we wanted first to give a try about keeping the ability to just get the message in middleware.

Now, I'm still convinced making the Envelope a first-class citizen for everything inside the bus would homogenize and simplify things for everyone and is a better design than the EnvelopeAwareInterface, while keeping the message at both ends of the bus (i.e: MessageBusInterface::dispatch and handlers).

Cons:

Not what command buses' users are already used to
Requires an extra call to $envelope->getMessage() in a middleware not using the Envelope
Envelope seems not really relevant in some parts of a CQRS / layered architecture app (...or is it?)

Pros:

Homogeneous use of the Envelope anywhere inside the bus
Strict & faithful MiddlewareInterface::handle(Envelope $envelope) typehint.
Not cheating with the non-restrictive object typehint which was never intended for this feature.
No more EnvelopeAwareInterface as there is no more core usage
No more manipulation regarding the next message or envelope consumer:
e.g:
- base message bus implementation is simplified
- middleware decorators won't have to care about the inner middleware type with the Envelope::getMessageFor($target) util (ActivationMiddlewareDecorator, TraceableMiddleware, ...). Currently it means such decorators must absolutely implement EnvelopeAwareInterface and care about this. Also see [Messenger] Add the Envelope in the documentation symfony-docs#9757 (comment)

My opinion is the naming and the concepts introduced by the Envelope VO are small and abstract (envelope, message, items) enough to even fit in the CQRS world or in a middleware in my Application namespace (when such a middleware is relevant).

(cheers from Spain)

yceruto

This looks better to me, thank you!

yceruto · 2018-05-20T14:13:03Z

src/Symfony/Component/Messenger/Tests/Asynchronous/Middleware/SendMessageMiddlewareTest.php

@@ -53,78 +39,78 @@ public function testItSendsTheMessageToAssignedSenderWithPreWrappedMessage()

    public function testItAlsoCallsTheNextMiddlewareBasedOnTheMessageClass()
    {
-        $message = new DummyMessage('Hey');
+        $envelope = Envelope::wrap(new DummyMessage('Hey'));


It feels like new Envelope(...) makes more sense here if you're sure that the message isn't another envelope instance, it also seems natural to me, IMHO. (same for other tests)

You're right. I think I'll remove this named construct as well as it doesn't really serve a purpose anymore, appart the fluent api without extra ( )

sroze · 2018-05-21T08:56:10Z

Let's move this to 4.2; time for 4.1-beta2 :)

hhamon · 2018-05-21T12:26:43Z

Postponing to 4.2 means having a BC break.

jvasseur · 2018-05-21T12:36:15Z

@hhamon BC breaks are OK since the component is experimental

ogizanagi · 2018-05-21T14:02:53Z

Right, I’ve added the BC break label already. I should have submitted this sooner, my bad. The component is experimental so we could just document the upgrade path in 4.2 but I agree this is far from being ideal to require such a change for end-users and lib maintainers.
I already agreed with @sroze to move it to 4.2. There is only a few time before 4.1 release and if this is not part of beta2 we can’t make it. ~~However if we don’t want this now, will it ever happen anyway?~~ Definitively time for 4.1-beta2. This'll wait 4.2.

sroze · 2018-06-30T11:27:00Z

I still believe that adding this is wrong: in many cases that I have, I don't care about the Envelope, just the message; and in this specific case, I don't see why I should care about it. Also, having the Envelope exposes to potential confusion like highlighted in the documentation PR: "So should I do $envelope->get(MyMessage::class) or $envelope->getMessage()?". Such complexity should be hidden from the end-user. Dealing with the Envelope is something "advanced" than developers should not have to care about at first glance IMHO.

ogizanagi · 2018-06-30T11:42:37Z

If that's the only fear you have about this, I'd suggest to add an abstract Middleware class which would just ask users to implement a handleMessage(object $message, callable $next) method instead (in the same spirit of the abstract Voter class hiding some complexity from end-users).
To me, comments & facts like in symfony/symfony-docs#9757 (comment) hint this design is a bit flawed this way.
Code removal in this PR is another hint.

sroze · 2018-07-20T09:46:17Z

Following #27841 being merged, I'm closing this one :)

ogizanagi · 2018-07-20T10:45:36Z

#27841 being merged does not change anything regarding this PR actually.
But we've already expressed each other our arguments multiple times. I would have expected to get more insights from the community after using the component and this specific feature (still too soon?), this PR allowing discussions. So, your call. 🤷‍♂️

…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

ogizanagi requested a review from sroze as a code owner May 20, 2018 11:09

carsonbot added Status: Needs Review Messenger labels May 20, 2018

ogizanagi added this to the 4.1 milestone May 20, 2018

[Messenger] Envelope as first class citizen in middleware too

a2d7960

yceruto reviewed May 20, 2018

View reviewed changes

sroze modified the milestones: 4.1, next May 21, 2018

ogizanagi added the BC Break label May 21, 2018

ogizanagi mentioned this pull request May 22, 2018

[Messenger] Add the Envelope in the documentation symfony/symfony-docs#9757

Merged

[Messenger] Remove Envelope::wrap

b3a6768

nicolas-grekas changed the base branch from 4.1 to master June 19, 2018 13:19

sroze mentioned this pull request Jul 4, 2018

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

Merged

sroze closed this Jul 20, 2018

ogizanagi mentioned this pull request Oct 16, 2018

[Messenger] use Envelope internally, return void, add EnvelopeHandlerInterface and other cleanups #28881

Closed

nicolas-grekas mentioned this pull request Oct 18, 2018

[Messenger] make Envelope first class citizen for middleware handlers #28914

Merged

ogizanagi deleted the messenger/middleware_envelope branch October 31, 2018 17:40

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 as first class citizen in middleware too #27322

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

ogizanagi commented May 20, 2018 •

edited

Loading

yceruto left a comment

yceruto May 20, 2018

ogizanagi May 20, 2018 •

edited

Loading

sroze commented May 21, 2018

hhamon commented May 21, 2018

jvasseur commented May 21, 2018

ogizanagi commented May 21, 2018 •

edited

Loading

sroze commented Jun 30, 2018

ogizanagi commented Jun 30, 2018 •

edited

Loading

sroze commented Jul 20, 2018

ogizanagi commented Jul 20, 2018

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

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

Conversation

ogizanagi commented May 20, 2018 • edited Loading

yceruto left a comment

Choose a reason for hiding this comment

yceruto May 20, 2018

Choose a reason for hiding this comment

ogizanagi May 20, 2018 • edited Loading

Choose a reason for hiding this comment

sroze commented May 21, 2018

hhamon commented May 21, 2018

jvasseur commented May 21, 2018

ogizanagi commented May 21, 2018 • edited Loading

sroze commented Jun 30, 2018

ogizanagi commented Jun 30, 2018 • edited Loading

sroze commented Jul 20, 2018

ogizanagi commented Jul 20, 2018

ogizanagi commented May 20, 2018 •

edited

Loading

ogizanagi May 20, 2018 •

edited

Loading

ogizanagi commented May 21, 2018 •

edited

Loading

ogizanagi commented Jun 30, 2018 •

edited

Loading