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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Console] Add support for command lazy-loading #22734

Merged
merged 1 commit into from Jul 12, 2017

Conversation

@chalasr
Member

chalasr commented May 17, 2017

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 todo

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 command attribute on their console.command tag are now instantiated when calling Application::get() instead of all instantiated at run().

Usage

app.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:

$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).

Show outdated Hide outdated src/Symfony/Component/Console/CommandLoader/ContainerCommandLoader.php
public function __construct(ContainerInterface $container, array $commandNames)
{
$this->container = $container;
$this->commandNames = $commandNames;

This comment has been minimized.

@Taluu

Taluu May 17, 2017

Contributor

what's the point of having an array of command names when you could just have all these in the container ? Just for the all ?

@Taluu

Taluu May 17, 2017

Contributor

what's the point of having an array of command names when you could just have all these in the container ? Just for the all ?

This comment has been minimized.

@chalasr

chalasr May 17, 2017

Member

yes, for find() and related helpers especially (resolving req asrequire)

@chalasr

chalasr May 17, 2017

Member

yes, for find() and related helpers especially (resolving req asrequire)

Show outdated Hide outdated src/Symfony/Component/Console/CommandLoader/ContainerCommandLoader.php
*/
public function get($name)
{
if (!in_array($name, $this->commandNames, true)) {

This comment has been minimized.

@javiereguiluz

javiereguiluz May 18, 2017

Member

Could we replace this by if(!$this->has($name)) to avoid duplicating code? (unless this function call makes a big difference in performance)

@javiereguiluz

javiereguiluz May 18, 2017

Member

Could we replace this by if(!$this->has($name)) to avoid duplicating code? (unless this function call makes a big difference in performance)

This comment has been minimized.

@chalasr

chalasr May 18, 2017

Member

I have no strong opinion on this, I looked at the existing and it seems duplicating has been preferred https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/ServiceLocator.php#L47.
However it means that if a sub class extends has() for some reasons, get() would still use the "default" way. Not sure if it's a good thing, perhaps someone else has an hint?

@chalasr

chalasr May 18, 2017

Member

I have no strong opinion on this, I looked at the existing and it seems duplicating has been preferred https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/ServiceLocator.php#L47.
However it means that if a sub class extends has() for some reasons, get() would still use the "default" way. Not sure if it's a good thing, perhaps someone else has an hint?

This comment has been minimized.

@ro0NL

ro0NL May 18, 2017

Contributor

i would use the api method indeed, or mark final :)

@ro0NL

ro0NL May 18, 2017

Contributor

i would use the api method indeed, or mark final :)

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone May 18, 2017

@ro0NL

like this in general 👍

Show outdated Hide outdated src/Symfony/Component/Console/Application.php
} elseif ($this->commandLoader && $this->commandLoader->has($name)) {
$command = $this->commandLoader->get($name);
if (!$command->getName()) {
$command->setName($name);

This comment has been minimized.

@ro0NL

ro0NL May 18, 2017

Contributor

wouldnt it be more safe to always set this?

@ro0NL

ro0NL May 18, 2017

Contributor

wouldnt it be more safe to always set this?

This comment has been minimized.

@chalasr

chalasr May 18, 2017

Member

indeed

@chalasr

chalasr May 18, 2017

Member

indeed

if (isset($this->commands[$name])) {
$command = $this->commands[$name];
} elseif ($this->commandLoader && $this->commandLoader->has($name)) {
$command = $this->commandLoader->get($name);

This comment has been minimized.

@ro0NL

ro0NL May 18, 2017

Contributor

$command = $this->commands[$name] = ...;?

@ro0NL

ro0NL May 18, 2017

Contributor

$command = $this->commands[$name] = ...;?

This comment has been minimized.

@chalasr

chalasr May 18, 2017

Member

add() does more than $this->command[$name] = $command, hence calling it

@chalasr

chalasr May 18, 2017

Member

add() does more than $this->command[$name] = $command, hence calling it

This comment has been minimized.

@ro0NL

ro0NL May 18, 2017

Contributor

omfg. my bad :)

@ro0NL

ro0NL May 18, 2017

Contributor

omfg. my bad :)

Show outdated Hide outdated src/Symfony/Component/Console/CommandLoader/CommandLoaderInterface.php
/**
* @return string[] All registered command names
*/
public function getCommandNames();

This comment has been minimized.

@ro0NL

ro0NL May 18, 2017

Contributor

getNames?

@ro0NL

ro0NL May 18, 2017

Contributor

getNames?

Show outdated Hide outdated src/Symfony/Component/Console/CommandLoader/ContainerCommandLoader.php
*/
public function get($name)
{
if (!in_array($name, $this->commandNames, true)) {

This comment has been minimized.

@ro0NL

ro0NL May 18, 2017

Contributor

i would use the api method indeed, or mark final :)

@ro0NL

ro0NL May 18, 2017

Contributor

i would use the api method indeed, or mark final :)

Show outdated Hide outdated src/Symfony/Component/Console/DependencyInjection/AddConsoleCommandPass.php
@@ -35,22 +48,41 @@ public function process(ContainerBuilder $container)
if (!$r = $container->getReflectionClass($class)) {

This comment has been minimized.

@ro0NL

ro0NL May 18, 2017

Contributor

should this skip abstracts first?

@ro0NL

ro0NL May 18, 2017

Contributor

should this skip abstracts first?

Show outdated Hide outdated src/Symfony/Component/Console/DependencyInjection/AddConsoleCommandPass.php
private $loaderServiceId;
private $commandTag;
public function __construct($loaderServiceId = 'console.command_loader', $commandTag = 'console.command')

This comment has been minimized.

@ro0NL

ro0NL May 18, 2017

Contributor

imo. $tag or $commandLoaderServiceId ;-)

@ro0NL

ro0NL May 18, 2017

Contributor

imo. $tag or $commandLoaderServiceId ;-)

