8000 [Messenger] allow to change the message name by Tobion · Pull Request #29045 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Messenger] allow to change the message name #29045

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 to change the message name #29045

wants to merge 1 commit into from

Conversation

Tobion
Copy link
Contributor
@Tobion Tobion commented Oct 31, 2018 •
edited
Loading
Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Having the class name as default for the message name is the best practice and usually does not need to be changed. But I think we need to provide a way to do so for just for flexibility. This allows people to opt-out of interface/parent class based routing (#28993) by manually calling $bus->dispatch(Envelope::createNamed($message, 'foo')). IMO we don't need to add an optional name parameter to the dispatch method like originally proposed in #28944 since it's an edge case that does not need to be exposed at the primary entry point.

@Tobion Tobion requested a review from sroze as a code owner October 31, 2018 16:58
@nicolas-grekas nicolas-grekas added this to the next milestone Oct 31, 2018
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Oct 31, 2018 •
edited
Loading

The serializing part loses the name - that's why I added it as a stamp in a previous PR.
Also we should define how we expect senders/handlersLocator to behave when a message is passed.
Should they still look at parent classes? etc.
Opting-out could also be seen as a decision that belongs to these locators btw. We should think about the responsibility of the different parts here.

@Tobion
Copy link
Contributor Author
Tobion commented Nov 1, 2018 •
edited
Loading

Let's say there is a message with arbitrary data and you want to route to handlers based on the data of the message.
Currently the routing is solely based on the message name (which is fixed to the class). So you cannot manually implement such a scenario at all. This to me is the problem with HandlersLocatorInterface::getHandlers(string $name) that removes alot of possibilities. If we change it back to HandlersLocatorInterface::getHandlers(Envelope $envelope), then we probably don't even need to make the message name adjustable, as people could just implement their own locator with custom logic. One such custom locators could also be to skip parent/interface based routing.

@nicolas-grekas nicolas-grekas mentioned this pull request Nov 1, 2018
Merged
@skalpa
Copy link
Contributor
skalpa commented Nov 1, 2018

+1 to changing the interface back so it accepts an Envelope $envelope. I need to be able to route the messages sent to my events bus according to their content, and the argument signature change prevents me from doing that now.

As for responsibilities, this is how I see things:

  • HandlersLocatorInterface associates handlers to messages. How it does that is the implementation responsibility. Some implementations may not use classes at all for this.
  • HandlersLocator is only an implementation of the interface, that uses the classes/interfaces of the messages to lookup handlers. It could be renamed ClassBasedHandlersLocator or TypeBasedHandlersLocator to make this clearer. Its behavior should be clearly defined, but that's already the case imho.
  • I am not against adding a name-based locator implementation by default, but then it should be implemented in a new NameBasedHandlerLocator that doesn't use parent classes or interfaces at all. In that case we may need a way to customize the message name during dispatch (ie: a stamp), and a ChainHandlerLocator to keep the current behavior (so both name-based and class-based resolutions are enabled by default).

@nicolas-grekas
Copy link
Member

@skalpa see #29052 ;)

symfony-splitter pushed a commit to symfony/messenger that referenced this pull request Nov 1, 2018
@Tobion
minor #29052 [Messenger] make senders/handlers locator accept envelop…
d3509f2 
…es (nicolas-grekas)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[Messenger] make senders/handlers locator accept envelopes

| Q             | A
| ------------- | ---
| Branch?       | 4.2
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

As suggested by @Tobion in symfony/symfony#29045 (comment) - works for me also.

Commits
-------

9cd88b0dc5 [Messenger] make senders/handlers locator accept envelopes
Tobion added a commit that referenced this pull request Nov 1, 2018
@Tobion
minor #29052 [Messenger] make senders/handlers locator accept envelop…
2bfcaeb 
…es (nicolas-grekas)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[Messenger] make senders/handlers locator accept envelopes

| Q             | A
| ------------- | ---
| Branch?       | 4.2
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

As suggested by @Tobion in #29045 (comment) - works for me also.

Commits
-------

9cd88b0 [Messenger] make senders/handlers locator accept envelopes
@Tobion
Copy link
Contributor Author
Tobion commented Nov 1, 2018

Replaced by #29052. The rest can be implemented manually or in future symfony versions.

@Tobion Tobion closed this Nov 1, 2018
@Tobion Tobion deleted the allow-change-message-name branch November 1, 2018 13:38
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018
@Tobion Tobion mentioned this pull request Feb 8, 2019
Closed
symfony-splitter pushed a commit to symfony/messenger that referenced this pull request Jan 28, 2020
@Tobion
minor #29052 [Messenger] make senders/handlers locator accept envelop…
5106028 
…es (nicolas-grekas)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[Messenger] make senders/handlers locator accept envelopes

| Q             | A
| ------------- | ---
| Branch?       | 4.2
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

As suggested by @Tobion in symfony/symfony#29045 (comment) - works for me also.

Commits
-------

9cd88b0dc5 [Messenger] make senders/handlers locator accept envelopes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sroze sroze Awaiting requested review from sroze

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

Successfully merging this pull request may close these issues.
4 participants
@Tobion @nicolas-grekas @skalpa @carsonbot
0