Skip to content
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] Select alternatives on missing receiver arg or typo #27230

Merged
merged 1 commit into from May 17, 2018

Conversation

Projects
None yet
6 participants
@yceruto
Copy link
Contributor

commented May 10, 2018

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

If there is more than one receiver available - bin/console messenger:consume-messages:
messenger_consume_missing

If typo and there is similarities - bin/console messenger:consume-messages amq:
messenger_consume_typo

requires #27224

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

This comment has been minimized.

Copy link
@ogizanagi

ogizanagi May 11, 2018

Member

I think keeping the defaultReceiver argument would be better than arbitrary selecting the first one.

This comment has been minimized.

Copy link
@yceruto

yceruto May 11, 2018

Author Contributor

As we don't have default receiver by definition I'm selecting the first one and the only one when there is just one receiver... but I guess out-of-the-box could makes sense set a default one among several... still worth it?

This comment has been minimized.

Copy link
@ogizanagi

ogizanagi May 11, 2018

Member

I think we should never select a default ourself if many receivers are available. Within the fwb, this means just exposing the whole list except when there is only one receiver.
Without, it's the developper responsibility to set one.
I feel like selecting arbitrary the first receiver is against DX.

(but, interactively in the command, the choice question could have a default, as it still requires validating it)

This comment has been minimized.

Copy link
@sroze

sroze May 11, 2018

Member

But as far as I understand the code now, it only selects the first one only if there is only one receiver, so that works well.

This comment has been minimized.

Copy link
@ogizanagi

ogizanagi May 11, 2018

Member

😅 good point. I missed that.
I guess everything is fine then 😛

This comment has been minimized.

Copy link
@yceruto
@@ -183,8 +182,12 @@ private function registerReceivers(ContainerBuilder $container)
}
}
if (1 === \count($taggedReceivers) && $container->hasDefinition('console.command.messenger_consume_messages')) {
$container->getDefinition('console.command.messenger_consume_messages')->replaceArgument(3, (string) current($receiverMapping));
if ($container->hasDefinition('console.command.messenger_consume_messages')) {

This comment has been minimized.

Copy link
@ogizanagi

ogizanagi May 11, 2018

Member

Relates to above, I think we should only have the default defined if there is just one like in removed lines (but still inject all names independently for interactivity)

@yceruto yceruto force-pushed the yceruto:messenger_consume_cmd branch from 5f702be to 4c98ba8 May 11, 2018

@yceruto

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2018

Rebased and ready.

@yceruto yceruto force-pushed the yceruto:messenger_consume_cmd branch from 4c98ba8 to 54c2541 May 11, 2018

@sroze

This comment has been minimized.

Copy link
Member

commented May 11, 2018

@ogizanagi

This comment has been minimized.

Copy link
Member

commented May 11, 2018

Is that really an issue?
I think the best practice for scripts is to always use --no-interaction and define all arguments.

@yceruto

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2018

I agree completely with @ogizanagi.

It's really useful only for development. In production you must always configure the receiver argument without interaction.

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

@yceruto

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

What about to disable the interaction mode on debug false ?

@sroze

This comment has been minimized.

Copy link
Member

commented May 14, 2018

@ogizanagi

This comment has been minimized.

Copy link
Member

commented May 14, 2018

That would still not solve the case the dev adds a receiver where previously there was only one and the script calls the command without explicit arguments (hence fails). So, also make the argument required if no debug?
Then it's probably semantically more correct to inject a default in the constructor rather than a $debug argument?
Disabling interaction on debug false isn't really something we do.
Again, do we really care?

@sroze sroze added the DX label May 14, 2018

@sroze

sroze approved these changes May 14, 2018

@@ -134,4 +158,21 @@ private function convertToBytes(string $memoryLimit): int
return $max;
}
private function findAlternatives($name, array $collection)

This comment has been minimized.

Copy link
@sroze

sroze May 14, 2018

Member

Isn't this already somewhere else? Can't we share instead of copy? Maybe at least reference the other occurrences if it has been pasted so we know when maintaining it?

This comment has been minimized.

Copy link
@yceruto

yceruto May 14, 2018

Author Contributor

Yes, from debug command of the Form component, but still there is other places where similar procedure has been used, probably a trait could be created for such cases.

This comment has been minimized.

Copy link
@fabpot

fabpot May 15, 2018

Member

We don"t have a central place for such logic for 2 reasons: it's small enough and we don't want to create a "util" component.

This comment has been minimized.

Copy link
@sroze

sroze May 17, 2018

Member

@fabpot a good practice can be to add a comment that refers to the place it was copy/pasted from.

@sroze

This comment has been minimized.

Copy link
Member

commented May 17, 2018

Thank you @yceruto.

@sroze sroze merged commit 54c2541 into symfony:4.1 May 17, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

sroze added a commit that referenced this pull request May 17, 2018

feature #27230 [Messenger] Select alternatives on missing receiver ar…
…g or typo (yceruto)

This PR was merged into the 4.1 branch.

Discussion
----------

[Messenger] Select alternatives on missing receiver arg or typo

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

If there is more than one receiver available - `bin/console messenger:consume-messages`:
![messenger_consume_missing](https://user-images.githubusercontent.com/2028198/39895594-d4f652c6-5478-11e8-87cf-a1e08e681320.png)

If typo and there is similarities - `bin/console messenger:consume-messages amq`:
![messenger_consume_typo](https://user-images.githubusercontent.com/2028198/39895599-d9f0705e-5478-11e8-9d6b-59d62930d4cb.png)

requires #27224

Commits
-------

54c2541 Select alternatives on missing receiver arg or typo

@yceruto yceruto deleted the yceruto:messenger_consume_cmd branch May 17, 2018

@fabpot fabpot referenced this pull request May 21, 2018

Merged

Release v4.1.0-BETA2 #27331

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.