Show outdated Hide outdated src/Symfony/Component/Console/Application.php
@@ -478,7 +495,7 @@ public function get($name)
*/
public function has($name)
{
return isset($this->commands[$name]);
return isset($this->commands[$name]) || $this->commandLoader && $this->commandLoader->has($name);

This comment has been minimized.

@ro0NL

ro0NL May 18, 2017

Contributor

personally i prefer x || (y && z)

@ro0NL

ro0NL May 18, 2017

Contributor

personally i prefer x || (y && z)

@theofidry

This comment has been minimized.

Show comment
Hide comment
@theofidry

theofidry May 18, 2017

Contributor

Wouldn't an alternative be to make an abstract static function getName(): string which could be called without instantiating the command? This would be a simpler solution and avoid the need to sync the command name from the tag and the code, although it would require to change the way we currently give the names to the commands.

Contributor

theofidry commented May 18, 2017

Wouldn't an alternative be to make an abstract static function getName(): string which could be called without instantiating the command? This would be a simpler solution and avoid the need to sync the command name from the tag and the code, although it would require to change the way we currently give the names to the commands.

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr May 18, 2017

Member

@theofidry I thought about it.
First, making getName() static would forbid having several commands for the same class, that is a valid use case to me (2b82fcb).
It would also be hard to achieve from a BC pov (actually, I don't see any smooth upgrade path for making a method static).
Lastly, this is mostly about commands registered as services (with dependencies), the current implementation doesn't require any change for basic command which are still in majority.
So I think the static alternative would be a too big step for the need that this is trying to solve.

Member

chalasr commented May 18, 2017

@theofidry I thought about it.
First, making getName() static would forbid having several commands for the same class, that is a valid use case to me (2b82fcb).
It would also be hard to achieve from a BC pov (actually, I don't see any smooth upgrade path for making a method static).
Lastly, this is mostly about commands registered as services (with dependencies), the current implementation doesn't require any change for basic command which are still in majority.
So I think the static alternative would be a too big step for the need that this is trying to solve.

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr May 18, 2017

Member

sync the command name from the tag and the code

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.

Member

chalasr commented May 18, 2017

sync the command name from the tag and the code

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

This comment has been minimized.

Show comment
Hide comment
@Koc

Koc May 19, 2017

Contributor

This implementation looks better than previous

Contributor

Koc commented May 19, 2017

This implementation looks better than previous

@theofidry

This comment has been minimized.

Show comment
Hide comment
@theofidry

theofidry May 19, 2017

Contributor

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.

Perfect then 👌

Contributor

theofidry commented May 19, 2017

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.

Perfect then 👌

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan May 22, 2017

Member

@chalasr Thanks for this! There is one practical problem: it will kill autoconfigure for commands :) ... which works really well right now. Wdyt?

Member

weaverryan commented May 22, 2017

@chalasr Thanks for this! There is one practical problem: it will kill autoconfigure for commands :) ... which works really well right now. Wdyt?

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr May 22, 2017

Member

@weaverryan You're right, this kills autoconfigure for commands.

To me, there are two different approaches for using console commands:

  1. Your command has no explicit dependency (it is container aware or has no dependency at all) and you don't care about laziness for the command itself
  2. Your command has explicit dependencies (constructor or setters) and you want it lazy

For 1, you probably rely on the fact that all commands present in the Command directory of each bundle are automatically added to the application. Even if doing so doesn't harm, there is no need for registering those as services.

For 2, you actually need to configure your service explicitly (still using autowire, defaults and other features that help simplifying service configuration), and you have to use this tag.

Also I must say that I'm very happy to deprecate the way commands services are currently added to the application, iterating over the ugly console.command.ids parameter for loading them all on the first run, that actually defeats the purpose of registering commands as services, losing laziness provided by container aware in favour of good design provided by explicit dependencies.

Considering that container aware stuff are far from ideal (since we push toward explicit deps and types, see also #21623), and that complex console commands do exist (more and more), I think we can safely kill it.

The other alternative is to make this optional, keeping the current way of registering command services (which, as said, is not DI-friendly at all and quite ugly), that would add some complexity to the "command as service" feature.

So... worth it? :) Wdyt?

Member

chalasr commented May 22, 2017

@weaverryan You're right, this kills autoconfigure for commands.

To me, there are two different approaches for using console commands:

  1. Your command has no explicit dependency (it is container aware or has no dependency at all) and you don't care about laziness for the command itself
  2. Your command has explicit dependencies (constructor or setters) and you want it lazy

For 1, you probably rely on the fact that all commands present in the Command directory of each bundle are automatically added to the application. Even if doing so doesn't harm, there is no need for registering those as services.

For 2, you actually need to configure your service explicitly (still using autowire, defaults and other features that help simplifying service configuration), and you have to use this tag.

Also I must say that I'm very happy to deprecate the way commands services are currently added to the application, iterating over the ugly console.command.ids parameter for loading them all on the first run, that actually defeats the purpose of registering commands as services, losing laziness provided by container aware in favour of good design provided by explicit dependencies.

Considering that container aware stuff are far from ideal (since we push toward explicit deps and types, see also #21623), and that complex console commands do exist (more and more), I think we can safely kill it.

The other alternative is to make this optional, keeping the current way of registering command services (which, as said, is not DI-friendly at all and quite ugly), that would add some complexity to the "command as service" feature.

So... worth it? :) Wdyt?

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr May 22, 2017

Member

Deprecation removed. If you want a command to be lazy then set the command attribute, and autoconfigure keeps working.

Member

chalasr commented May 22, 2017

Deprecation removed. If you want a command to be lazy then set the command attribute, and autoconfigure keeps working.

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan May 23, 2017

Member

@chalasr With the new PSR4 loader - and especially the fact that there won't be any bundles in Flex - command in the future will all need to be registered as services. I think - either official or in practice - the old way will go away. Then, the issue is just if we let autoconfigure continue to work for commands, and I hope we can :).

I actually like your latest change: autoconfigured commands still work, just aren't lazy. All course commands would of course be lazy. I was also thinking about a new, optional interface - e.g. NamedCommandInterface with a static public function getCommandName(). If you implemented this, it would be lazy without needing the tag (but you could also still tag of course). The idea is that end-users would typically use this. Wdyt?

Member

weaverryan commented May 23, 2017

@chalasr With the new PSR4 loader - and especially the fact that there won't be any bundles in Flex - command in the future will all need to be registered as services. I think - either official or in practice - the old way will go away. Then, the issue is just if we let autoconfigure continue to work for commands, and I hope we can :).

I actually like your latest change: autoconfigured commands still work, just aren't lazy. All course commands would of course be lazy. I was also thinking about a new, optional interface - e.g. NamedCommandInterface with a static public function getCommandName(). If you implemented this, it would be lazy without needing the tag (but you could also still tag of course). The idea is that end-users would typically use this. Wdyt?

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr May 23, 2017

Member

the old way will go away.

I agree, I was about to open an issue proposing to deprecate the convention-based bundle registration, in fact this is the ugly (reflection based) thing.

NamedCommandInterface with a static public function getCommandName(). If you implemented this, it would be lazy without needing the tag (but you could also still tag of course).

I like that, that feels more "robust" than using tag attributes and consistent with event/service subscribers. It should make the feature simpler to understand also (removing the complexity around the tag). Let's go for an interface with static getCommandName()/getCommandAliases()

Member

chalasr commented May 23, 2017

the old way will go away.

I agree, I was about to open an issue proposing to deprecate the convention-based bundle registration, in fact this is the ugly (reflection based) thing.

NamedCommandInterface with a static public function getCommandName(). If you implemented this, it would be lazy without needing the tag (but you could also still tag of course).

I like that, that feels more "robust" than using tag attributes and consistent with event/service subscribers. It should make the feature simpler to understand also (removing the complexity around the tag). Let's go for an interface with static getCommandName()/getCommandAliases()

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof May 23, 2017

Member

@weaverryan if we want to make the old way go away, we need to make all shared bundles go away. Flex does not make bundle disappear. It only removes project-specific bundles.

@chalasr please don't make things static. This is even worse for configurability, as you cannot make the name configurable anymore in commands (while it is currently), except by using some global static state (which is very bad), and only if you don't register the same command class multiple times with different names (and different instances of your deps, which is the use case for registering multiple times)

Member

stof commented May 23, 2017

@weaverryan if we want to make the old way go away, we need to make all shared bundles go away. Flex does not make bundle disappear. It only removes project-specific bundles.

@chalasr please don't make things static. This is even worse for configurability, as you cannot make the name configurable anymore in commands (while it is currently), except by using some global static state (which is very bad), and only if you don't register the same command class multiple times with different names (and different instances of your deps, which is the use case for registering multiple times)

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof May 23, 2017

Member

Thus, it will be almost impossible to write a good migration path for this: you cannot make the default implementation of the static method call the existing code, as static cannot call non-static code; so it means you still have to allow commands not implementing this static-based interface everywhere in the code.

Member

stof commented May 23, 2017

Thus, it will be almost impossible to write a good migration path for this: you cannot make the default implementation of the static method call the existing code, as static cannot call non-static code; so it means you still have to allow commands not implementing this static-based interface everywhere in the code.

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan May 23, 2017

Member

@stof I think the idea was for the interface with static methods to be optional. I agree we also need to have the tags way for the reasons you mentioned. That should give us the flexibility needed... but if you don't need that flexibility (most of the time you don't in end-user code), you have the static option.

