8000 [Messenger][DX] Uses a default receiver when only one is defined by sroze · Pull Request #27203 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[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

Merged

Conversation

sroze
Copy link
Contributor
@sroze sroze commented May 8, 2018
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.

@carsonbot carsonbot added Status: Needs Review DX DX = Developer eXperience (anything that improves the experience of using Symfony) labels May 8, 2018
@sroze sroze added the Messenger label May 8, 2018
@sroze sroze force-pushed the uses-a-default-receiver-when-only-one-declared branch from 4a25ddc to df22cf0 Compare May 8, 2018 17:34
@sroze sroze added this to the 4.1 milestone May 8, 2018
$receiverName = $input->getArgument('receiver');

if (empty($receiverName)) {
$receiverName = '.default';
Copy link
Contributor
@ogizanagi ogizanagi May 8, 2018

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.

Copy link
Member
@yceruto yceruto May 8, 2018

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 ;)

Copy link
Contributor

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.

Copy link
Contributor Author

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 :)

Copy link
Contributor Author

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 😜

Copy link
Contributor

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 :)

Copy link
Contributor Author

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';
Copy link
Member

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?

Copy link
Contributor

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)

@sroze 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
@@ -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'),
Copy link
Contributor

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?

Copy link
Contributor Author

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)
Copy link
Contributor

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));
Copy link
Contributor

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.

Copy link
Contributor Author

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.

8000

@sroze sroze force-pushed the uses-a-default-receiver-when-only-one-declared branch from c393792 to 61813f0 Compare May 8, 2018 20:27
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);
Copy link
Member
@yceruto yceruto May 8, 2018

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?

Copy link
Contributor Author

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);
Copy link
Member
@yceruto yceruto May 9, 2018

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() ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed 😉

@sroze 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
Copy link
Contributor
@jakzal jakzal left a comment

Choose a reason for hiding this comment

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

Well done 👍

@sroze sroze force-pushed the uses-a-default-receiver-when-only-one-declared branch from 0b63eb4 to 8315b86 Compare May 9, 2018 14:42
@sroze sroze merged commit 8315b86 into symfony:4.1 May 9, 2018
sroze added a commit that referenced this pull request May 9, 2018
… 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
@sroze sroze deleted the uses-a-default-receiver-when-only-one-declared branch May 9, 2018 14:43
sroze added a commit that referenced this pull request May 11, 2018
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Messenger Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0