Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] Add support for command lazy-loading#22734
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
0ae80fd tof04cd0fCompare| publicfunction__construct(ContainerInterface$container,array$commandNames) | ||
| { | ||
| $this->container =$container; | ||
| $this->commandNames =$commandNames; |
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.
what's the point of having an array of command names when you could just have all these in the container ? Just for theall ?
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.
yes, forfind() and related helpers especially (resolvingreq asrequire)
| */ | ||
| publicfunctionget($name) | ||
| { | ||
| if (!in_array($name,$this->commandNames,true)) { |
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.
Could we replace this byif(!$this->has($name)) to avoid duplicating code? (unless this function call makes a big difference in performance)
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.
I have no strong opinion on this, I looked at the existing and it seems duplicating has been preferredhttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/ServiceLocator.php#L47.
However it means that if a sub class extendshas() for some reasons,get() would still use the "default" way. Not sure if it's a good thing, perhaps someone else has an hint?
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.
i would use the api method indeed, or mark final :)
3539734 to10871a4Compare
ro0NL left a comment
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.
like this in general 👍
| }elseif ($this->commandLoader &&$this->commandLoader->has($name)) { | ||
| $command =$this->commandLoader->get($name); | ||
| if (!$command->getName()) { | ||
| $command->setName($name); |
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.
wouldnt it be more safe to always set this?
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.
indeed
| if (isset($this->commands[$name])) { | ||
| $command =$this->commands[$name]; | ||
| }elseif ($this->commandLoader &&$this->commandLoader->has($name)) { | ||
| $command =$this->commandLoader->get($name); |
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.
$command = $this->commands[$name] = ...;?
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.
add() does more than$this->command[$name] = $command, hence calling it
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.
omfg. my bad :)
| /** | ||
| * @return string[] All registered command names | ||
| */ | ||
| publicfunctiongetCommandNames(); |
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.
getNames?
| */ | ||
| publicfunctionget($name) | ||
| { | ||
| if (!in_array($name,$this->commandNames,true)) { |
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.
i would use the api method indeed, or mark final :)
| $definition =$container->getDefinition($id); | ||
| $class =$container->getParameterBag()->resolveValue($definition->getClass()); | ||
| if (!$r =$container->getReflectionClass($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.
should this skip abstracts first?
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 can't happen, this already throws on abstractshttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/DependencyInjection/AddConsoleCommandPass.php#L28
| private$loaderServiceId; | ||
| private$commandTag; | ||
| publicfunction__construct($loaderServiceId ='console.command_loader',$commandTag ='console.command') |
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.
imo.$tag or$commandLoaderServiceId ;-)
| publicfunctionhas($name) | ||
| { | ||
| returnisset($this->commands[$name]); | ||
| returnisset($this->commands[$name]) ||$this->commandLoader &&$this->commandLoader->has($name); |
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.
personally i preferx || (y && z)
6461a31 to3ec61bbComparetheofidry commentedMay 18, 2017
Wouldn't an alternative be to make an abstract static function |
chalasr commentedMay 18, 2017
@theofidry I thought about it. |
chalasr commentedMay 18, 2017 • 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.
Note that when using this feature, you don't need to set the name in your command. It is automatically set when the command is loaded. Same for aliases. |
Koc commentedMay 19, 2017
This implementation looks better than previous |
sepehr commentedJul 11, 2017
This is a very good news for a lot of people around the world! Thanks. |
6cd19b0 to9fa9fc7Compare
nicolas-grekas left a comment• 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.
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.
👍 with "for fine tuning" comments.
In a future PR, I thinkCommandLoaderInterface could be replaced by a more generic interface in the DI component, which would be an extension of PSR11 and would add a "getProvidedServiceIds" to it. But that's to be discussed in another PR :)
| /** | ||
| * @return iterable All registered commands mapped by names | ||
| */ | ||
| publicfunctionall(); |
nicolas-grekasJul 12, 2017 • 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.
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.
unless I'm misread, this method is not used at all, better remove it then, esp. since getNames() already allows iterating when needed
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.
agreed,all() removed
| * @param ContainerInterface $container A container from which to load command services | ||
| * @param array $commandMap An array with command names as keys and service ids as values | ||
| */ | ||
| publicfunction__construct(ContainerInterface$container,array$commandMap) |
nicolas-grekasJul 12, 2017 • 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.
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.
if container can be keyed by command names, then we won't need $commandMap (we'll need "$commandNames", which would be enough.)
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.
I started with only command names, but@mnapoli noticed that it prevents mapping a command to a service that is not named the same as the command (#22734 (comment)), which can be useful especially outside of the fullstack I think.
A good side effect is that using acommandMap makes that we register only one closure for a command and its aliases (they're all mapped to the same service id in the commandMap array instead of in the locator factories, that gives a lighter container). Still not convinced?
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.
Ok, fine for me, thanks for the details
ogizanagi commentedJul 12, 2017
I think we should avoid replacing |
nicolas-grekas commentedJul 12, 2017
Thank you@chalasr. |
This PR was merged into the 3.4 branch.Discussion----------[Console] Add support for command lazy-loading| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#12063#16438#13946#21781| License | MIT| Doc PR | todoThis PR adds command lazy-loading support to the console, based on PSR-11 and DI tags.(#12063 (comment))Commands registered as services which set the `command` attribute on their `console.command` tag are now instantiated when calling `Application::get()` instead of all instantiated at `run()`.__Usage__```yamlapp.command.heavy: tags: - { name: console.command, command: app:heavy }```This way private command services can be inlined (no public aliases, remain really private).With console+PSR11 implem only:```php$application = new Application();$container = new ServiceLocator(['heavy' => function () { return new Heavy(); }]);$application->setCommandLoader(new ContainerCommandLoader($container, ['app:heavy' => 'heavy']);```Implementation is widely inspired from Twig runtime loaders (without the definition/runtime separation which is not needed here).Commits-------7f97519 Add support for command lazy-loading
| thrownewInvalidArgumentException(sprintf('Missing "command" attribute on tag "%s" for service "%s".',$this->commandTag,$id)); | ||
| } | ||
| if ($commandName !==$tag['command']) { | ||
| thrownewInvalidArgumentException(sprintf('The "command" attribute must be the same on each "%s" tag for service "%s".',$this->commandTag,$id)); |
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.
Wouldn't it be more straightforward if aliases are just the additional tags using thecommand attribute as well?
Then there is no need for an alias property at all and this strange condition doesn't apply either.
tags: - { name: console.command, command: app:my-command } - { name: console.command, command: app:my-alias }…application with lazy-loading needs (ogizanagi)This PR was merged into the 3.4 branch.Discussion----------[Console] Add a factory command loader for standalone application with lazy-loading needs| Q | A| ------------- | ---| Branch? | 3.4 <!-- see comment below -->| Bug fix? | no| New feature? | yes <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass? | yes (failure unrelated)| Fixed tickets |#22734 (comment) <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | todo (withsymfony/symfony-docs#8147)So standalone applications can also benefit from the lazy loading feature without requiring a PSR-11 implementation specifically for this need.The loader does not memoize any resolved command from factories, as it's the `Application` responsibility and the `ContainerCommandLoader` does not either (the PSR-11 does not enforce two successive calls to return the same value).Commits-------9b40b4a [Console] Add a factory command loader for standalone application with lazy-loading needs
…application with lazy-loading needs (ogizanagi)This PR was merged into the 3.4 branch.Discussion----------[Console] Add a factory command loader for standalone application with lazy-loading needs| Q | A| ------------- | ---| Branch? | 3.4 <!-- see comment below -->| Bug fix? | no| New feature? | yes <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass? | yes (failure unrelated)| Fixed tickets |symfony/symfony#22734 (comment) <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | todo (withsymfony/symfony-docs#8147)So standalone applications can also benefit from the lazy loading feature without requiring a PSR-11 implementation specifically for this need.The loader does not memoize any resolved command from factories, as it's the `Application` responsibility and the `ContainerCommandLoader` does not either (the PSR-11 does not enforce two successive calls to return the same value).Commits-------9b40b4a [Console] Add a factory command loader for standalone application with lazy-loading needs
…oconfigure enabled (chalasr)This PR was merged into the 3.4 branch.Discussion----------[Console] Fix registering lazy command services with autoconfigure enabled| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aFor```yaml_defaults: autoconfigure: trueApp\: resource: '../../src/*'App\Command\FooCommand: tags: - { name: console.command, command: foo }```Before you get the following error:> Missing "command" attribute on tag "console.command" for service "App\Command\FooCommand"Now the command is lazy.----Btw,@Tobion's#22734 (comment)> Wouldn't it be more straightforward if aliases are just the additional tags using the command attribute as well?Then there is no need for an alias property at all and this strange condition doesn't apply either.Partially addressed here by removing the need for repeating the `command` attribute on each `console.command` tag```yaml# beforetags: - { name: console.command, command: foo } - { name: console.command, command: foo, alias: foobar }# aftertags: - { name: console.command, command: foo } - { name: console.command, alias: foobar }```Tobias proposal:```yamltags: - { name: console.command, command: app:my-command } - { name: console.command, command: app:my-alias }```I wanted to propose exactly the same at first, but finally found more clear to add a specific attribute for aliases, especially because relying on the order on which tags are defined sounds less good to me. Please tell me about your preference.(And sorry for the noise around this feature, I want to polish it for 3.4)Commits-------8a71aa3 Fix registering lazy command services with autoconfigure enabled…oconfigure enabled (chalasr)This PR was merged into the 3.4 branch.Discussion----------[Console] Fix registering lazy command services with autoconfigure enabled| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aFor```yaml_defaults: autoconfigure: trueApp\: resource: '../../src/*'App\Command\FooCommand: tags: - { name: console.command, command: foo }```Before you get the following error:> Missing "command" attribute on tag "console.command" for service "App\Command\FooCommand"Now the command is lazy.----Btw,@Tobion'ssymfony/symfony#22734 (comment)> Wouldn't it be more straightforward if aliases are just the additional tags using the command attribute as well?Then there is no need for an alias property at all and this strange condition doesn't apply either.Partially addressed here by removing the need for repeating the `command` attribute on each `console.command` tag```yaml# beforetags: - { name: console.command, command: foo } - { name: console.command, command: foo, alias: foobar }# aftertags: - { name: console.command, command: foo } - { name: console.command, alias: foobar }```Tobias proposal:```yamltags: - { name: console.command, command: app:my-command } - { name: console.command, command: app:my-alias }```I wanted to propose exactly the same at first, but finally found more clear to add a specific attribute for aliases, especially because relying on the order on which tags are defined sounds less good to me. Please tell me about your preference.(And sorry for the noise around this feature, I want to polish it for 3.4)Commits-------8a71aa31bb Fix registering lazy command services with autoconfigure enabledDanack commentedAug 13, 2017
Hi everyone, The solution that has been raised in this ticket seems complex and a workaround for the fundamental problem that combining the 'routing' of commands with the dispatching of commands is a 'bad idea', which is why most modern frameworks separate those two concerns in HTTP contexts; not separating them in CLI contexts seems just the wrong thing to do. I've was hoping when Iraised this issue a while ago that it could be improved in the next version of Symfony. I still think that would be a better approach than using lazy loading, which always makes it harder to reason about what is actually happening inside an application. I realise github comments are never the best place to discuss philosophical differences in software architecture - do you guys have a more appropriate place to discuss this? Otherwise I think I'm going to invite people to an 'official', maintained fork of the Symfony/console library, which would be along the lines of the version I've been using for a couple of yearshttps://github.com/danack/console |
nicolas-grekas commentedAug 13, 2017
I don't see what's complex with implemeting |
Ocramius commentedAug 13, 2017 via email
The complexity comes from adding a workaround rather than tackling theissue at core (which might not be possible). …On 13 Aug 2017 5:34 PM, "Nicolas Grekas" ***@***.***> wrote: I don't see what's complex with implemeting CommandLoaderInterface. Basically, it's what you say: a intermediary "router" between names and commands. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22734 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakC9e_jZVDSHlnhnE8ycZrLhLgI5tks5sXxeHgaJpZM4Neeqc> . |
nicolas-grekas commentedAug 13, 2017 • 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.
(sorry, bad issue number, reposting on#11974) |
Uh oh!
There was an error while loading.Please reload this page.
This PR adds command lazy-loading support to the console, based on PSR-11 and DI tags.
(#12063 (comment))
Commands registered as services which set the
commandattribute on theirconsole.commandtag are now instantiated when callingApplication::get()instead of all instantiated atrun().Usage
This way private command services can be inlined (no public aliases, remain really private).
With console+PSR11 implem only:
Implementation is widely inspired from Twig runtime loaders (without the definition/runtime separation which is not needed here).