Member

weaverryan commented May 23, 2017

@stof I think the idea was for the interface with static methods to be optional. I agree we also need to have the tags way for the reasons you mentioned. That should give us the flexibility needed... but if you don't need that flexibility (most of the time you don't in end-user code), you have the static option.

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr May 23, 2017

Member

@weaverryan Given the cons of the static api and the limitations that come with and although "implement this interface" feels better than "add this tag with this attribute" to me, I think we would better stick to the tag attribute only. I'm not fond of having two ways of doing the same thing, especially when one of them has limitations that the other has not, it makes it harder to understand.

Member

chalasr commented May 23, 2017

@weaverryan Given the cons of the static api and the limitations that come with and although "implement this interface" feels better than "add this tag with this attribute" to me, I think we would better stick to the tag attribute only. I'm not fond of having two ways of doing the same thing, especially when one of them has limitations that the other has not, it makes it harder to understand.

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan May 23, 2017

Member

@chalasr That's fine :). We can always consider it later. Unless I'm mistaken, with your current implementation, autoconfigure will still work... those commands just won't be lazy. That's a great step forward (and we can build on that if we want to).

Member

weaverryan commented May 23, 2017

@chalasr That's fine :). We can always consider it later. Unless I'm mistaken, with your current implementation, autoconfigure will still work... those commands just won't be lazy. That's a great step forward (and we can build on that if we want to).

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr May 23, 2017

