Skip to content

[FrameworkBundle] Multiple services on one Command class #19305

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

Closed
wants to merge 7 commits into from
Closed

[FrameworkBundle] Multiple services on one Command class #19305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
860b69e
Append service id to alias if it already exists
SenseException Jul 3, 2016
c01d94f
Add test to cover multiple services with one command class
SenseException Jul 6, 2016
caadcb3
Append service id to alias if it already exists
SenseException Jul 3, 2016
7753387
Add test to cover multiple services with one command class
SenseException Jul 6, 2016
11a5347
Fix of coding styles
SenseException Jul 6, 2016
859d517
Merge branch 'issue_19001' of github.com:SenseException/symfony into …
SenseException Jul 6, 2016
ba31ed8
Remove usage of setParameter for command class
SenseException Jul 7, 2016
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
6 changes: 5 additions & 1 deletion src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/AddConsoleCommandPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,11 @@ public function process(ContainerBuilder $container)
if (!is_subclass_of($class, 'Symfony\\Component\\Console\\Command\\Command')) {
throw new \InvalidArgumentException(sprintf('The service "%s" tagged "console.command" must be a subclass of "Symfony\\Component\\Console\\Command\\Command".', $id));
}
$container->setAlias($serviceId = 'console.command.'.strtolower(str_replace('\\', '_', $class)), $id);
$serviceId = 'console.command.'.strtolower(str_replace('\\', '_', $class));
if ($container->hasAlias($serviceId)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be replaced by something like this to work for any number of commands.

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 prefer to enumerate the alias for services of the same command class? What do we gain by adding another loop into the foreach and using numbers?

Copy link
Contributor

Choose a reason for hiding this comment

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

The actual proposition doesn't fix the current limitation but only reduce its impact.
For example still won't be able to have 3 commands with the same class.
If it is decided to fix this limitation it should be fully fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It uses the service name in the alias. For working services, the $id has to be unique, which is given in the alias too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh indeed sorry i thought it was a number... looks good to me then.

Copy link
Contributor

@mcfedr mcfedr Jul 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This little section can also be rearraged so that the check for $definition->isPublic() comes first, and avoid making $serviceId
Also, the check for existing alias could be avoided by always making $serviceId with ..._$id - there is no harm in doing this Seem that would break the automatic registration of Commands

$serviceId = $serviceId.'_'.$id;
}
$container->setAlias($serviceId, $id);
$serviceIds[] = $definition->isPublic() ? $id : $serviceId;
}

Expand Down
23 changes: 23 additions & 0 deletions ...y/Bundle/FrameworkBundle/Tests/DependencyInjection/Compiler/AddConsoleCommandPassTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,29 @@ public function testHttpKernelRegisterCommandsIngoreCommandAsAService()
$bundle->setContainer($container);
$bundle->registerCommands($application);
}

public function testProcessServicesWithSameCommand()
{
$container = new ContainerBuilder();
$container->addCompilerPass(new AddConsoleCommandPass());
$className = 'Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\Compiler\MyCommand';

$definition1 = new Definition($className);
$definition1->addTag('console.command');

$definition2 = new Definition($className);
$definition2->addTag('console.command');

$container->setDefinition('my-command1', $definition1);
$container->setDefinition('my-command2', $definition2);

$container->compile();

$alias1 = 'console.command.symfony_bundle_frameworkbundle_tests_dependencyinjection_compiler_mycommand';
$alias2 = $alias1.'_my-command2';
$this->assertTrue($container->hasAlias($alias1));
$this->assertTrue($container->hasAlias($alias2));
}
}

class MyCommand extends Command
Expand Down