Skip to content

[Messenger] Select alternatives on missing receiver arg or typo #27230

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.

Sign up for GitHub

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
merged 1 commit into from
May 17, 2018
Merged

[Messenger] Select alternatives on missing receiver arg or typo #27230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 45 additions & 4 deletions src/Symfony/Component/Messenger/Command/ConsumeMessagesCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Input\InputOption;
use Symfony\Component\Console\Output\OutputInterface;
use Symfony\Component\Console\Style\SymfonyStyle;
use Symfony\Component\Messenger\MessageBusInterface;
use Symfony\Component\Messenger\Transport\Enhancers\StopWhenMemoryUsageIsExceededReceiver;
use Symfony\Component\Messenger\Transport\Enhancers\StopWhenMessageCountIsExceededReceiver;
Expand All @@ -37,14 +38,14 @@ class ConsumeMessagesCommand extends Command
private $bus;
private $receiverLocator;
private $logger;
private $defaultReceiverName;
private $receiverNames;

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 = array())
{
$this->bus = $bus;
$this->receiverLocator = $receiverLocator;
$this->logger = $logger;
$this->defaultReceiverName = $defaultReceiverName;
$this->receiverNames = $receiverNames;

parent::__construct();
}
Expand All @@ -54,9 +55,11 @@ public function __construct(MessageBusInterface $bus, ContainerInterface $receiv
*/
protected function configure(): void
{
$defaultReceiverName = 1 === \count($this->receiverNames) ? current($this->receiverNames) : null;

$this
->setDefinition(array(
new InputArgument('receiver', $this->defaultReceiverName ? InputArgument::OPTIONAL : InputArgument::REQUIRED, 'Name of the receiver', $this->defaultReceiverName),
new InputArgument('receiver', $defaultReceiverName ? InputArgument::OPTIONAL : InputArgument::REQUIRED, 'Name of the receiver', $defaultReceiverName),
new InputOption('limit', 'l', InputOption::VALUE_REQUIRED, 'Limit the number of received messages'),
new InputOption('memory-limit', 'm', InputOption::VALUE_REQUIRED, 'The memory limit the worker can consume'),
new InputOption('time-limit', 't', InputOption::VALUE_REQUIRED, 'The time limit in seconds the worker can run'),
Expand All @@ -83,6 +86,27 @@ protected function configure(): void
;
}

/**
* {@inheritdoc}
*/
protected function interact(InputInterface $input, OutputInterface $output)
{
if (!$this->receiverNames || $this->receiverLocator->has($receiverName = $input->getArgument('receiver'))) {
return;
}

$style = new SymfonyStyle($input, $output);
if (null === $receiverName) {
$style->block('Missing receiver argument.', null, 'error', ' ', true);
$input->setArgument('receiver', $style->choice('Select one of the available receivers', $this->receiverNames));
} elseif ($alternatives = $this->findAlternatives($receiverName, $this->receiverNames)) {
$style->block(sprintf('Receiver "%s" is not defined.', $receiverName), null, 'error', ' ', true);
if ($style->confirm(sprintf('Do you want to receive from "%s" instead? ', $alternatives[0]), false)) {
$input->setArgument('receiver', $alternatives[0]);
}
}
}

/**
* {@inheritdoc}
*/
Expand Down Expand Up @@ -134,4 +158,21 @@ private function convertToBytes(string $memoryLimit): int

return $max;
}

private function findAlternatives($name, array $collection)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

{
$alternatives = array();
foreach ($collection as $item) {
$lev = levenshtein($name, $item);
if ($lev <= \strlen($name) / 3 || false !== strpos($item, $name)) {
$alternatives[$item] = isset($alternatives[$item]) ? $alternatives[$item] - $lev : $lev;
}
}

$threshold = 1e3;
$alternatives = array_filter($alternatives, function ($lev) use ($threshold) { return $lev < 2 * $threshold; });
ksort($alternatives, SORT_NATURAL | SORT_FLAG_CASE);

return array_keys($alternatives);
}
}
11 changes: 7 additions & 4 deletions src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,9 +166,8 @@ private function guessHandledClasses(\ReflectionClass $handlerClass, string $ser
private function registerReceivers(ContainerBuilder $container)
{
$receiverMapping = array();
$taggedReceivers = $container->findTaggedServiceIds($this->receiverTag);

foreach ($taggedReceivers as $id => $tags) {
foreach ($container->findTaggedServiceIds($this->receiverTag) as $id => $tags) {
$receiverClass = $container->findDefinition($id)->getClass();
if (!is_subclass_of($receiverClass, ReceiverInterface::class)) {
throw new RuntimeException(sprintf('Invalid receiver "%s": class "%s" must implement interface "%s".', $id, $receiverClass, ReceiverInterface::class));
Expand All @@ -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')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

$receiverNames = array();
foreach ($receiverMapping as $name => $reference) {
$receiverNames[(string) $reference] = $name;
}
$container->getDefinition('console.command.messenger_consume_messages')->replaceArgument(3, array_values($receiverNames));
}

$container->getDefinition('messenger.receiver_locator')->replaceArgument(0, $receiverMapping);
Expand Down
6 changes: 3 additions & 3 deletions src/Symfony/Component/Messenger/Tests/Command/ConsumeMessagesCommandTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,15 @@ class ConsumeMessagesCommandTest extends TestCase
{
public function testConfigurationWithDefaultReceiver()
{
$command = new ConsumeMessagesCommand($this->createMock(MessageBus::class), $this->createMock(ServiceLocator::class), null, 'messenger.transport.amqp');
$command = new ConsumeMessagesCommand($this->createMock(MessageBus::class), $this->createMock(ServiceLocator::class), null, array('amqp'));
$inputArgument = $command->getDefinition()->getArgument('receiver');
$this->assertFalse($inputArgument->isRequired());
$this->assertSame('messenger.transport.amqp', $inputArgument->getDefault());
$this->assertSame('amqp', $inputArgument->getDefault());
}

public function testConfigurationWithoutDefaultReceiver()
{
$command = new ConsumeMessagesCommand($this->createMock(MessageBus::class), $this->createMock(ServiceLocator::class));
$command = new ConsumeMessagesCommand($this->createMock(MessageBus::class), $this->createMock(ServiceLocator::class), null, array('amqp', 'dummy'));
$inputArgument = $command->getDefinition()->getArgument('receiver');
$this->assertTrue($inputArgument->isRequired());
$this->assertNull($inputArgument->getDefault());
Expand Down
21 changes: 2 additions & 19 deletions src/Symfony/Component/Messenger/Tests/DependencyInjection/MessengerPassTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,24 +118,7 @@ public function testItRegistersReceiversWithoutTagName()
$this->assertEquals(array(AmqpReceiver::class => new Reference(AmqpReceiver::class)), $container->getDefinition('messenger.receiver_locator')->getArgument(0));
}

public function testItRegistersOneReceiverAndSetsTheDefaultOneOnTheCommand()
{
$container = $this->getContainerBuilder();
$container->register('console.command.messenger_consume_messages', ConsumeMessagesCommand::class)->setArguments(array(
new Reference('message_bus'),
new Reference('messenger.receiver_locator'),
null,
null,
));

$container->register(AmqpReceiver::class, AmqpReceiver::class)->addTag('messenger.receiver', array('alias' => 'amqp'));

(new MessengerPass())->process($container);

$this->assertSame(AmqpReceiver::class, $container->getDefinition('console.command.messenger_consume_messages')->getArgument(3));
}

public function testItRegistersMultipleReceiversAndDoesNotSetTheDefaultOneOnTheCommand()
public function testItRegistersMultipleReceiversAndSetsTheReceiverNamesOnTheCommand()
{
$container = $this->getContainerBuilder();
$container->register('console.command.messenger_consume_messages', ConsumeMessagesCommand::class)->setArguments(array(
Expand All @@ -150,7 +133,7 @@ public function testItRegistersMultipleReceiversAndDoesNotSetTheDefaultOneOnTheC

(new MessengerPass())->process($container);

$this->assertNull($container->getDefinition('console.command.messenger_consume_messages')->getArgument(3));
$this->assertSame(array('amqp', 'dummy'), $container->getDefinition('console.command.messenger_consume_messages')->getArgument(3));
}

public function testItRegistersSenders()
Expand Down