[Console] Added support for lazy-loaded commands #13946

mnapoli · 2015-03-17T04:02:35Z

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

This PR adds support for lazy-loaded commands in the Console component (#12063).

I had to refactor the commands to extract the configuration into a separate class. That allows to define the configuration of a command separately from the command itself, which means we don't have to instantiate it. The command is then loaded only when being used. This is particularly useful when writing an application with a dependency injection container.

Backward compatibility is kept (if not it's a bug to fix) as Command extend from the new CommandConfiguration class (i.e. current commands are their own configuration). I wish they didn't as composition would be better instead of inheritance here, suggestions are welcome. Maybe we can break BC if we are targeting 3.0 and the change is deemed good enough?

To define a lazy-loaded command, here is a simple example:

$application = new Application();

$configuration = new CommandConfiguration();
$configuration
    ->setName('foo:bar')
    ->setDescription('description')
    ->setHelp('help');
$configuration->setCommand(function ($configuration) {
    // You might want to use a DI container here
    return new GreetCommand(null, $configuration);
});

$application->addCommandConfiguration($configuration);
$application->run();

Lazy-loaded commands don't have to define configure():

class GreetCommand extends Command
{
    protected function execute(InputInterface $input, OutputInterface $output)
    {
        $output->writeln('Hello');
    }
}

I hesitated between using closures that return the Command instance, or introducing a CommandResolver. I went with the simpler solution.

As explained in #12063, the current implementation forces to create all the Command instances which has the following problems for example:

a large part of the application might be instantiated for nothing
any error happening in the init of the instantiated objects will prevent the application from starting at all
any misconfiguration in the DI container will prevent the application from starting at all
admin and troubleshooting commands cannot be run if the application doesn't start (e.g. clear caches, start/stop server, show debug information, install the app, disable modules, …)
any logic in the object's initialization will be run, which may require external services (database, remote cache, webservice, …) or may have side-effects (which is not good obviously, but it can happen)

The impact and reality of those problems depend a lot of your application obviously. But just like an application shouldn't load every controller and their dependencies on each request, the console application shouldn't load every command and their dependencies on each execution if that can be avoided.

I don't believe this change has any impact on the Symfony full-stack framework but maybe it will give you some ideas.

Refactored the commands to extract the configuration into a separate class. That allows to define the configuration of a command without instantiating the command. The command is then loaded only when being used.

mnapoli · 2015-03-17T04:54:49Z

The build on 2.7 is currently broken which explains the failing PR, I will rebase when it is fixed. Locally tests are passing.

unkind · 2015-03-18T21:44:50Z

Honestly, I don't understand why we can't (re)use existing Command->code (Command::setCode) for lazy-loading purpose. Do I miss something?

Anyway, I definitely like to see separate CommandConfiguration class, but it would be great to have immutable object without command dependency.

mnapoli · 2015-03-18T22:01:49Z

Honestly, I don't understand why we can't (re)use existing Command->code (Command::setCode) for lazy-loading purpose. Do I miss something?

Yes it's possible but I consider it a cheap workaround rather than a solution. With that you would have to write commands that do not extend the Command class, you couldn't use helpers for example. And you can't implement initialize() or interact().

Instead of putting the command out of the Command class, I'd rather put the configuration out of the Command class ;) (makes more sense)

but it would be great to have immutable object without command dependency.

I don't I understand that part sorry?

unkind · 2015-03-18T22:21:26Z

With that you would have to write commands that do not extend the Command class

Why? Closure can resolve command and execute it:

$command = new new Command(null, $configuration);
$command->setCode(function ($input, $output, $configuration) {
    // Feel free to resolve command instance from DI container
    $internal = new GreetCommand(null, $configuration);
    return $internal->run($input, $output);
});

You can add this 3rd argument without breaking BC.

I don't I understand that part sorry?

I'd like to see a value object, not a mutable one.

mnapoli · 2015-03-18T23:05:25Z

@unkind Your example is a workaround. That's also the kind of workaround I have implemented for using in Piwik and we are not using it yet (and not released) because "it's not the Symfony API so people have to learn the Piwik console version (and we have to maintain it)".

So yes of course you can always find a way, but here I'm suggesting to improve Symfony Console to have an easy and clean solution available for everyone instead of releasing a "fork" (or rather alternative to symfony/console).

unkind · 2015-03-19T16:47:36Z

Your example is a workaround

I don't state it isn't. The only thing I don't like in your one is that CommandConfiguration (meta data) depends on $command instance (or factory). It's like Route holds $controller instance (or factory).

AFAIK, Symfony 2.7 is already closed for new features according to http://symfony.com/doc/current/contributing/community/releases.html. If it isn't, I can make alternative PR. However, I don't see much sense in these workarounds for 3.0.

fabpot · 2015-03-19T18:25:13Z

2.7 will not accept any new features at the end of the month, so we still have time.

mnapoli · 2015-03-19T20:11:25Z

@unkind Ah you mean CommandConfiguration::getCommand()?

Yes I don't find this perfect too and I'd rather introduce a CommandResolver like the ControllerResolver. However when I suggested that in the original issue (edit: rather I read it somewhere else) I could sense that people where not really keen on "overengineered" solutions (which I don't think it is) so that's why I went this this simple one. If there's more chance for a PR with a CommandResolver then I'll gladly add it.

unkind · 2015-03-19T23:23:24Z

@mnapoli by the way, you should remove Command::isEnabled().

mickaelandrieu · 2015-04-02T22:59:18Z

@mnapoli what is the status of this pull request ?

mnapoli · 2015-04-02T23:03:57Z

@mickaelandrieu it is waiting for some love :)

I.e. waiting for a core team member to tell me:

never going to happen
want to merge this PR but X or Y needs to be fixed
discuss alternate implementation, I'm open to implement it differently if needed

stof · 2015-04-18T09:58:18Z

I would prefer a way staying closer to the current architecture, as I explained in #13989 (comment) as well

mnapoli · 2015-04-19T00:06:24Z

@stof OK I can try a different approach, just to be clear the name and aliases would have to be defined outside the command right?

Here is a solution using callbacks as a factory:

$callable = function ($commandName) {
    // return the command instance
};
$app->addLazyCommand('name', ['aliases'], $callable);

Or we could introduce a CommandResolver like the ControllerResolver:

interface CommandResolver {
    public function getCommand($name);
}

$app->setCommandResolver($resolver);
$app->addLazyCommand('name', ['aliases'], 'command-id');

Thoughts before I go with an implementation?

notrix · 2015-05-25T06:18:21Z

👍

linaori · 2015-07-13T08:48:23Z

Any update on this? I'm having major issues right now due to a SoapClient being a dependency of a command. Due to the nature of how the php soap client works, it fails writing the wsdl into the cache with parallel running processes. Because running 1 command, triggers loading the dependencies of all, this fails and crashes.

The only way I can fix it is by fetching the soap service from the container at a later stage and this is something I'm hoping to avoid.

/ping @mnapoli

mnapoli · 2015-07-18T21:06:09Z

On my side I have nothing new to add here, I opened the PR and also offered alternative ideas for implementations so I'm waiting for feedback ;)

And since I opened this PR I keep seeing this problem more and more, for example on Piwik as we introduce dependency injection more and more.

xabbuh · 2015-07-20T09:00:29Z

@iltar For now, you could declare the SoapClient service lazy, can't you?

linaori · 2015-07-20T09:03:54Z

@xabbuh it's an option but I actually wrote my own interfaced wrapper for it, which in turn is a nicer solution than it originally was by calling the SoapClient directly. I don't like introducing lazy services via that package as the generated proxies provide some obstacles.

The php SoapClient is just bad by design.

TomasVotruba · 2015-11-16T00:28:43Z

What state is this in?

cedvan · 2016-12-16T10:42:40Z

👍

mnapoli · 2017-07-06T18:30:27Z

Have a look also at #22734 which seems to be a very good solution (maybe better than this one because simpler).

nicolas-grekas · 2017-07-11T14:39:58Z

Closing since a better approach is being worked on.

TomasVotruba · 2017-07-11T14:42:53Z

@nicolas-grekas Could you link that one please?

theofidry · 2017-07-11T14:45:16Z

@TomasVotruba I think he was referring to #22734

TomasVotruba · 2017-07-11T14:54:14Z

@theofidry I prefer links over guessing. Thanks.

mnapoli · 2017-07-11T14:55:55Z

🎉 thanks @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

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 | symfony/symfony#12063 symfony/symfony#16438 symfony/symfony#13946 #21781 | License | MIT | Doc PR | todo This PR adds command lazy-loading support to the console, based on PSR-11 and DI tags. (symfony/symfony#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

[Console] Support for lazy-loaded commands

bdc93d2

Refactored the commands to extract the configuration into a separate class. That allows to define the configuration of a command without instantiating the command. The command is then loaded only when being used.

mnapoli force-pushed the console branch from 16e9bf1 to bdc93d2 Compare March 17, 2015 04:05

unkind mentioned this pull request Mar 19, 2015

[Console] Added support for lazy-loaded commands (with Command resolver) #13989

Closed

fabpot added the Console label Oct 5, 2015

mnapoli mentioned this pull request Feb 22, 2016

Standardize on invokation of callables? container-interop/container-interop#43

Open

mnapoli mentioned this pull request Mar 3, 2016

[Console] Refactor the Console component for lazy-loading of commands #12063

Closed

javiereguiluz added the Feature label Mar 3, 2016

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

ogizanagi mentioned this pull request Feb 15, 2017

Deprecate ContainerAwareCommand #21623

Closed

robfrawley mentioned this pull request Feb 27, 2017

Configure is called for all commands when potentially/arguably only the description is needed. #21781

Closed

chalasr mentioned this pull request May 17, 2017

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

Merged

nicolas-grekas closed this Jul 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] Added support for lazy-loaded commands #13946

[Console] Added support for lazy-loaded commands #13946

mnapoli commented Mar 17, 2015

mnapoli commented Mar 17, 2015

unkind commented Mar 18, 2015

mnapoli commented Mar 18, 2015

unkind commented Mar 18, 2015

mnapoli commented Mar 18, 2015

unkind commented Mar 19, 2015

fabpot commented Mar 19, 2015

mnapoli commented Mar 19, 2015

unkind commented Mar 19, 2015

mickaelandrieu commented Apr 2, 2015

mnapoli commented Apr 2, 2015

stof commented Apr 18, 2015

mnapoli commented Apr 19, 2015

notrix commented May 25, 2015

linaori commented Jul 13, 2015

mnapoli commented Jul 18, 2015

xabbuh commented Jul 20, 2015

linaori commented Jul 20, 2015

TomasVotruba commented Nov 16, 2015

cedvan commented Dec 16, 2016

mnapoli commented Jul 6, 2017

nicolas-grekas commented Jul 11, 2017

TomasVotruba commented Jul 11, 2017

theofidry commented Jul 11, 2017

TomasVotruba commented Jul 11, 2017

mnapoli commented Jul 11, 2017

[Console] Added support for lazy-loaded commands #13946

[Console] Added support for lazy-loaded commands #13946

Conversation

mnapoli commented Mar 17, 2015

mnapoli commented Mar 17, 2015

unkind commented Mar 18, 2015

mnapoli commented Mar 18, 2015

unkind commented Mar 18, 2015

mnapoli commented Mar 18, 2015

unkind commented Mar 19, 2015

fabpot commented Mar 19, 2015

mnapoli commented Mar 19, 2015

unkind commented Mar 19, 2015

mickaelandrieu commented Apr 2, 2015

mnapoli commented Apr 2, 2015

stof commented Apr 18, 2015

mnapoli commented Apr 19, 2015

notrix commented May 25, 2015

linaori commented Jul 13, 2015

mnapoli commented Jul 18, 2015

xabbuh commented Jul 20, 2015

linaori commented Jul 20, 2015

TomasVotruba commented Nov 16, 2015

cedvan commented Dec 16, 2016

mnapoli commented Jul 6, 2017

nicolas-grekas commented Jul 11, 2017

TomasVotruba commented Jul 11, 2017

theofidry commented Jul 11, 2017

TomasVotruba commented Jul 11, 2017

mnapoli commented Jul 11, 2017