Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This way, a Command class can be used in more than just oneservice without breaking registerCommands of HttpKernelcomponent.
This way, a Command class can be used in more than just oneservice without breaking registerCommands of HttpKernelcomponent.
…issue_19001Conflicts:src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/AddConsoleCommandPass.phpsrc/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Compiler/AddConsoleCommandPassTest.php
| $container->addCompilerPass(newAddConsoleCommandPass()); | ||
| $container->setParameter('my-command.class','Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\Compiler\MyCommand'); | ||
| $definition1 =newDefinition('%my-command.class%'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just put this in a variable, no need to use parameters to define class names
linaori commentedJul 7, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
So now that you can register the same command service twice, how will you solve the conflicting name problem? |
ogizanagi commentedJul 7, 2016
@iltar: I guess it's up to you to configure a different name according to a constructor argument or whatever. |
linaori commentedJul 7, 2016
This feature is closely related to another; Lazy command initialization. Ideally I'd configure the name via the tag. |
SenseException commentedJul 7, 2016
linaori commentedJul 7, 2016
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 commentedJul 7, 2016
Now that I think of it, Commands could be decorated. In a compiler pass you could do something that would eventually trigger this: |
| } | ||
| $container->setAlias($serviceId ='console.command.'.strtolower(str_replace('\\','_',$class)),$id); | ||
| $serviceId ='console.command.'.strtolower(str_replace('\\','_',$class)); | ||
| if ($container->hasAlias($serviceId)) { |
There was a problem hiding this comment.
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 likethis to work for any number of commands.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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$serviceIdAlso, the check for existing alias could be avoided byalways making Seem that would break the automatic registration of Commands$serviceId with..._$id - there is no harm in doing this
SenseException commentedJul 21, 2016
It would be nice if Symfony would handle its commands like any other class in a service. |
mcfedr commentedJul 26, 2016
Are there any work arounds for the moment? I guess I just need to create subclasses for each service. |
mcfedr commentedJul 26, 2016
Also, I would call this a bug fix not a feature. |
SenseException commentedJul 27, 2016
@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 commentedSep 15, 2016
Ping. |
fabpot commentedMar 22, 2017
👍 Can you rebase to fix conflicts and get rid of the merge commit? Thanks. |
fabpot commentedMar 22, 2017
I've done the work in#22120 |
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.