[Messenger] use Envelope internally, return void, add EnvelopeHandlerInterface and other cleanups #28881
Conversation
2d7176a to
1fce598
Compare
| * | ||
| * @author Nicolas Grekas <p@tchwork.com> | ||
| */ | ||
| interface EnvelopeHandlerInterface extends MessageHandlerInterface |
| * @return mixed | ||
| */ | ||
| public function handle($message, callable $next); | ||
| public function handle(Envelope $envelope, callable $next): void; |
| 'message' => $message, | ||
| 'class' => \get_class($message), | ||
| ); | ||
| $this->logger->debug('Finished handling message {class}', $context); |
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 m 8000 ention 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()); |
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:
Envelopeobjects (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 = nullas 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]AllowNoHandlerMiddlewareand 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.