-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Messenger] use Envelope internally, return void, add EnvelopeHandlerInterface and other cleanups #28881
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
2d7176a
to
1fce598
Compare
* | ||
* @author Nicolas Grekas <p@tchwork.com> | ||
*/ | ||
interface EnvelopeHandlerInterface extends MessageHandlerInterface |
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.
👍
* @return mixed | ||
*/ | ||
public function handle($message, callable $next); | ||
public function handle(Envelope $envelope, callable $next): void; |
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.
👍
'message' => $message, | ||
'class' => \get_class($message), | ||
); | ||
$this->logger->debug('Finished handling message {class}', $context); |
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.
s/{class}/{name}
what about query bus? It would be nice provide query bus out of the box |
59ff103
to
eefb815
Compare
They could use mutable command objects (e.g. a Right now, the return value blurs the scope of the component to me. Also look at e.g. ChainHandler: returning the array of all results makes no sense: this is configuration leaking to the outside. |
eefb815
to
1964754
Compare
I didn't review this yet, but you've got my 👍 for this. I already suggested this in the past (#27322) and suggested an abstract Meanwhile, I'm not convinced |
Since |
Note that I may be missing the reason why #27322 has been rejected: right now, I'm thinking about middlewares as things that are tightly bound to Messenger so that they legitimately know about it, and about Envelope especially. |
It's not about the envelope being internal or not regarding this. Envelopes are meant for runtime "config" of middleware & transports. If the handler needs some data to treat the message, it should be part of it, not the envelope. |
1964754
to
77b83ab
Compare
aren't there any cases where the handler will want to know about the way routing did happen? E.g. number of retries, timestamps, some routing/logging identifier, etc. - Things that could be of interest for some handlers? |
Obviously not to me. Handlers are pure business logic. The use-cases you mention are valid as a middleware (which if really needed can mutate the message, but those are usually supposed immutable). |
foreach ($this->handlers as $handler) { | ||
$results[] = $handler($message); | ||
$handler($envelope instanceof EnvelopeHandlerInterface ? $envelope : $envelope->getMessage()); |
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.
$handler instanceof
77b83ab
to
0ea6d4c
Compare
Argument accepted, makes perfect sense. That's another argument to make middlewares deal with envelopes: they're part of the routing - use handlers for the part that is outside. |
0ea6d4c
to
45c59f1
Compare
45c59f1
to
361f3f6
Compare
…Interface and other cleanups
361f3f6
to
f80cbca
Compare
Sorry in advance for this late PR, I've just reviewed the component for the first time and I think we should do some changes in its internal design, so here we are, last chance before tagging it as stable.
Each item in this list can potentially trigger a discussion. Here are the topics of this PR:
Envelope
objects (middlewares, transports, etc.)replace[edit: bad idea, see discussion]EnvelopeAwareInterface
(for middlewares) byEnvelopeHandlerInterface
(for userland handlers)dispatch()
+handle()
: we never use it and it contradicts the very idea of async (mutable messages can be used instead, but that's not our business)string $name = null
as 2nd argument to dispatch(), used as dispatching key when provided for more flexibility (it's not always possible/desirable to create a new class to define a dispatching key)locating senders/handlers using the parent classes+interfaces of the message is removed.To me, locating based on the type hierarchy is a code smell that we hijack the type hierarchy to make it fit what should be done via configuration. [edit: now kept to reduce the impact of this PR - could be deprecated later if this smell is proven by experience in the future]AllowNoHandlerMiddleware
and replace it with a constructor argument onHandleMessageMiddleware
: using exceptions for signaling is going to slow down the component for no real reasons to me. This solves it.I expect this to raise some discussions. To resolve them one by one, I propose to split this PR into separate ones while we move our common understanding forward.
Meanwhile, I didn't update tests nor DI integration. I'll do as this moves forward.