8000 [Messenger] remove interfaces/parent classes-based message subcription in favor of topic-based subscription by nicolas-grekas · Pull Request #28993 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Messenger] remove interfaces/parent classes-based message subcription in favor of topic-based subscription #28993

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 2 commits into from
Closed

[Messenger] remove interfaces/parent classes-based message subcription in favor of topic-based subscription #28993

wants to merge 2 commits into from

Conversation

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

Subscribing to messages based on their interfaces or on their parent classes is very fragile, for example:

  • two different messages with the same interfaces declared in a different order can route to different handlers/senders
  • adding an interface or a parent class on an existing message class can also change the routing destination
  • MessageSubscriberInterface::getHandledMessages() doesn't work when ChainHandler is involved - doing so correctly would require a much more dynamic logic to be correct (one based on a supports() method called each time on each handler and for each message.)

That's the same kind of issue that we had with type-based autowiring: bad DX, because changing a class declaration can lead to unrelated changes and unexpected side effects on the routing side.

The good news is that we have a nice alternative: topic-based dispatching (see 1st commit, coming from #28944 until it's merged.)

@nicolas-grekas nicolas-grekas requested a review from sroze as a code owner October 26, 2018 12:26
@nicolas-grekas nicolas-grekas added this to the 4.2 milestone Oct 26, 2018
@nicolas-grekas nicolas-grekas force-pushed the messenger-no-reflection branch from 2687710 to 4ac0b20 Compare October 26, 2018 12:26
Tobion
Tobion reviewed Oct 26, 2018
View reviewed changes
src/Symfony/Component/Messenger/Transport/Sender/Locator/ContainerSenderLocator.php Outdated
use Symfony\Component\Messenger\Transport\Sender\SenderInterface;

/**
* @author Samuel Roze <samuel.roze@gmail.com>
*/
class ContainerSenderLocator extends AbstractSenderLocator
class ContainerSenderLocator implements SenderLocatorInterface
{
private $senderServiceLocator;
private $messageToSenderIdMapping;
Copy link
Contributor

Choose a reason for hiding this comment

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

$topicToSenderIdMapping

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed thanks

@nicolas-grekas nicolas-grekas changed the title Messenger no reflection [Messenger] remove interfaces/parent classes-based message subcription in favor of topic-based subscription Oct 26, 2018
@nicolas-grekas nicolas-grekas force-pushed the messenger-no-reflection branch from 4ac0b20 to e96b126 Compare October 27, 2018 14:36
@nicolas-grekas nicolas-grekas force-pushed the messenger-no-reflection branch from e96b126 to f936764 Compare October 27, 2018 14:44
@Tobion Tobion mentioned this pull request Oct 27, 2018
Closed
@Tobion Tobion mentioned this pull request Oct 31, 2018
Closed
@nicolas-grekas nicolas-grekas deleted the messenger-no-reflection branch May 5, 2020 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tobion Tobion Tobion left review comments

@sroze sroze Awaiting requested review from sroze

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

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