[Messenger][DX] Uses a default receiver when only one is defined #27203

sroze · 2018-05-08T17:33:45Z

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

When using only one receiver, as a developer, it makes no sense for me to have to precise the receiver name when using the messenger:consume-messages command. This is the change:

- bin/console messenger:consume-messages default
+ bin/console messenger:consume-messages

If I have more than one transport configured, I'll get the following message:

You have 0 or more than one receiver (no default have been found). You need to specify the receiver name with an argument.

ogizanagi · 2018-05-08T17:47:57Z

src/Symfony/Component/Messenger/Command/ConsumeMessagesCommand.php

+        $receiverName = $input->getArgument('receiver');
+
+        if (empty($receiverName)) {
+            $receiverName = '.default';


What about injecting the default receiver name in the constructor instead (optional)?
If provided, use it as default value for the receiver arg and make it optional. If not, keep it mandatory. When using the component standalone, it'll make sense to define a default this way.
With the framework bundle, do only provide the default if there is a single receiver.

What about showing the list of receivers available and selecting one - if none were provided? thus we avoid double check on receiver names ;)

But that means also injecting the whole list of receiver names, as the ContainerInterface does not allow to get defined keys. So, should be optional IMHO.

What about injecting the default receiver name in the constructor instead (optional)?

Yes! Much better idea :)

What about showing the list of receivers available and selecting one

Shall we do that in another PR? I'm not 100% sure it makes sense to be honest so I'd prefer somebody else to work on that 😜

Fine for another PR. I'm sure @yceruto would be glad to do it. Otherwise I'll do :)

I've updated it based on your reviews, it's much better 😉

yceruto · 2018-05-08T17:53:33Z

src/Symfony/Component/Messenger/Command/ConsumeMessagesCommand.php

