[Console] Add command resolver (proof of concept) #16438

funivan · 2015-11-03T12:12:52Z

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

Hello. This is simple proof of concept for command resolver.

Why?

On huge projects we should always initialize all commands, even if they are not used.
Some users want lazy-loading of commands. And there are a lot of ways how to do that. Store cache in files, db, initialize commands by unique id etc..

How?

This PR introduce new interface CommandResolverInterface. Any user can implement logic and inject it to the application. With this interface developer can create commands by request

BC ?

This interface is softly injected to this application. All tests pass. By default CommandResolver is simple holder for commands.

Current state

There are one problem: when we add Command to application, application add this command only if it is available. This logic is not covered in CommandResolverInterface. We put all responsibility to developer which Commands are available to application.

If it will be interesting to the team I can polish it (fix code style, possible change names of interface, write documentation etc.)

stof · 2015-11-03T12:53:08Z

src/Symfony/Component/Console/Application.php

@@ -87,6 +93,8 @@ public function __construct($name = 'UNKNOWN', $version = 'UNKNOWN')
        $this->helperSet = $this->getDefaultHelperSet();
        $this->definition = $this->getDefaultInputDefinition();

+        $this->commands = !empty($commandResolver) ? $commandResolver : new CommandResolver();


use a null comparison instead

aitboudad · 2015-11-04T08:35:21Z

src/Symfony/Component/Console/Application.php

@@ -87,6 +93,8 @@ public function __construct($name = 'UNKNOWN', $version = 'UNKNOWN')
        $this->helperSet = $this->getDefaultHelperSet();
        $this->definition = $this->getDefaultInputDefinition();

+        $this->commandResolver = ($commandResolver !== null) ? $commandResolver : new CommandResolver();


it should be null !== $commandResolver ? and parentheses is useless here

stloyd · 2015-11-04T08:54:20Z

src/Symfony/Component/Console/Tests/CommandResolver/CommandResolverTest.php

+ */
+class CommandResolverTest extends \PHPUnit_Framework_TestCase
+{
+  public function testLazyCommandResolver()


Wrong indent in whole file. Use 4 spaces not 2.

Sorry for that. I will fix it.

funivan · 2015-11-12T10:55:01Z

@stof Hello. Can you help me? I can`t pass all tests of this PR at CI services (In local environment all tests are green).
Some times it failure in AppVeyor sometimes in travis, but always in different places. And even more, sometimes failures are not relevant to my commits. Thank you.
What should I do to make this PR green?

TomasVotruba · 2016-03-03T00:55:12Z

Very nice PR!

Tests for Console are passing well. It fails on Symfony/Bridge/ProxyManager package, not sure why.
Maybe rerun might help.

funivan · 2016-03-03T07:06:41Z

@TomasVotruba yep and I don`t know how to resolve this. Is there any way to rerun this tests?

DavidBadura · 2016-03-03T07:24:56Z

with a rebase 😉

funivan · 2016-07-22T13:33:53Z

Hi 2 all. It is easier to create new pull request and implement the same logic.
If this feature is interesting to the community and to the core team I can implement the same algorithm of the command resolver.

chalasr · 2016-10-13T21:44:26Z

src/Symfony/Component/Console/Application.php

    {
        $this->name = $name;
        $this->version = $version;
        $this->defaultCommand = 'list';
        $this->helperSet = $this->getDefaultHelperSet();
        $this->definition = $this->getDefaultInputDefinition();

+        $this->commandResolver = $commandResolver !== null ? $commandResolver : new CommandResolver();


I think we use $this->commandResolver = $commandResolver ?: new CommandResolver()

nicolas-grekas · 2017-07-11T14:44:38Z

Closing in favor of #22734 which is going to be accepted and provide most of what this tries to achieve.
Thanks for working on this @funivan.

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

add command resolver (proof of concept)

c18f8e2

carsonbot added the Status: Needs Review label Nov 3, 2015

Ivan Scherbak added 2 commits November 3, 2015 14:13

fix code style

997d45d

fix php 5.3 $this context

1009753

stof reviewed Nov 3, 2015
View reviewed changes

Ivan Scherbak added 4 commits November 3, 2015 15:36

fix code style according to code review

997d431

fix cs. Remove white spaces

48c5426

fix code style and phpdoc

d883f89

fix phpdoc

d07ee8a

funivan mentioned this pull request Nov 4, 2015

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

Closed

aitboudad reviewed Nov 4, 2015
View reviewed changes

fix code style according to @aitboudad code review

caf62b3

stloyd reviewed Nov 4, 2015
View reviewed changes

Ivan Scherbak added 4 commits November 4, 2015 17:02

fix spaces in test class

0d9ddec

Improve test command resolver class

40f304a

add phpdoc

64a9e67

add phpdoc to add method

6b0f347

xabbuh added the Console label Nov 12, 2015

javiereguiluz changed the title ~~[CONSOLE] Add command resolver (proof of concept)~~ [Console] Add command resolver (proof of concept) Mar 3, 2016

javiereguiluz added the Feature label Mar 3, 2016

chalasr reviewed Oct 13, 2016

View reviewed changes

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

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

funivan deleted the console-command-resolv branch July 11, 2017 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] Add command resolver (proof of concept) #16438

[Console] Add command resolver (proof of concept) #16438

funivan commented Nov 3, 2015

stof Nov 3, 2015

aitboudad Nov 4, 2015

stloyd Nov 4, 2015

funivan Nov 4, 2015

funivan commented Nov 12, 2015

TomasVotruba commented Mar 3, 2016

funivan commented Mar 3, 2016

DavidBadura commented Mar 3, 2016

funivan commented Jul 22, 2016 •

edited

Loading

chalasr Oct 13, 2016

nicolas-grekas commented Jul 11, 2017

[Console] Add command resolver (proof of concept) #16438

[Console] Add command resolver (proof of concept) #16438

Conversation

funivan commented Nov 3, 2015

Why?

How?

BC ?

Current state

stof Nov 3, 2015

Choose a reason for hiding this comment

aitboudad Nov 4, 2015

Choose a reason for hiding this comment

stloyd Nov 4, 2015

Choose a reason for hiding this comment

funivan Nov 4, 2015

Choose a reason for hiding this comment

funivan commented Nov 12, 2015

TomasVotruba commented Mar 3, 2016

funivan commented Mar 3, 2016

DavidBadura commented Mar 3, 2016

funivan commented Jul 22, 2016 • edited Loading

chalasr Oct 13, 2016

Choose a reason for hiding this comment

nicolas-grekas commented Jul 11, 2017

funivan commented Jul 22, 2016 •

edited

Loading