Member

Unless I'm mistaken, with your current implementation, autoconfigure will still work

yes it does :) I also think that's a good thing because as you said, it works very well.

Member

chalasr commented May 23, 2017

Unless I'm mistaken, with your current implementation, autoconfigure will still work

yes it does :) I also think that's a good thing because as you said, it works very well.

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Jun 1, 2017

Member

rebased and green, ready to review

Member

chalasr commented Jun 1, 2017

rebased and green, ready to review

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Jun 10, 2017

Contributor

This seems extremely complex. Given that you already know all the dependencies of all commands from the container, wouldn't it be easier to mark those services as "lazy" (where applicable) in a compiler pass and be done with it?

The same behavior of this patch can be achieved with just the compiler pass then.

Contributor

Ocramius commented Jun 10, 2017

This seems extremely complex. Given that you already know all the dependencies of all commands from the container, wouldn't it be easier to mark those services as "lazy" (where applicable) in a compiler pass and be done with it?

The same behavior of this patch can be achieved with just the compiler pass then.

@theofidry

This comment has been minimized.

Show comment
Hide comment
@theofidry

theofidry Jun 11, 2017

Contributor
Contributor

theofidry commented Jun 11, 2017

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Jun 11, 2017

Contributor

The dependencies are usually only used in a command's execute.