+        $receiverName = $input->getArgument('receiver');
+
+        if (empty($receiverName)) {
+            $receiverName = '.default';


You mean simply default? or the dot means something?

I think this is just trying to introduce a convention for defining the default service to use in the locator and not collide with a 'default' receiver. That wouldn't be necessary anymore with #27203 (comment)

ogizanagi · 2018-05-08T20:01:03Z

src/Symfony/Component/Messenger/Command/ConsumeMessagesCommand.php

@@ -54,7 +56,7 @@ protected function configure(): void
    {
        $this
            ->setDefinition(array(
-                new InputArgument('receiver', InputArgument::REQUIRED, 'Name of the receiver'),
+                new InputArgument('receiver', InputArgument::OPTIONAL, 'Name of the receiver'),


- new InputArgument('receiver', InputArgument::OPTIONAL, 'Name of the receiver'), + new InputArgument('receiver', $this->defaultReceiverId ? InputArgument::OPTIONAL : InputArgument:: REQUIRED, 'Name of the receiver', $this->defaultReceiverId),

and remove the changes in execute?

Even better.

ogizanagi · 2018-05-08T20:03:08Z

src/Symfony/Component/Messenger/Command/ConsumeMessagesCommand.php


-    public function __construct(MessageBusInterface $bus, ContainerInterface $receiverLocator, LoggerInterface $logger = null)
+    public function __construct(MessageBusInterface $bus, ContainerInterface $receiverLocator, LoggerInterface $logger = null, string $defaultReceiverId = null)


defaultReceiverName (or even just defaultReceiver)?
The argument description mentions a name.

ogizanagi · 2018-05-08T20:04:49Z

src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

@@ -171,6 +173,10 @@ private function registerReceivers(ContainerBuilder $container)
            }
        }

+        if (1 === \count($taggedReceivers) && $container->hasDefinition('console.command.messenger_consume_messages')) {
+            $container->getDefinition('console.command.messenger_consume_messages')->setArgument(3, (string) current($receiverMapping));


The (string) cast looks unnecessary to me?

We usually change the definition in xml files to define a placeholder for the argument with a comment and use replaceArgument instead of setArgument. To me, that's a good hint to known this argument is replaced by a pass or an extension when reading the sources.

You are right, I've changed it to use replaceArgument. Though, I ke 8000 pt the (string) to make the cast from Reference to string explicit.

yceruto · 2018-05-08T21:37:04Z

src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

-        foreach ($container->findTaggedServiceIds($this->receiverTag) as $id => $tags) {
+        $taggedReceivers = $container->findTaggedServiceIds($this->receiverTag);
+
+        foreach ($taggedReceivers as $id => $tags) {
            foreach ($tags as $tag) {
                $receiverMapping[$id] = new Reference($id);


It's unrelated to this PR, but I believe that this statement can be moved to the parent loop, isn't it?

I've updated it 😉

yceruto · 2018-05-09T10:03:45Z

src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

+        $taggedReceivers = $container->findTaggedServiceIds($this->receiverTag);
+
+        foreach ($taggedReceivers as $id => $tags) {
+            $receiverMapping[$id] = new Reference($id);


same in registerSenders() ;)

Changed 😉

jakzal

Well done 👍

… defined (sroze) This PR was squashed before being merged into the 4.1 branch (closes #27203). Discussion ---------- [Messenger][DX] Uses a default receiver when only one is defined | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | ø | License | MIT | Doc PR | ø When using only one receiver, as a developer, it makes no sense for me to have to precise the receiver name when using the `messenger:consume-messages` command. This is the change: ```patch - bin/console messenger:consume-messages default + bin/console messenger:consume-messages ``` If I have more than one transport configured, I'll get the following message: > > You have 0 or more than one receiver (no default have been found). You need to specify the receiver name with an argument. > Commits ------- 8315b86 [Messenger][DX] Uses a default receiver when only one is defined

…command configuration (yceruto) This PR was squashed before being merged into the 4.1 branch (closes #27224). Discussion ---------- [Messenger] Make sure default receiver name is set before command configuration | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27203 | License | MIT | Doc PR | - Otherwise the receiver's name would still be required always. https://github.com/symfony/symfony/blob/3cc4a701e6f24cb2eaaf1234b331f182dea0c9b4/src/Symfony/Component/Console/Command/Command.php#L77 Commits ------- 63871c9 [Messenger] Make sure default receiver name is set before command configuration

carsonbot added Status: Needs Review DX DX = Developer eXperience (anything that improves the experience of using Symfony) labels May 8, 2018

sroze added the Messenger label May 8, 2018

sroze force-pushed the uses-a-default-receiver-when-only-one-declared branch from 4a25ddc to df22cf0 Compare May 8, 2018 17:34

sroze added this to the 4.1 milestone May 8, 2018

ogizanagi reviewed May 8, 2018

View reviewed changes

yceruto reviewed May 8, 2018

View reviewed changes

sroze force-pushed the uses-a-default-receiver-when-only-one-declared branch 2 times, most recently from b15599b to c393792 Compare May 8, 2018 19:55

ogizanagi reviewed May 8, 2018

View reviewed changes

sroze force-pushed the uses-a-default-receiver-when-only-one-declared branch from c393792 to 61813f0 Compare May 8, 2018 20:27

yceruto reviewed May 8, 2018

View reviewed changes

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

ogizanagi approved these changes May 9, 2018

View reviewed changes

yceruto reviewed May 9, 2018

View reviewed changes

sroze force-pushed the uses-a-default-receiver-when-only-one-declared branch 2 times, most recently from 68fa326 to 0b63eb4 Compare May 9, 2018 11:51

jakzal approved these changes May 9, 2018

View reviewed changes

[Messenger][DX] Uses a default receiver when only one is defined

8315b86

sroze force-pushed the uses-a-default-receiver-when-only-one-declared branch from 0b63eb4 to 8315b86 Compare DA00 May 9, 2018 14:42

sroze merged commit 8315b86 into symfony:4.1 May 9, 2018

sroze deleted the uses-a-default-receiver-when-only-one-declared branch May 9, 2018 14:43

yceruto mentioned this pull request May 10, 2018

[Messenger] Make sure default receiver name is set before command configuration #27224

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Messenger][DX] Uses a default receiver when only one is defined #27203

[Messenger][DX] Uses a default receiver when only one is defined #27203

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!


		public function __construct(MessageBusInterface $bus, ContainerInterface $receiverLocator, LoggerInterface $logger = null)
		public function __construct(MessageBusInterface $bus, ContainerInterface $receiverLocator, LoggerInterface $logger = null, string $defaultReceiverId = null)

Uh oh!

[Messenger][DX] Uses a default receiver when only one is defined #27203

[Messenger][DX] Uses a default receiver when only one is defined #27203

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!