-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Messenger][DX] Uses a default receiver when only one is defined #27203
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.
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
[Messenger][DX] Uses a default receiver when only one is defined #27203
Conversation
4a25ddc
to
df22cf0
Compare
$receiverName = $input->getArgument('receiver'); | ||
|
||
if (empty($receiverName)) { | ||
$receiverName = '.default'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about showing the list of receivers available and selecting one - if none were provided? thus we avoid double check on receiver names ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about injecting the default receiver name in the constructor instead (optional)?
Yes! Much better idea :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😜
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine for another PR. I'm sure @yceruto would be glad to do it. Otherwise I'll do :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated it based on your reviews, it's much better 😉
$receiverName = $input->getArgument('receiver'); | ||
|
||
if (empty($receiverName)) { | ||
$receiverName = '.default'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean simply default
? or the dot means something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
b15599b
to
c393792
Compare
@@ -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'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even better.
|
||
public function __construct(MessageBusInterface $bus, ContainerInterface $receiverLocator, LoggerInterface $logger = null) | ||
public function __construct(MessageBusInterface $bus, ContainerInterface $receiverLocator, LoggerInterface $logger = null, string $defaultReceiverId = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaultReceiverName
(or even just defaultReceiver
)?
The argument description mentions a name.
@@ -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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I've changed it to use replaceArgument
. Though, I kept the (string)
to make the cast from Reference
to string
explicit.
c393792
to
61813f0
Compare
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unrelated to this PR, but I believe that this statement can be moved to the parent loop, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated it 😉
$taggedReceivers = $container->findTaggedServiceIds($this->receiverTag); | ||
|
||
foreach ($taggedReceivers as $id => $tags) { | ||
$receiverMapping[$id] = new Reference($id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displ A3E2 ayed to describe this comment to others. Learn more.
same in registerSenders()
;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed 😉
68fa326
to
0b63eb4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done 👍
0b63eb4
to
8315b86
Compare
… 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
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:If I have more than one transport configured, I'll get the following message: