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] make dispatch(), handle() and send() methods return Envelope#28983
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
8677998 to43ce7ddCompare
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.
Yes, much better. That sounds good.
| * @final | ||
| */ | ||
| finalclass Envelope | ||
| class 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The tests require the change for mocks.
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.
(In order to mock the new return values.)
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Data object -> no mocks. In any case, removingfinal "just" for the tests sounds like a terrible idea.
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.
I agree with@fabpot that Envelope does not need to be mocked and thus could remain final.
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.
I agree with@fabpot too
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.
reverted, the class is now really final again
Tobion 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.
I like all the recent changes in Messenger.
43ce7dd tofa63e7eComparefa63e7e to4b0e015Comparenicolas-grekas commentedOct 26, 2018
PR rebased, ready. |
sroze commentedOct 26, 2018
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
Uh oh!
There was an error while loading.Please reload this page.
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.)