8000 [Messenger] allow specifying the topic when calling dispatch() by nicolas-grekas · Pull Request #28944 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Messenger] allow specifying the topic when calling dispatch() #28944

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Messenger] allow specifying the topic when calling dispatch() #28944

wants to merge 1 commit into from

Conversation

nicolas-grekas
Copy link
Member
@nicolas-grekas nicolas-grekas commented Oct 21, 2018 •
edited
Loading
Q A
Branch? 4.2
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

When dispatching a message, it's not always possible to know its "class" beforehand. Currently, the component suffers from this limitation of not allowing one to dispatch messages based on a string that would be provided at runtime.

Dispatching by parent classes + interfaces looks like a workaround for this limitation to me. Binding the type-system and configuration is what made autowiring clumsy in 2.8. The issue was that changing/refactoring some code would have unrelated and unwanted side effects. Here, that'd be the same on the way messages are dispatched. Adding a dispatch key fixes this issue by allowing one to opt-out from type-based dispatching, without removing this strategy (time will tell if I'm wrong)

Anyway, allowing to provide a runtime-computed dispatch key is a missing core feature.
It allows both:

  • dispatching several subclasses under the same key, allowing to use their type as useful info for a common set of handlers/senders
  • dispatching the same class under different keys, allowing one to provide more specialized dispatching than just the class when appropriate (when the key is known at runtime).

@nicolas-grekas nicolas-grekas added this to the 4.2 milestone Oct 21, 2018
@nicolas-grekas nicolas-grekas requested a review from sroze as a code owner October 21, 2018 22:18
ro0NL
ro0NL reviewed Oct 22, 2018
View reviewed changes
ogizanagi
ogizanagi reviewed Oct 22, 2018
View reviewed changes
Copy link
Contributor
@ogizanagi ogizanagi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this dispatching key would mainly be used for the two use cases mentioned beforehand, do we really need to expose $key in the MessageBusInterface::dispatch() signature rather than letting users to stamp themselves when needed?

@nicolas-grekas
Copy link
Member Author

do we really need to expose $key in the MessageBusInterface::dispatch()

I think so yes: looking at e.g. EventDispatcher interface, this dispatching key is decoupled from the concept of an Envelope: even if we use an Envelope actually to convey it, that's an implementation detail to me. At the abstraction level, providing only the message to decide the dispatching key is creating a half backed abstraction, missing the dispatch key :)

ro0NL
ro0NL reviewed Oct 22, 2018
View reviewed changes
@nicolas-grekas nicolas-grekas force-pushed the messenger-name branch 2 times, most recently from 3c837db to ffc553b Compare October 22, 2018 19:52
@ro0NL
Copy link
Contributor
ro0NL commented Oct 23, 2018

symfony/src/Symfony/Bundle/FrameworkBundle/Controller/ControllerTrait.php

Line 393 in 73032f3

protected function dispatchMessage($message)
should be updated as well

@nicolas-grekas
Copy link
Member Author

protected function dispatchMessage($message) should be updated as well

good catch, fixed thank.

@nicolas-grekas nicolas-grekas force-pushed the messenger-name branch 2 times, most recently from 7acc671 to 9c3d5b0 Compare October 25, 2018 14:15
@nicolas-grekas
Copy link
Member Author
nicolas-grekas commented Oct 25, 2018 •
edited
Loading

PR rebased and updated: getDispatchKey now always returns a key, and senders and handlers locators get only this key as argument. This makes the type-based dispatching an implementation detail of the locators' behavior, as it should. This resolves the discussion above with @ro0NL.

@nicolas-grekas nicolas-grekas changed the title [Messenger] allow specifying the dispatch key when calling dispatch() [Messenger] allow specifying the message name when calling dispatch() Oct 25, 2018
Tobion
Tobion suggested changes Oct 25, 2018
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Controller/ControllerTrait.php Outdated
*
* @final
*/
protected function dispatchMessage($message)
protected function dispatchMessage($message, string $key = null): void
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you forgot to update $key here

@ro0NL
Copy link
Contributor
ro0NL commented Oct 25, 2018 •
edited
Loading

i'd say $messageKey, which to me implies an "address" already. Somehow "address" feels less intuitive to me, as its implementation is just a "key".

key over name might imply "is unique", thus an address.

@sroze
Copy link
Contributor
sroze commented Oct 25, 2018

We thought about address as well but our reasoning is that messageKey works for both the handler & the sender resolution while address only works for the sender.

@Tobion
Copy link
Contributor
Tobion commented Oct 25, 2018 •
edited
Loading

We thought about address as well but our reasoning is that messageKey works for both the handler & the sender resolution while address only works for the sender.

The handler is the recipient of the message which is exactly the purpose of an adding an address. I cannot follow your argument.

@nicolas-grekas
Copy link
Member Author
nicolas-grekas commented Oct 26, 2018 •
edited
Loading

IMHO, "address" is pushing the metaphor a bit too far. There is a difference here with postal routing: the dispatcher doesn't want to target a specific handler/sender. It just dispatches a name - and then - maybe - a sender/handler will subscribe to the topic.

@nicolas-grekas
Copy link
Member Author