The patch above goes great lengths to make some lightweight command object replacement, but that's a lot of wasted effort, as the commamd itself is generally a very lightweight object.

Contributor

Ocramius commented Jun 11, 2017

The dependencies are usually only used in a command's execute.

The patch above goes great lengths to make some lightweight command object replacement, but that's a lot of wasted effort, as the commamd itself is generally a very lightweight object.

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Jun 11, 2017

Member

Even if used in execute, dependencies are loaded at command construction, we can't assume that those are all "lazy" (symfony core and third party bundles services aren't). Commands can be heavy to instantiate (e.g. workers). Effort isn't wasted.

The goal is to provide a built-in way for lazy commands, no matter where their dependencies come from.
About complexity, this doesn't require more than adding the lazy attribute using the fullstack, and still no more than using proxy-manager with the console only.

Member

chalasr commented Jun 11, 2017

Even if used in execute, dependencies are loaded at command construction, we can't assume that those are all "lazy" (symfony core and third party bundles services aren't). Commands can be heavy to instantiate (e.g. workers). Effort isn't wasted.

The goal is to provide a built-in way for lazy commands, no matter where their dependencies come from.
About complexity, this doesn't require more than adding the lazy attribute using the fullstack, and still no more than using proxy-manager with the console only.

@skalpa

This comment has been minimized.

Show comment
Hide comment
@skalpa

skalpa Jun 11, 2017

Contributor

The Console component is probably the most widely used Symfony component, often with a different DI container.

Any solution that would depend on the Symfony DI, as opposed to the current one, would be useless to a lot of people.

Contributor

skalpa commented Jun 11, 2017

The Console component is probably the most widely used Symfony component, often with a different DI container.

Any solution that would depend on the Symfony DI, as opposed to the current one, would be useless to a lot of people.

@theofidry

This comment has been minimized.

Show comment
Hide comment
@theofidry

theofidry Jun 11, 2017

Contributor

@chalasr on another hand if you only need to add lazy to commands and install proxy manager, I'm not sure it's worth trying to add another way to do the lazy.

we can't assume that those are all "lazy"

