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

chalasr · 2017-05-17T21:44:55Z

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

Taluu · 2017-05-17T22:57:21Z

src/Symfony/Component/Console/CommandLoader/ContainerCommandLoader.php

+    public function __construct(ContainerInterface $container, array $commandNames)
+    {
+        $this->container = $container;
+        $this->commandNames = $commandNames;


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 ?

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

javiereguiluz · 2017-05-18T11:19:29Z

src/Symfony/Component/Console/CommandLoader/ContainerCommandLoader.php

+     */
+    public function get($name)
+    {
+        if (!in_array($name, $this->commandNames, true)) {


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

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?

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

ro0NL

like this in general 👍

ro0NL · 2017-05-18T17:08:11Z

src/Symfony/Component/Console/Application.php

+        } elseif ($this->commandLoader && $this->commandLoader->has($name)) {
+            $command = $this->commandLoader->get($name);
+            if (!$command->getName()) {
+                $command->setName($name);


wouldnt it be more safe to always set this?

ro0NL · 2017-05-18T17:09:07Z

src/Symfony/Component/Console/Application.php

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


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

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

omfg. my bad :)

ro0NL · 2017-05-18T17:12:17Z

src/Symfony/Component/Console/CommandLoader/CommandLoaderInterface.php

+    /**
+     * @return string[] All registered command names
+     */
+    public function getCommandNames();


getNames?

ro0NL · 2017-05-18T17:13:29Z

src/Symfony/Component/Console/CommandLoader/ContainerCommandLoader.php

+     */
+    public function get($name)
+    {
+        if (!in_array($name, $this->commandNames, true)) {


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

ro0NL · 2017-05-18T17:16:03Z

src/Symfony/Component/Console/DependencyInjection/AddConsoleCommandPass.php

@@ -35,22 +48,41 @@ public function process(ContainerBuilder $container)
            if (!$r = $container->getReflectionClass($class)) {


should this skip abstracts first?

it can't happen, this already throws on abstracts https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/DependencyInjection/AddConsoleCommandPass.php#L28

ro0NL · 2017-05-18T17:17:50Z

src/Symfony/Component/Console/DependencyInjection/AddConsoleCommandPass.php

+    private $loaderServiceId;
+    private $commandTag;
+
+    public function __construct($loaderServiceId = 'console.command_loader', $commandTag = 'console.command')


imo. $tag or $commandLoaderServiceId ;-)

ro0NL · 2017-05-18T17:35:33Z

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);


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

theofidry · 2017-05-18T18:41:03Z

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 · 2017-05-18T19:05:52Z

@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 · 2017-05-18T19:08:56Z

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 · 2017-05-19T08:25:16Z

This implementation looks better than previous

sepehr · 2017-07-11T14:57:06Z

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

nicolas-grekas

👍 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 :)

nicolas-grekas · 2017-07-12T07:53:34Z

src/Symfony/Component/Console/CommandLoader/CommandLoaderInterface.php

+    /**
+     * @return iterable All registered commands mapped by names
+     */
+    public function all();


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

agreed, all() removed

nicolas-grekas · 2017-07-12T07:54:58Z

src/Symfony/Component/Console/CommandLoader/ContainerCommandLoader.php

+     * @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)


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

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?

Ok, fine for me, thanks for the details

ogizanagi · 2017-07-12T09:24:04Z

@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 · 2017-07-12T11:56:52Z

Thank you @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

Tobion · 2017-07-14T08:33:50Z

src/Symfony/Component/Console/DependencyInjection/AddConsoleCommandPass.php

+                    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));


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 }

…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  | Bug fix? | no | New feature? | yes  | BC breaks? | no | Deprecations? | no  | Tests pass? | yes (failure unrelated) | Fixed tickets | #22734 (comment)  | 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

…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  | Bug fix? | no | New feature? | yes  | BC breaks? | no | Deprecations? | no  | Tests pass? | yes (failure unrelated) | Fixed tickets | symfony/symfony#22734 (comment)  | 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

@Tobion

…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

@Tobion

…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 · 2017-08-13T15:16:25Z

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 · 2017-08-13T15:34:16Z

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

Ocramius · 2017-08-13T15:35:49Z

The complexity comes from adding a workaround rather than tackling the issue at core (which might not be possible).

…

