[Messenger] implement several senders using a ChainSender #27002

Tobion · 2018-04-21T19:31:09Z

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

sroze · 2018-04-22T10:22:14Z

What's the use-case for such change?

Tobion · 2018-04-22T17:34:38Z

Actually I think returning several senders does not make sense. It should be public function getSenderForMessage($message): ?SenderInterface; If multiple senders are wanted, just return a ChainSender implementation. And the forwardToSenderAndHandler logic should not depend on some arbitrary null value that violates the interface anyway. It could just be a marker interface implemented by the sender. Or an additional method in the SenderInterface like forwardToHandler: bool

sroze · 2018-04-22T17:41:35Z

I appreciate and agree that this isn't the "cleanest" solution but it has the benefit of being simple and working well 🙃I see the point of the ChainSender but I don't really get the "marker interface implemented by the sender". What do you mean? How would you configure this on the framework.messenger configuration?

Tobion · 2018-04-25T22:21:32Z

Something like where handle defaults to false.

framework:
    messenger:
        routing:
            'My\Message\ToBeSentToTwoSenders': 
                senders: [amqp, audit]
                handle: bool

nicolas-grekas · 2018-04-29T06:35:04Z

So, shouldn't we change the interface as @Tobion suggests, allowing only one sender?
The configuration can be kept the same, creating a chained sender when needed.

sroze · 2018-04-30T16:48:37Z

@Tobion are you willing to make this change? I'd definitly approve it.

Tobion · 2018-04-30T19:47:58Z

I try to get to it tomorrow

Needs work

sroze · 2018-05-03T13:14:30Z

Status: Needs work

Tobion · 2018-05-06T20:06:31Z

I implemented the single sender using a ChainSender approach.

ogizanagi

I've rebased it after the merge of #26945

ogizanagi · 2018-05-07T14:10:54Z

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

+     *
+     * @return bool
+     */
+    public function forwardToSenderAndHandler($message): bool;


Isn't this method name a bit misleading? I didn't expected it to be as "isser" at first, but something performing an action.
Shouldn't it be something like mustForwardToSenderAndHandler (or better)?

Yeah makes sense to change it.

sroze

Except the SenderInterface changed, I'm 👍

sroze · 2018-05-08T15:38:18Z

src/Symfony/Component/Messenger/Transport/ChainSender.php

+    /**
+     * {@inheritdoc}
+     */
+    public function send($message)


This now has the : void typehint, to be changed.

sroze

Adding the "Request changes" review explicitly until the SenderInterface contract is changed.

sroze · 2018-05-09T12:14:35Z

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

        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'];