I would argue that:

  • It's trivial to make all commands lazy with a Compiler pass if you really need lazyness
  • I don't think lazyness is required by default. I mean it is nice, but it's only needed on commands that requires things like database connection like in FosUserBundle (and that's where extra effort to ensure they are lazy even with the proxy manager have already been done) and workers which are arguably no so common (and when you have any and get annoyed by the slowness or care enough about, finding a solution which is making them lazy is relatively easy)
  • If Symfony really wants to try harder to make things lazy, the first point would be one easy solution

So whilst I do appreciate the PR, if there's already a solution that is good enough I don't really see the point to add extra code for another solution.

@skalpa if you need lazyness with the Console component and a different DI container, IMO the way to go is either take advantage of the DI container capacities in question (so here in FrameworkBundle the Symfony DIC) and not the Console component itself, and otherwise pursue something like #20656 with a PSR-11 container or use directly ocramius/proxy-manager.

Contributor

theofidry commented Jun 11, 2017

@chalasr on another hand if you only need to add lazy to commands and install proxy manager, I'm not sure it's worth trying to add another way to do the lazy.

we can't assume that those are all "lazy"

I would argue that:

  • It's trivial to make all commands lazy with a Compiler pass if you really need lazyness
  • I don't think lazyness is required by default. I mean it is nice, but it's only needed on commands that requires things like database connection like in FosUserBundle (and that's where extra effort to ensure they are lazy even with the proxy manager have already been done) and workers which are arguably no so common (and when you have any and get annoyed by the slowness or care enough about, finding a solution which is making them lazy is relatively easy)
  • If Symfony really wants to try harder to make things lazy, the first point would be one easy solution

So whilst I do appreciate the PR, if there's already a solution that is good enough I don't really see the point to add extra code for another solution.

@skalpa if you need lazyness with the Console component and a different DI container, IMO the way to go is either take advantage of the DI container capacities in question (so here in FrameworkBundle the Symfony DIC) and not the Console component itself, and otherwise pursue something like #20656 with a PSR-11 container or use directly ocramius/proxy-manager.

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Jun 11, 2017

Member

@theofidry proxy-manager is optional and will remain as is. Providing laziness in the core has been decided already, this just makes the console able to leverage it like others do (twig runtimes for instance).
Having a compiler pass looking at all the dependencies of a command to flag them and all their dependencies as "lazy" is not an option to me, especially because it requires both the symfony dic and proxy-manager, that would prevent making core commands lazy.
Having a component able to provide laziness is the proposed approach and I think is the way to go, we did it for a number of them already.

Member

chalasr commented Jun 11, 2017

@theofidry proxy-manager is optional and will remain as is. Providing laziness in the core has been decided already, this just makes the console able to leverage it like others do (twig runtimes for instance).
Having a compiler pass looking at all the dependencies of a command to flag them and all their dependencies as "lazy" is not an option to me, especially because it requires both the symfony dic and proxy-manager, that would prevent making core commands lazy.
Having a component able to provide laziness is the proposed approach and I think is the way to go, we did it for a number of them already.

@skalpa

This comment has been minimized.

Show comment
Hide comment
@skalpa

skalpa Jun 11, 2017

Contributor

@theofidry I think you're missing an important point. The console application needs to call the commands' getName() to retrieve their name.

Thus, just proxying the commands would have absolutely no benefit: you'd just delay the instantiation a bit, until their getName() is called, and would still end up instantiating every single command all the time.

This PR is not just about making the commands lazy, it's also about solving this problem while keeping BC.

Contributor

skalpa commented Jun 11, 2017

@theofidry I think you're missing an important point. The console application needs to call the commands' getName() to retrieve their name.

Thus, just proxying the commands would have absolutely no benefit: you'd just delay the instantiation a bit, until their getName() is called, and would still end up instantiating every single command all the time.

This PR is not just about making the commands lazy, it's also about solving this problem while keeping BC.

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Jun 11, 2017

Contributor
Contributor

Ocramius commented Jun 11, 2017

@gnugat

This comment has been minimized.

Show comment
Hide comment
@gnugat

gnugat Jun 12, 2017

Contributor

@Ocramius: The dependencies are usually only used in a command's execute.

Usually is highly subjective, there are some use cases for using dependencies in the help method too, or example to show default values for options that might be configured from the database.

I'm gonna reference here a comment I've made on the previous Pull Request:

I'd like to also highlight the following use case: displaying help using database (for example to set an option's default value).
This is currently impossible in a symfony full stack application, as it would prevent it from being installed: in order to create the database and the schema, we need to run some console commands, which means loading all commands including the one which tries to make a database query to set the default value of an option.

original comment

Contributor

gnugat commented Jun 12, 2017

@Ocramius: The dependencies are usually only used in a command's execute.

Usually is highly subjective, there are some use cases for using dependencies in the help method too, or example to show default values for options that might be configured from the database.

I'm gonna reference here a comment I've made on the previous Pull Request:

I'd like to also highlight the following use case: displaying help using database (for example to set an option's default value).
This is currently impossible in a symfony full stack application, as it would prevent it from being installed: in order to create the database and the schema, we need to run some console commands, which means loading all commands including the one which tries to make a database query to set the default value of an option.