On 13 Aug 2017 5:34 PM, "Nicolas Grekas" ***@***.***> wrote: I don't see what's complex with implemeting CommandLoaderInterface. Basically, it's what you say: a intermediary "router" between names and commands. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22734 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakC9e_jZVDSHlnhnE8ycZrLhLgI5tks5sXxeHgaJpZM4Neeqc> .

nicolas-grekas · 2017-08-13T15:37:16Z

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

carsonbot added Status: Needs Review Console Feature Deprecation labels May 17, 2017

chalasr force-pushed the lazy-console-commands branch 4 times, most recently from 0ae80fd to f04cd0f Compare May 17, 2017 22:09

chalasr mentioned this pull request May 17, 2017

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

Closed

chalasr force-pushed the lazy-console-commands branch from f04cd0f to a521c32 Compare May 17, 2017 22:16

Taluu reviewed May 17, 2017

View reviewed changes

javiereguiluz reviewed May 18, 2017

View reviewed changes

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

chalasr force-pushed the lazy-console-commands branch 6 times, most recently from 3539734 to 10871a4 Compare May 18, 2017 14:15

ro0NL reviewed May 18, 2017

View reviewed changes

chalasr force-pushed the lazy-console-commands branch 4 times, most recently from 6461a31 to 3ec61bb Compare May 18, 2017 18:13

chalasr force-pushed the lazy-console-commands branch from 3ec61bb to 7e90c79 Compare May 18, 2017 20:43

fabpot approved these changes Jul 11, 2017

View reviewed changes

chalasr force-pushed the lazy-console-commands branch 4 times, most recently from 6cd19b0 to 9fa9fc7 Compare July 11, 2017 22:59

nicolas-grekas approved these changes Jul 12, 2017

View reviewed changes

Add support for command lazy-loading

7f97519

chalasr force-pushed the lazy-console-commands branch from 9fa9fc7 to 7f97519 Compare July 12, 2017 09:59

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

chalasr deleted the lazy-console-commands branch July 12, 2017 12:27

xabbuh mentioned this pull request Jul 12, 2017

[Console] Add documentation about the console.command "command" attribute (lazy commands) symfony/symfony-docs#8147

Closed

Tobion reviewed Jul 14, 2017

View reviewed changes

ogizanagi mentioned this pull request Jul 14, 2017

[Console] Add a factory command loader for standalone application with lazy-loading needs #23510

Merged

chalasr mentioned this pull request Jul 17, 2017

[Console] Fix registering lazy command services with autoconfigure enabled #23556

Merged

nicolas-grekas mentioned this pull request Jul 21, 2017

[Console] DX: Catch Fatal errors in Command collection #23610

Closed

chalasr mentioned this pull request Aug 6, 2017

[RFC] Add static method + interface to get command names at compile time #23796

Closed

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

		@@ -35,22 +48,41 @@ public function process(ContainerBuilder $container)
		if (!$r = $container->getReflectionClass($class)) {

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

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

Conversation

chalasr commented May 17, 2017 • edited by nicolas-grekas Loading

Choose a reason for hiding this comment

chalasr May 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL May 18, 2017 • edited Loading

Choose a reason for hiding this comment

theofidry commented May 18, 2017

chalasr commented May 18, 2017

chalasr commented May 18, 2017 • edited Loading

Koc commented May 19, 2017

sepehr commented Jul 11, 2017

nicolas-grekas left a comment • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi commented Jul 12, 2017

nicolas-grekas commented Jul 12, 2017

Choose a reason for hiding this comment

Danack commented Aug 13, 2017

nicolas-grekas commented Aug 13, 2017

Ocramius commented Aug 13, 2017 via email

nicolas-grekas commented Aug 13, 2017 • edited Loading

chalasr commented May 17, 2017 •

edited by nicolas-grekas

Loading

chalasr May 17, 2017 •

edited

Loading

chalasr May 18, 2017 •

edited

Loading

ro0NL May 18, 2017 •

edited

Loading

chalasr commented May 18, 2017 •

edited

Loading

nicolas-grekas left a comment •

edited

Loading

nicolas-grekas Jul 12, 2017 •

edited

Loading

nicolas-grekas Jul 12, 2017 •

edited

Loading

nicolas-grekas commented Aug 13, 2017 •

edited

Loading