+            if (1 < count($messageConfiguration['senders'])) {


sroze · 2018-05-09T18:56:23Z

@Tobion, unfortunately, you are going to have to rebase. Last step 🤞

yceruto · 2018-05-16T14:24:48Z

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

+    /**
+     * @internal
+     */
+    public static function getValueFromMessageRouting(array $mapping, $message)


: ?array at end?

that would not be correct. we cant typehint it.

I meant the return typehint, we can't?

Because that function is used for two use cases: return strings and bools. (it does not return an array anymore, that's the point of this PR :))

sroze · 2018-05-17T07:11:40Z

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

+    /**
+     * @internal
+     */
+    public static function getValueFromMessageRouting(array $mapping, $message)


Because that function is used for two use cases: return strings and bools. (it does not return an array anymore, that's the point of this PR :))

sroze · 2018-05-17T07:13:12Z

Thank you @Tobion.

…r (Tobion) This PR was merged into the 4.1 branch. Discussion ---------- [Messenger] implement several senders using a ChainSender | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | | License | MIT | Doc PR | Commits ------- 198925e [Messenger] implement several senders using a ChainSender

@xabbuh

… (ogizanagi) This PR was merged into the 4.1 branch. Discussion ---------- [Messenger] Fix messenger.routing send_and_handle config Ref: symfony/symfony#27002 Credits to @xabbuh who found this doc was outdated. Commits ------- 9b91634 [Messenger] Fix messenger.routing send_and_handle config

carsonbot added Status: Needs Review Feature labels Apr 21, 2018

Tobion added this to the 4.1 milestone Apr 21, 2018

sroze added the Messenger label Apr 22, 2018

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 27, 2018

sroze previously approved these changes Apr 27, 2018

View reviewed changes

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

Tobion force-pushed the support-iterable-messenger-sender-locator branch from f24671e to 3935548 Compare May 6, 2018 20:03

Tobion changed the title ~~[Messenger] support iterable in sender locator~~ [Messenger] implement several senders using a ChainSender May 6, 2018

Tobion force-pushed the support-iterable-messenger-sender-locator branch from 3935548 to 3d0eb0a Compare May 7, 2018 09:05

Tobion added Status: Needs Review and removed Status: Needs Work labels May 7, 2018

Tobion assigned sroze May 7, 2018

ogizanagi force-pushed the support-iterable-messenger-sender-locator branch 2 times, most recently from b8316cf to dc9bc95 Compare May 7, 2018 14:08

ogizanagi reviewed May 7, 2018

View reviewed changes

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

Tobion force-pushed the support-iterable-messenger-sender-locator branch 3 times, most recently from a0653d1 to 0d430b8 Compare May 8, 2018 14:58

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

sroze approved these changes May 8, 2018

View reviewed changes

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

sroze suggested changes May 9, 2018

View reviewed changes

Tobion force-pushed the support-iterable-messenger-sender-locator branch from 0d430b8 to a1d2dfe Compare May 9, 2018 12:00

sroze reviewed May 9, 2018

View reviewed changes

Tobion force-pushed the support-iterable-messenger-sender-locator branch from a1d2dfe to 1966cd7 Compare May 9, 2018 16:21

Tobion force-pushed the support-iterable-messenger-sender-locator branch 2 times, most recently from 78b1b71 to ddc0c3b Compare May 11, 2018 13:00

ogizanagi mentioned this pull request May 15, 2018

some tweaks for the Messenger component docs symfony/symfony-docs#9771

Merged

[Messenger] implement several senders using a ChainSender

198925e

Tobion force-pushed the support-iterable-messenger-sender-locator branch from ddc0c3b to 198925e Compare May 16, 2018 13:12

nicolas-grekas approved these changes May 16, 2018

View reviewed changes

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

yceruto reviewed May 16, 2018

View reviewed changes

sroze approved these changes May 17, 2018

View reviewed changes

sroze merged commit 198925e into symfony:4.1 May 17, 2018

Tobion deleted the support-iterable-messenger-sender-locator branch May 17, 2018 09:34

ogizanagi mentioned this pull request May 22, 2018

[Messenger] Allow defining an empty sender service #27335

Closed

ogizanagi mentioned this pull request Oct 11, 2018

[Messenger] Fix messenger.routing send_and_handle config symfony/symfony-docs#10473

Merged

Uh oh!

[Messenger] implement several senders using a ChainSender #27002

[Messenger] implement several senders using a ChainSender #27002

Uh oh!

Conversation

Tobion commented Apr 21, 2018

Uh oh!

sroze commented Apr 22, 2018

Uh oh!

Tobion commented Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sroze commented Apr 22, 2018

Uh oh!

Tobion commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Apr 29, 2018

Uh oh!

sroze commented Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tobion commented Apr 30, 2018

Uh oh!

sroze commented May 3, 2018

Uh oh!

Tobion commented May 6, 2018

Uh oh!

ogizanagi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sroze left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sroze left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sroze commented May 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sroze commented May 17, 2018

Uh oh!

Uh oh!

Tobion commented Apr 22, 2018 •

edited

Loading

Tobion commented Apr 25, 2018 •

edited

Loading

sroze commented Apr 30, 2018 •

edited

Loading