original comment

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Jun 12, 2017

Contributor

Usually is highly subjective, there are some use cases for using dependencies in the help method too, or example to show default values for options that might be configured from the database.

Assuming that command names are constant is also highly subjective ;-)

Contributor

Ocramius commented Jun 12, 2017

Usually is highly subjective, there are some use cases for using dependencies in the help method too, or example to show default values for options that might be configured from the database.

Assuming that command names are constant is also highly subjective ;-)

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Jun 12, 2017

Member

Both aren't common use cases, but tags can still be set/altered through extensions/compiler passes.

Member

chalasr commented Jun 12, 2017

Both aren't common use cases, but tags can still be set/altered through extensions/compiler passes.

Show outdated Hide outdated src/Symfony/Component/Console/CommandLoader/ContainerCommandLoader.php
private $container;
private $names;
public function __construct(ContainerInterface $container, array $names)

This comment has been minimized.

@mnapoli

mnapoli Jul 6, 2017

Contributor

Should $names be a map of (command name) => (service name) rather than forcing the command name to be the same of the service name ?

Or maybe I misunderstood what this class is doing?

@mnapoli

mnapoli Jul 6, 2017

Contributor

Should $names be a map of (command name) => (service name) rather than forcing the command name to be the same of the service name ?

Or maybe I misunderstood what this class is doing?

This comment has been minimized.

@chalasr

chalasr Jul 6, 2017

Member

Very good point, it is now

@chalasr

chalasr Jul 6, 2017

Member

Very good point, it is now

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Jul 6, 2017

Contributor

❤️ This is very interesting!

+1000 with the argument that this solution is good for Symfony fullstack as well as outside of Symfony. The Console is a component used a lot outside of Symfony and this is important to find a solution that works in both contexts.

Contributor

mnapoli commented Jul 6, 2017

❤️ This is very interesting!

+1000 with the argument that this solution is good for Symfony fullstack as well as outside of Symfony. The Console is a component used a lot outside of Symfony and this is important to find a solution that works in both contexts.

@fabpot

fabpot approved these changes Jul 11, 2017

@sepehr

This comment has been minimized.

Show comment
Hide comment
@sepehr

sepehr Jul 11, 2017

Contributor

This is a very good news for a lot of people around the world! Thanks.

Contributor

sepehr commented Jul 11, 2017

This is a very good news for a lot of people around the world! Thanks.

@nicolas-grekas

nicolas-grekas approved these changes Jul 12, 2017 edited

👍 with "for fine tuning" comments.

In a future PR, I think CommandLoaderInterface 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 :)

Show outdated Hide outdated src/Symfony/Component/Console/CommandLoader/CommandLoaderInterface.php
/**
* @return iterable All registered commands mapped by names
*/
public function all();

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 12, 2017

Member

unless I'm misread, this method is not used at all, better remove it then, esp. since getNames() already allows iterating when needed

@nicolas-grekas

nicolas-grekas Jul 12, 2017

Member

unless I'm misread, this method is not used at all, better remove it then, esp. since getNames() already allows iterating when needed

This comment has been minimized.

@chalasr

chalasr Jul 12, 2017

Member

agreed, all() removed

@chalasr

chalasr Jul 12, 2017

Member

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
*/
public function __construct(ContainerInterface $container, array $commandMap)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 12, 2017

Member

if container can be keyed by command names, then we won't need $commandMap (we'll need "$commandNames", which would be enough.)

@nicolas-grekas

nicolas-grekas Jul 12, 2017

Member

if container can be keyed by command names, then we won't need $commandMap (we'll need "$commandNames", which would be enough.)

This comment has been minimized.

@chalasr

chalasr Jul 12, 2017

Member

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 a commandMap 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?

@chalasr

chalasr Jul 12, 2017

Member

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 a commandMap 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?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 12, 2017

Member

Ok, fine for me, thanks for the details

@nicolas-grekas

nicolas-grekas Jul 12, 2017

Member

Ok, fine for me, thanks for the details

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Jul 12, 2017

Member

@nicolas-grekas :

In a future PR, I think CommandLoaderInterface 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 :)

