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

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

Closed
rquadling opened this Issue Feb 27, 2017 · 3 comments

Comments

Projects
None yet
4 participants
@rquadling
Contributor

rquadling commented Feb 27, 2017

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? yes/no
Symfony version 2.8.17

If you have several \Symfony\Component\Console\Command\Commands, all registered against a \Symfony\Component\Console\Application, when running the application, all the \Symfony\Component\Console\Command\Command::configure() methods are called, even when a single command is chosen.

This is normally fine and I suspect this presents little or no problems at all.

But, as a consequence, when only the command's description is required (when running the application without any command) or no configuration required at all (when running the application with another command), the entire command is configured every single time. If that configuration requires access to external sources to provide options, or default values, etc. then this happens unnecessarily. Added to that, if there is an error in this external data retrieval, the exception is generated for what seems a completely unrelated command.

It would be nice to be able to be a bit more circumspect with what code is executed.

From that, I think there are 3 types of usage:

  1. When running the application without a command, only the description is required.
  2. When running the application with a different command, don't configure anything at all.
  3. When running the application with this command, configure it as things currently are.

One area where I'm not sure of is the interaction between command A running command B. I'm thinking this should match type 3 above.

If this is an issue for anyone, then I'm happy to work on it and build a PR, but would appreciate some direction on a good implementation. Ideally not too intrusive and something that is backward compatible.

@robfrawley

This comment has been minimized.

Show comment
Hide comment
@robfrawley

robfrawley Feb 27, 2017

Contributor

FYI: There have been many, many, different, failed, attempts at resolving the issues you listed and others in the past. Good luck, though: I think most people would agree the command component needs some TLC. The five aforementioned PRs and issues linked may give you some inspiration, as well as context as to what directions not to take, at the very least.

Contributor

robfrawley commented Feb 27, 2017

FYI: There have been many, many, different, failed, attempts at resolving the issues you listed and others in the past. Good luck, though: I think most people would agree the command component needs some TLC. The five aforementioned PRs and issues linked may give you some inspiration, as well as context as to what directions not to take, at the very least.

@rquadling

This comment has been minimized.

Show comment
Hide comment
@rquadling

rquadling Feb 27, 2017

Contributor

Hmm. I see. I hadn't considered the lazyloading/DI context. From my dumb perspective, injecting the application into the command during construction, rather than setting the application on the command once it is instantiated and having the state of the application known (registering commands, requesting command descriptions, executing a command) would seem to fulfil my requirement.

If such an approach was deemed valid, I could present a PR on this.

Contributor

rquadling commented Feb 27, 2017

Hmm. I see. I hadn't considered the lazyloading/DI context. From my dumb perspective, injecting the application into the command during construction, rather than setting the application on the command once it is instantiated and having the state of the application known (registering commands, requesting command descriptions, executing a command) would seem to fulfil my requirement.

If such an approach was deemed valid, I could present a PR on this.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jul 11, 2017

Member

Should be fixed by #22734

Member

nicolas-grekas commented Jul 11, 2017

Should be fixed by #22734

nicolas-grekas added a commit that referenced this issue 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment