[FrameworkBundle] Multiple services on one Command class #19305

SenseException · 2016-07-06T21:00:05Z

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

Based on my opened issue #19001, I created this PR to introduce the possibility of using multiple command-services with one command class. This should make Commands much more equal to other services in Symfony.

This way, a Command class can be used in more than just one service without breaking registerCommands of HttpKernel component.

…issue_19001 Conflicts: src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/AddConsoleCommandPass.php src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Compiler/AddConsoleCommandPassTest.php

linaori · 2016-07-07T07:00:23Z

...fony/Bundle/FrameworkBundle/Tests/DependencyInjection/Compiler/AddConsoleCommandPassTest.php

+        $container->addCompilerPass(new AddConsoleCommandPass());
+        $container->setParameter('my-command.class', 'Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\Compiler\MyCommand');
+
+        $definition1 = new Definition('%my-command.class%');


Just put this in a variable, no need to use parameters to define class names

linaori · 2016-07-07T07:01:07Z

So now that you can register the same command service twice, how will you solve the conflicting name problem?

ogizanagi · 2016-07-07T07:42:12Z

@iltar: I guess it's up to you to configure a different name according to a constructor argument or whatever.

linaori · 2016-07-07T07:48:32Z

This feature is closely related to another; Lazy command initialization. Ideally I'd configure the name via the tag.

SenseException · 2016-07-07T08:46:54Z

Yes, Command allows to inject the command's name in the constructor. I think injecting it should be a best practice for a future major Symfony release, but I also like @iltar's idea with using the name in the tag.

@iltar: I'll adapt the test. Is there a link to that other feature you mentioned?

linaori · 2016-07-07T08:53:23Z

No, but it's come up quite a few times. The current system lets Commands define their own name, which works. However, this means the command has to be initialized. So if you have 20 commands, every single command has to be instantiated in order to get the name.

The problem arises when you start using command as a service (my preference). It means that every single command will be initialized with all their dependencies. Hence there should be some kind of support of defining and retrieving their names without initializing them.

linaori · 2016-07-07T10:31:14Z

Now that I think of it, Commands could be decorated. In a compiler pass you could do something that would eventually trigger this:

new LazyCommand($container, $commandServiceId, $name);
getName(): $name
run($input, $output) {
    $this->container->get($commandServiceId)->run($input, $output);
}

GuilhemN · 2016-07-07T20:25:10Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/AddConsoleCommandPass.php

@@ -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)) {


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

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?

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.

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

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

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

SenseException · 2016-07-21T09:58:57Z

It would be nice if Symfony would handle its commands like any other class in a service.

mcfedr · 2016-07-26T15:26:34Z

Are there any work arounds for the moment? I guess I just need to create subclasses for each service.

mcfedr · 2016-07-26T15:28:29Z

Also, I would call this a bug fix not a feature.

SenseException · 2016-07-27T20:50:41Z

@mcfedr Subclasses are my current workaround. I marked the PR as feature because commands as a service were introduced in Symfony 2.4 and we're looking on legacy-code from the time before that. It just didn't grow with the rest.

SenseException · 2016-09-15T20:51:39Z

Ping.

fabpot · 2017-03-22T21:49:11Z

👍 Can you rebase to fix conflicts and get rid of the merge commit? Thanks.

fabpot · 2017-03-22T22:50:34Z

I've done the work in #22120

…ss (SenseException) This PR was squashed before being merged into the 3.3-dev branch (closes #22120). Discussion ---------- [FrameworkBundle] Multiple services on one Command class rebased version of #19305 Commits ------- 2b82fcb [FrameworkBundle] Multiple services on one Command class

SenseException added 4 commits July 3, 2016 23:35

Append service id to alias if it already exists

860b69e

This way, a Command class can be used in more than just one service without breaking registerCommands of HttpKernel component.

Add test to cover multiple services with one command class

c01d94f

Append service id to alias if it already exists

caadcb3

This way, a Command class can be used in more than just one service without breaking registerCommands of HttpKernel component.

Add test to cover multiple services with one command class

7753387

carsonbot added Status: Needs Review FrameworkBundle Feature labels Jul 6, 2016

SenseException added 2 commits July 6, 2016 23:07

Fix of coding styles

11a5347

linaori reviewed Jul 7, 2016
View reviewed changes

Remove usage of setParameter for command class

ba31ed8

GuilhemN reviewed Jul 7, 2016
View reviewed changes

mcfedr mentioned this pull request Jul 27, 2016

[Console] Move AddConsoleCommandPass from FrameworkBundle to Console. #19443

Merged

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

fabpot mentioned this pull request Mar 22, 2017

[FrameworkBundle] Multiple services on one Command class #22120

Closed

fabpot closed this Mar 22, 2017

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle] Multiple services on one Command class #19305

[FrameworkBundle] Multiple services on one Command class #19305

SenseException commented Jul 6, 2016

linaori Jul 7, 2016

linaori commented Jul 7, 2016 •

edited

Loading

ogizanagi commented Jul 7, 2016

linaori commented Jul 7, 2016

SenseException commented Jul 7, 2016

linaori commented Jul 7, 2016

linaori commented Jul 7, 2016

GuilhemN Jul 7, 2016

SenseException Jul 8, 2016

GuilhemN Jul 10, 2016

SenseException Jul 10, 2016

GuilhemN Jul 10, 2016

mcfedr Jul 26, 2016 •

edited

Loading

SenseException commented Jul 21, 2016

mcfedr commented Jul 26, 2016

mcfedr commented Jul 26, 2016

SenseException commented Jul 27, 2016

SenseException commented Sep 15, 2016

fabpot commented Mar 22, 2017

fabpot commented Mar 22, 2017

[FrameworkBundle] Multiple services on one Command class #19305

[FrameworkBundle] Multiple services on one Command class #19305

Conversation

SenseException commented Jul 6, 2016

linaori Jul 7, 2016

Choose a reason for hiding this comment

linaori commented Jul 7, 2016 • edited Loading

ogizanagi commented Jul 7, 2016

linaori commented Jul 7, 2016

SenseException commented Jul 7, 2016

linaori commented Jul 7, 2016

linaori commented Jul 7, 2016

GuilhemN Jul 7, 2016

Choose a reason for hiding this comment

SenseException Jul 8, 2016

Choose a reason for hiding this comment

GuilhemN Jul 10, 2016

Choose a reason for hiding this comment

SenseException Jul 10, 2016

Choose a reason for hiding this comment

GuilhemN Jul 10, 2016

Choose a reason for hiding this comment

mcfedr Jul 26, 2016 • edited Loading

Choose a reason for hiding this comment

SenseException commented Jul 21, 2016

mcfedr commented Jul 26, 2016

mcfedr commented Jul 26, 2016

SenseException commented Jul 27, 2016

SenseException commented Sep 15, 2016

fabpot commented Mar 22, 2017

fabpot commented Mar 22, 2017

linaori commented Jul 7, 2016 •

edited

Loading

mcfedr Jul 26, 2016 •

edited

Loading