I think we should avoid replacing CommandLoaderInterface completely, which would mean tying this to the DI component. In a simple CLI application, you probably won't require symfony/dependency-injection but simply create your own implementation instead.
BTW, I'd like to suggest a simple factory based implementation in another PR (similar to DI component's ServiceLocator).

Member

ogizanagi commented Jul 12, 2017

@nicolas-grekas :

In a future PR, I think CommandLoaderInterface 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 :)

I think we should avoid replacing CommandLoaderInterface completely, which would mean tying this to the DI component. In a simple CLI application, you probably won't require symfony/dependency-injection but simply create your own implementation instead.
BTW, I'd like to suggest a simple factory based implementation in another PR (similar to DI component's ServiceLocator).

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
Member

nicolas-grekas commented Jul 12, 2017

Thank you @chalasr.

@nicolas-grekas nicolas-grekas merged commit 7f97519 into symfony:3.4 Jul 12, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Jul 12, 2017

feature #22734 [Console] Add support for command lazy-loading (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        | todo

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 `command` attribute on their `console.command` tag are now instantiated when calling `Application::get()` instead of all instantiated at `run()`.

__Usage__

```yaml
app.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

@chalasr chalasr deleted the chalasr:lazy-console-commands branch Jul 12, 2017

throw new InvalidArgumentException(sprintf('Missing "command" attribute on tag "%s" for service "%s".', $this->commandTag, $id));
}
if ($commandName !== $tag['command']) {
throw new InvalidArgumentException(sprintf('The "command" attribute must be the same on each "%s" tag for service "%s".', $this->commandTag, $id));

This comment has been minimized.

@Tobion

Tobion Jul 14, 2017

Member

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.

tags:
        - { name: console.command, command: app:my-command }
        - { name: console.command, command: app:my-alias }
@Tobion

Tobion Jul 14, 2017

Member

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.

tags:
        - { name: console.command, command: app:my-command }
        - { name: console.command, command: app:my-alias }

chalasr added a commit that referenced this pull request Jul 19, 2017

feature #23510 [Console] Add a factory command loader for standalone …
…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 (with symfony/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

symfony-splitter pushed a commit to symfony/console that referenced this pull request Jul 19, 2017

feature #23510 [Console] Add a factory command loader for standalone …
…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 (with symfony/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

fabpot added a commit that referenced this pull request Jul 20, 2017

minor #23556 [Console] Fix registering lazy command services with aut…
…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/a

For
```yaml
_defaults:
    autoconfigure: true

App\:
    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
# before
tags:
    - { name: console.command, command: foo }
    - { name: console.command, command: foo, alias: foobar }

# after
tags:
    - { name: console.command, command: foo }
    - { name: console.command, alias: foobar }
```

Tobias proposal:

```yaml
tags:
    - { 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

symfony-splitter pushed a commit to symfony/console that referenced this pull request Jul 20, 2017

minor #23556 [Console] Fix registering lazy command services with aut…
…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/a

For
```yaml
_defaults:
    autoconfigure: true

App\:
    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 symfony/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
# before
tags:
    - { name: console.command, command: foo }
    - { name: console.command, command: foo, alias: foobar }

# after
tags:
    - { name: console.command, command: foo }
    - { name: console.command, alias: foobar }
```

Tobias proposal:

```yaml
tags:
    - { 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 enabled
@Danack

This comment has been minimized.

Show comment
Hide comment
@Danack

Danack Aug 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 I raised 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 years https://github.com/danack/console

Danack commented Aug 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 I raised 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 years https://github.com/danack/console

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 13, 2017

Member

I don't see what's complex with implemeting CommandLoaderInterface. Basically, it's what you say: a intermediary "router" between names and commands.

Member

nicolas-grekas commented Aug 13, 2017

I don't see what's complex with implemeting CommandLoaderInterface. Basically, it's what you say: a intermediary "router" between names and commands.

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Aug 13, 2017

Contributor
Contributor

Ocramius commented Aug 13, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 13, 2017

Member

(sorry, bad issue number, reposting on #11974)
(hum, ok, no, the comment has been double-posted, I've been confused :) )

Member

nicolas-grekas commented Aug 13, 2017

(sorry, bad issue number, reposting on #11974)
(hum, ok, no, the comment has been double-posted, I've been confused :) )

This was referenced Oct 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment