[Messenger] Fix return senders based on the message parents/interfaces #27184

yceruto · 2018-05-07T17:50:21Z

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

symfony/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

Lines 1494 to 1499 in c3d4536

    
           foreach ($config['routing'] as $message => $messageConfiguration) { 
        
               if ('*' !== $message && !class_exists($message) && !interface_exists($message, false)) { 
        
                   throw new LogicException(sprintf('Messenger routing configuration contains a mistake: message "%s" does not exist. It needs to match an existing class or interface.', $message)); 
        
               } 
        
               $messageToSenderIdsMapping[$message] = $messageConfiguration['senders'];

According to the code a message interface is supported into routing configuration, but it doesn't work when SendMessageMiddleware gets the mapping senders for the current object message.

This PR tries to fix it.

nicolas-grekas · 2018-05-07T18:21:27Z

I'm not sure this is a good idea, is it?
This is like by-id vs by-type autowiring.
Messages are VO objects, their interface are mostly meaningless to me.

kbond · 2018-05-07T18:27:38Z

I foresee it being a burden to declare all the messages you want sent in your config. This would alleviate that.

nicolas-grekas · 2018-05-07T18:33:10Z

But this makes it impossible to opt-out from a handler while implementing an interface or extending a base message class. This feels a lot like global interface-based configuration, which is a no-go.
Creating a new message class should require one to explicitly tell where it should be handled IMHO.

kbond · 2018-05-07T18:44:02Z

Yeah, I understand what you're saying. I have thought of alternative solutions that would work within the current implementation.

ogizanagi · 2018-05-07T18:47:19Z

But this makes it impossible to opt-out from a handler

That's for senders, not handlers. Also you can opt-out by specifying the specific message class on the routing configuration as classes take precedence over interfaces in SenderLocator.

nicolas-grekas · 2018-05-07T18:49:08Z

~~Putting a 👎 here to prevent merging unless the discussion resolves the issue.~~

~~@ogizanagi this is still doing interface-based configuration. It's nice that there is a way to work around it, but that shouldn't be needed at all IMHO. It should be the other way around.~~

waiting decision

yceruto · 2018-05-07T19:14:12Z

Is the same issue blocker applicable to wildcard '*' option too?

yceruto · 2018-05-07T19:29:12Z

For the record, this is what it would allows:

framework:
    messenger:
        transports:
            amqp: ...
            aws: ...
        routing:
            App\Message\AMQPMessageInterface: amqp
            App\Message\AWSMessageInterface: aws

from here, all my messages that need to be send through amqp transport will implement AMQPMessageInterface and so on. Otherwise, I must configure each new message class within the routing configuration.

yceruto · 2018-05-07T19:46:16Z

But I understand what @nicolas-grekas says too and probably this goal should be reached in another way around...

nicolas-grekas · 2018-05-07T20:49:11Z

OK, I get it better now. I'm removing my -1 above, this may actually be a good idea: this is configuration-based already and senders are not bound by messages' interfaces. The issue here is finding which sender to select for a given message, and when no sender is explicitly defined, falling back to interfaces then * should be OK. Sorry for the misunderstanding.

I then have a question: what about parent classes? e.g VarDumper's AbstractCloner checks them before:
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/VarDumper/Cloner/AbstractCloner.php#L273

Don't we want to do the same here?

yceruto · 2018-05-07T22:15:36Z

Parent classes supports added.

nicolas-grekas · 2018-05-07T22:35:59Z

We should be clear that this is a fallback: when no sender exists for a given class, we check the parents. One could expect that when a sender is registered for a given interface, it gets all the messages from that type even if a class-specific sender is also configured. Correct ?

App\Message\AMQPMessageInterface: amqp

Note that I feel like this kind of dispatching should be avoided: messages should be independent of the sender that may handle them, isn't it? The interface should be semantic instead, allowing to label message by "kinds", and not by infrastructure component.

yceruto · 2018-05-07T23:58:18Z

We should be clear that this is a fallback: when no sender exists for a given class, we check the parents.

Indeed...

One could expect that when a sender is registered for a given interface, it gets all the messages from that type even if a class-specific sender is also configured. Correct ?

Sorry I'm not sure to understand your statement very well... when a sender is registered for a given interface it's used to send all messages that implement that interface unless exists a class-specific sender also configured for the same message, which probably should be a different sender and the message shouldn't implement that interface. Anyway this is the current order of priority: 'class|parents|interfaces|*': sender_name.

Note that I feel like this kind of dispatching should be avoided: messages should be independent of the sender that may handle them, isn't it?

It's just an example, but I guess it depeds of how many senders you have and how many senders are handling the same kind of messages.

sroze · 2018-05-08T10:10:05Z

src/Symfony/Component/Messenger/Tests/Asynchronous/Routing/SenderLocatorTest.php

+        $container->set('my_amqp_sender', $sender);
+
+        $locator = new SenderLocator($container, array(
+            DummyMessage::class => array(


Can you configure it here so that the interface is the first argument of the locator to prove that the parent class takes precedence on the interface?

sroze · 2018-05-08T10:10:42Z

src/Symfony/Component/Messenger/Asynchronous/Routing/SenderLocator.php

+
+    private function getSenderIds($message): array
+    {
+        $senderIds = array();


I'm not sure you need that variable anymore, you can use returns directly.

sroze · 2018-05-08T10:13:37Z

src/Symfony/Component/Messenger/Tests/Asynchronous/Routing/SenderLocatorTest.php

+            ),
+        ));
+
+        $this->assertEquals(array($sender), $locator->getSendersForMessage(new ChildDummyMessage('Hello', array('name' => 'World'))));


assertSame for all the occurrences of assertEquals

sroze · 2018-05-08T10:14:14Z

src/Symfony/Component/Messenger/Tests/Asynchronous/Routing/SenderLocatorTest.php

+
+class ChildDummyMessage extends DummyMessage
+{
+    private $extra;


I don't see you using that extra so I guess it can be removed.

Removed extra code.

yceruto · 2018-05-08T11:27:01Z

Status: Needs Review

sroze

Looks good to me 👍

yceruto · 2018-05-09T19:18:03Z

Rebased.

sroze · 2018-05-10T15:06:54Z

Thank you @yceruto.

…s/interfaces (yceruto) This PR was merged into the 4.1 branch. Discussion ---------- [Messenger] Fix return senders based on the message parents/interfaces | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - https://github.com/symfony/symfony/blob/c3d4536203981d425adc47552c553a9633186a5e/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L1494-L1499 According to the code a message interface is supported into routing configuration, but it doesn't work when `SendMessageMiddleware` gets the mapping senders for the current object message. This PR tries to fix it. Commits ------- 41e25ab Fixed return senders based on the message parents/interfaces

carsonbot added Status: Needs Review Bug labels May 7, 2018

ogizanagi previously approved these changes May 7, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels May 7, 2018

nicolas-grekas added this to the 4.1 milestone May 7, 2018

yceruto force-pushed the messenger_support_message_interface branch from 966ebb1 to 0ac140a Compare May 7, 2018 22:13

yceruto force-pushed the messenger_support_message_interface branch from 0ac140a to 8b8100c Compare May 7, 2018 22:25

yceruto changed the title ~~[Messenger] Fix return senders based on the message interface~~ [Messenger] Fix return senders based on the message parents/interfaces May 7, 2018

sroze added the Messenger label May 8, 2018

sroze suggested changes May 8, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels May 8, 2018

yceruto force-pushed the messenger_support_message_interface branch 2 times, most recently from 12c0f65 to 88375f5 Compare May 8, 2018 11:19

carsonbot added Status: Needs Review and removed Status: Needs Work labels May 8, 2018

kbond mentioned this pull request May 8, 2018

[Messenger] Configure a handler to subscribe to messages by their interface/parent class #27076

Closed

sroze approved these changes May 8, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels May 8, 2018

Fixed return senders based on the message parents/interfaces

41e25ab

yceruto force-pushed the messenger_support_message_interface branch from 88375f5 to 41e25ab Compare May 9, 2018 19:16

sroze requested a review from ogizanagi May 10, 2018 08:24

carsonbot added Status: Needs Review and removed Status: Reviewed Status: Needs Review labels May 10, 2018

ogizanagi approved these changes May 10, 2018

View reviewed changes

carsonbot added the Status: Reviewed label May 10, 2018

sroze merged commit 41e25ab into symfony:4.1 May 10, 2018

yceruto deleted the messenger_support_message_interface branch May 10, 2018 15:17

fabpot mentioned this pull request May 21, 2018

Release v4.1.0-BETA2 #27331

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Messenger] Fix return senders based on the message parents/interfaces #27184

[Messenger] Fix return senders based on the message parents/interfaces #27184

yceruto commented May 7, 2018 •

edited

Loading

nicolas-grekas commented May 7, 2018

kbond commented May 7, 2018

nicolas-grekas commented May 7, 2018

kbond commented May 7, 2018

ogizanagi commented May 7, 2018

nicolas-grekas commented May 7, 2018 •

edited

Loading

yceruto commented May 7, 2018 •

edited

Loading

yceruto commented May 7, 2018 •

edited

Loading

yceruto commented May 7, 2018 •

edited

Loading

nicolas-grekas commented May 7, 2018 •

edited

Loading

yceruto commented May 7, 2018

nicolas-grekas commented May 7, 2018

yceruto commented May 7, 2018 •

edited

Loading

sroze May 8, 2018

yceruto May 8, 2018

sroze May 8, 2018

yceruto May 8, 2018

sroze May 8, 2018

yceruto May 8, 2018

sroze May 8, 2018

yceruto May 8, 2018

yceruto commented May 8, 2018

sroze left a comment

yceruto commented May 9, 2018

sroze commented May 10, 2018

	foreach ($config['routing'] as $message => $messageConfiguration) {
	if ('*' !== $message && !class_exists($message) && !interface_exists($message, false)) {
	throw new LogicException(sprintf('Messenger routing configuration contains a mistake: message "%s" does not exist. It needs to match an existing class or interface.', $message));
	}

	$messageToSenderIdsMapping[$message] = $messageConfiguration['senders'];

[Messenger] Fix return senders based on the message parents/interfaces #27184

[Messenger] Fix return senders based on the message parents/interfaces #27184

Conversation

yceruto commented May 7, 2018 • edited Loading

nicolas-grekas commented May 7, 2018

kbond commented May 7, 2018

nicolas-grekas commented May 7, 2018

kbond commented May 7, 2018

ogizanagi commented May 7, 2018

nicolas-grekas commented May 7, 2018 • edited Loading

yceruto commented May 7, 2018 • edited Loading

yceruto commented May 7, 2018 • edited Loading

yceruto commented May 7, 2018 • edited Loading

nicolas-grekas commented May 7, 2018 • edited Loading

yceruto commented May 7, 2018

nicolas-grekas commented May 7, 2018

yceruto commented May 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto commented May 8, 2018

sroze left a comment

Choose a reason for hiding this comment

yceruto commented May 9, 2018

sroze commented May 10, 2018

yceruto commented May 7, 2018 •

edited

Loading

nicolas-grekas commented May 7, 2018 •

edited

Loading

yceruto commented May 7, 2018 •

edited

Loading

yceruto commented May 7, 2018 •

edited

Loading

yceruto commented May 7, 2018 •

edited

Loading

nicolas-grekas commented May 7, 2018 •

edited

Loading

yceruto commented May 7, 2018 •

edited

Loading