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] Added support for lazy-loaded commands #13946

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
@mnapoli
Contributor

mnapoli commented Mar 17, 2015

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.

[Console] Support for lazy-loaded commands
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

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Mar 17, 2015

Contributor

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

Contributor

mnapoli commented Mar 17, 2015

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

This comment has been minimized.

Show comment
Hide comment
@unkind

unkind Mar 18, 2015

Contributor

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.

Contributor

unkind commented Mar 18, 2015

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

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Mar 18, 2015

Contributor

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?

Contributor

mnapoli commented Mar 18, 2015

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

This comment has been minimized.

Show comment
Hide comment
@unkind

unkind Mar 18, 2015

Contributor

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.

Contributor

unkind commented Mar 18, 2015

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

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Mar 18, 2015

Contributor

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

Contributor

mnapoli commented Mar 18, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@unkind

unkind Mar 19, 2015

Contributor

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.

Contributor

unkind commented Mar 19, 2015

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

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Mar 19, 2015

Member

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

Member

fabpot commented Mar 19, 2015

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

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Mar 19, 2015

Contributor

@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.

Contributor

mnapoli commented Mar 19, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@unkind

unkind Mar 19, 2015

Contributor

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

Contributor

unkind commented Mar 19, 2015

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

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Apr 2, 2015

Contributor

@mnapoli what is the status of this pull request ?

Contributor

mickaelandrieu commented Apr 2, 2015

@mnapoli what is the status of this pull request ?

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Apr 2, 2015

Contributor

@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
Contributor

mnapoli commented Apr 2, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@stof

stof Apr 18, 2015

Member

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

Member

stof commented Apr 18, 2015

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

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Apr 19, 2015

Contributor

@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?

Contributor

mnapoli commented Apr 19, 2015

@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

This comment has been minimized.

Show comment
Hide comment

notrix commented May 25, 2015

👍

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Jul 13, 2015

Contributor

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

Contributor

iltar commented Jul 13, 2015

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

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Jul 18, 2015

Contributor

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.

Contributor

mnapoli commented Jul 18, 2015

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

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Jul 20, 2015

Member

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

Member

xabbuh commented Jul 20, 2015

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

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Jul 20, 2015

Contributor

@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.

Contributor

iltar commented Jul 20, 2015

@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.

@fabpot fabpot added the Console label Oct 5, 2015

@TomasVotruba

This comment has been minimized.

Show comment
Hide comment
@TomasVotruba

TomasVotruba Nov 16, 2015

Contributor

What state is this in?

Contributor

TomasVotruba commented Nov 16, 2015

What state is this in?

@cedvan

This comment has been minimized.

Show comment
Hide comment

cedvan commented Dec 16, 2016

👍

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Jul 6, 2017

Contributor

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

Contributor

mnapoli commented Jul 6, 2017

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

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jul 11, 2017

Member

Closing since a better approach is being worked on.

Member

nicolas-grekas commented Jul 11, 2017

Closing since a better approach is being worked on.

@TomasVotruba

This comment has been minimized.

Show comment
Hide comment
@TomasVotruba

TomasVotruba Jul 11, 2017

Contributor

@nicolas-grekas Could you link that one please?

Contributor

TomasVotruba commented Jul 11, 2017

@nicolas-grekas Could you link that one please?

@theofidry

This comment has been minimized.

Show comment
Hide comment
@theofidry

theofidry Jul 11, 2017

Contributor

@TomasVotruba I think he was referring to #22734

Contributor

theofidry commented Jul 11, 2017

@TomasVotruba I think he was referring to #22734

@TomasVotruba

This comment has been minimized.

Show comment
Hide comment
@TomasVotruba

TomasVotruba Jul 11, 2017

Contributor

@theofidry I prefer links over guessing. Thanks.

Contributor

TomasVotruba commented Jul 11, 2017

@theofidry I prefer links over guessing. Thanks.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Jul 11, 2017

Contributor

🎉 thanks @chalasr

Contributor

mnapoli commented Jul 11, 2017

🎉 thanks @chalasr

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

symfony-splitter pushed a commit to symfony/security-bundle 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 | 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment