[Messenger] make dispatch(), handle() and send() methods return Envelope #28983
Conversation
8677998 to
43ce7dd
Compare
| */ | ||
| final class Envelope | ||
| class Envelope |
There was a problem hiding this comment.
Why is the reason do remove PHP final here? I understand that the @final is not enforced, probably to allow people to override the class, but then what's the point?
There was a problem hiding this comment.
The tests require the change for mocks.
There was a problem hiding this comment.
(In order to mock the new return values.)
There was a problem hiding this comment.
You could consider to introduce an interface or to just create the Envelope object in tests (instead of mocking it). I don't see it mocked anywhere by the way.
There was a problem hiding this comment.
Data object -> no mocks. In any case, removing final "just" for the tests sounds like a terrible idea.
There was a problem hiding this comment.
I agree with @fabpot that Envelope does not need to be mocked and thus could remain final.
There was a problem hiding this comment.
I agree with @fabpot too
There was a problem hiding this comment.
reverted, the class is now really final again
43ce7dd to
fa63e7e
Compare
fa63e7e to
4b0e015
Compare
|
PR rebased, ready. |
|
Thank you @nicolas-grekas. |
…ds return Envelope (nicolas-grekas) This PR was merged into the 4.2-dev branch. Discussion ---------- [Messenger] make dispatch(), handle() and send() methods return Envelope | Q | A | ------------- | --- | Branch? | 4.2 | Bug fix? | no | New feature? | yes | BC breaks? | no (already broken ;) ) | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Follow up of #28909. We can do better than return `void`: let's return `Envelope`! This means middleware and senders should also return `Envelope`, so that we can typically read back the stamps that were added during the sending process (eg to get the Kafka queue id). ping @dunglas as we discussed that first on Slack, and @sroze as we confirmed interest IRL today. (User handlers don't know anything about envelopes so they still should return `void` - use senders or middleware if you need to populate/read envelopes.) Commits ------- 4b0e015 [Messenger] make dispatch(), handle() and send() methods return Envelope
…wareInterface bc-break (skalpa) This PR was merged into the 4.2-dev branch. Discussion ---------- [Messenger] Fix DoctrineTransactionMiddleware after MiddlewareInterface bc-break | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Fixes `DoctrineTransactionMiddleware` that got broken by #28983, and adds tests. Commits ------- 378ca06 Fix DoctrineTransactionMiddleware after MiddlewareInterface bc-break
Follow up of #28909. We can do better than return
void: let's returnEnvelope!This means middleware and senders should also return
Envelope, so that we can typically read back the stamps that were added during the sending process (eg to get the Kafka queue id).ping @dunglas as we discussed that first on Slack, and @sroze as we confirmed interest IRL today.
(User handlers don't know anything about envelopes so they still should return
void- use senders or middleware if you need to populate/read envelopes.)