what about $topic+getTopic()?

@nicolas-grekas nicolas-grekas changed the title [Messenger] allow specifying the message name when calling dispatch() [Messenger] allow specifying the topic when calling dispatch() Oct 26, 2018
@nicolas-grekas
Copy link
Member Author

PR rebased, now using the word "topic" - it makes so much sense to me.

@sroze
Copy link
Contributor
sroze commented Oct 26, 2018

I think that I prefer messageName than topic 🤔

@nicolas-grekas
Copy link
Member Author

Honestly I don't get why: topic is much easier to explain/teach. It's just the correct word for what we need here. Thesaurus suggests "subject" as a synonym. Not sure it's better than topic.

@nicolas-grekas nicolas-grekas mentioned this pull request Oct 26, 2018
Closed
@Tobion
Copy link
Contributor
Tobion commented Oct 26, 2018 •
edited
Loading

I don't really like topic because it gives meaning to something whose meaning depends on what you put there. WIth topic it suggests you can have several topics like in rabbitmq topic exchanges topic1.topic2.topic3. Rabbitmq also provides a way to bind them individually using * and # placeholders. But that is not supported yet in symfony messenger routing. So that makes using several topics useless. And limiting to one topic does not fit the semantics of using the term topic.

IMHO, "address" is pushing the metaphor a bit too far.

Then we could just call it routingKey like in rabbitmq. I actually prefer that over address and messageName because it explains what it is used for, namely routing to the correct transport and handler.

@nicolas-grekas
Copy link
Member Author
nicolas-grekas commented Oct 26, 2018 •
edited
Loading

I think that's too bad that AMQP pollutes the selection of the proper English word. Ignoring it, "topic" is really what this is about to me. Like "the thing the message is about" - ie it's "topic". "Routing key" is going to be as hard to teach as "message name" to me - and is directly imported from AMQP context this time.

Personally, I wouldn't consider that the AMQP vocabularies should influence ours. What do others think? Maybe @weaverryan?

topic - subject - name - class - theme - etc

@fabpot
Copy link
Member
fabpot commented Oct 27, 2018

As I said to @sroze, I like the "topic" word, but I didn't think about the fact that this is used heavily in AMQP like @Tobion commented. As AMQP and the Messenger component are related, I think we need to find another word.

@nicolas-grekas
Copy link
Member Author
nicolas-grekas commented Oct 27, 2018 •
edited
Loading

Would using topic really be any confusing?
Here, we define the topic of the message.
Amqp uses the word to define what exchanges subscribe to.
That's exactly the same thing to me, just seen from the sender pov while amqp is from the receiver one.
Yes amqp allows subscribing to topics using patterns. But that's the same concept otherwise to me.

We could use $messageTopic / getMessageTopic() instead of just $topic / getTopic(), would that make it acceptable?
It's really hard to find a new name when you already found the correct English word...

@nicolas-grekas nicolas-grekas force-pushed the messenger-name branch 2 times, most recently from 7911c08 to c73189a Compare October 27, 2018 08:18
@nicolas-grekas
Copy link
Member Author
nicolas-grekas commented Oct 27, 2018 •
edited
Loading

PR updated to account for the new vocabulary in service configuration, with some cleanups found meanwhile. I suggest to proceed with the word "topic" - currently I'm blocked with 3 PRs waiting for this to be resolved. We can always change later.
And honestly, better call a cat "a cat" as we say in french: the word "topic" is the correct one. Any word we're going to find will be a more generic one (eg "name") with even more polysemy IRL. Not using topic because of AMQP is just reflecting we think in a bubble here... ;)

@vincenttouzet
Copy link
Contributor

I agree with @nicolas-grekas on the word "topic". In RabbitMQ the word is only used to define the type of the exchange. IMHO they could have named it router as it uses a routing key to routes messages to corresponding queues.

@Tobion
Copy link
Contributor
Tobion commented Oct 27, 2018 •
edited
Loading

Topic to me means a message can have several topics. It's similar to tags and allows to categorize things. But here it's just a string and not an array for some reason. And there is not way to match parts of the topic string currently.
So esp. with #28993 I don't see how the new way of categorizing messages (previously done with interfaces) is supposed to work now. @nicolas-grekas can you give an example how to configure this now? E.g. I want to route certain messages (that used to implement an interface) to be routed to a transport. But others not.

@nicolas-grekas
Copy link
Member Author

Closing: talking with Ryan, we found a way to use interfaces for topics. But not the current way, which is totally broken. I'm on a PR.

@Tobion Tobion mentioned this pull request Oct 31, 2018
Closed
@nicolas-grekas nicolas-grekas deleted the messenger-name branch March 3, 2020 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ro0NL ro0NL ro0NL approved these changes

@ogizanagi ogizanagi ogizanagi left review comments

@Tobion Tobion Tobion requested changes

@sroze sroze Awaiting requested review from sroze

Assignees
No one assigned
Labels
BC Break Feature Messenger Status: Needs Review
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
8 participants
@nicolas-grekas @ro0NL @Tobion @sroze @fabpot @vincenttouzet @ogizanagi @carsonbot
0