Change artisan console to only fire constructors for the command being called

[samoldenburg]

Posted here as suggested from my issue in laravel/framework: laravel/framework#26570

Discussion detailed here: https://laracasts.com/discuss/channels/laravel/all-artisan-command-constructors-are-fired-when-running-any-artisan-commandwhat

In short, when running any artisan command, all artisan commands are instantiated to pull the command signature out, which is a protected property.

This is notably an issue if you use dependency injection in your constructors. In an application with many commands and dependencies, the artisan console will noticeably slow down.

This can be mitigated by resolving dependencies out of the container via $dependency = app(Dependency::class) in the handle() method, or by using dependency injection in the handle method rather than using dependency injection in the constructor, but the documentation specifically shows using dependency injection in the constructor with no warning about the repercussions of doing so. I have one or two particularly slow dependencies in my application which are used in only a couple artisan commands, yet all console methods and unit tests are slowed down as a result.

It would be great if the command signature could be delcared as public static or something so that instantiating the constructor for all artisan commands does not happen all the time. This would improve performance in (at least) 3 key areas for applications that use dependency injection in the constructor for their artisan console commands:

1. Artisan console
2. Programatically executing commands via Artisan::call()
3. Running unit tests that use database seeding

[mfn]

public static

I might not get love for this, but I've in fact dynamic signatures: they extract runtime information/configuration to present it in the command lines usage. How could I solve this with a changed model?

I'm however with you that the approach has problems 😄

[brunogaspar]

@samoldenburg I've started to do the DI on the handle method instead, as i was too noticing some slowdown and other problems (not entirely related to Laravel).
Works the same DI wise, but it's noticeable faster for me that way since it doesn't need to resolve the dependencies when you just do php artisan for example.

[samoldenburg]

@brunogaspar

I have taken the same approach for the time being, but that doesn't mean there isn't room for improvement here IMO.

The solution here could be as simple as updating the documentation use DI in the handle method instead of the __constructor, and removing the recommendation to do it in the constructor entirely, but that also sort of ignores the issue. My preferred solution would be to declare the command signature in a way that is visible without instantiating the command. However, I do not know the potential consequences of doing so, or if this problem reaches deeper into Symfony.

[jminkler]

I think there are a few issues here, the docs, the generated code creates a constructor, and how the lookup of the command through the signatures occurs. Perhaps the optimize command could cache the signatures, or the static suggestion, or reflection?

[HectorOrdonez]

What's the status of this? How can these ideas get upvotes or attention?

[samoldenburg]

I wish I knew.

More comments maybe?

I see this as a potentially large change, so I am nervous to invest time to doing a PR myself if Taylor disagrees with it for whatever reason.

[sisve]

A huge unanswered question still remains; how would you build dynamic signatures with this change? We currently allow a lot of magic in the Command::__construct.

One case would be safe-queue's WorkCommand which inherits from Laravel's WorkCommand, and just uses a regex to replace the command name, but keeps the parameters.
https://github.com/maxbrokman/SafeQueue/blob/0.3/src/Console/WorkCommand.php

[samoldenburg]

Is that a common use case? To change the command name like that? I've done dynamic things with arguments/options, but never with the command name.

Surely the configuration options beyond the command name are only necessary when calling help for a command, or actually calling the command.

I don't like the idea of caching the signatures - that seems a bit complex and adds overhead for something that really shouldn't need it.

[sisve]

Is that a common use case?

There are no usage statistics to answer that question..

Surely the configuration options beyond the command name are only necessary when calling help for a command, or actually calling the command.

Or when someone is calling php artisan --format=xml.

I'm still not sure that it's the interaction with the container alone that is problematic. Registering and resolving services is normally really quick. Why would that be slow? And the registration would happen anyway since service providers are executed even when just listing commands. (I'm ignoring deferred service providers here.)

Updating the documentation would be a appropriate fix in my eyes.

[samoldenburg]

I'm still not sure that it's the interaction with the container alone that is problematic. Registering and resolving services is normally really quick. Why would that be slow? And the registration would happen anyway since service providers are executed even when just listing commands. (I'm ignoring deferred service providers here.)

The case the prompted this for me was that I had one particularly slow deferred dependency. Now - maybe I could have just worked to make that dependency construct much faster and make the slow logic happen later, but I didn't think this would be an issue - because it would only be instantiated when I needed it, right? And when I needed it, it would have to do the slow requests anyway.

Updating the documentation would be a appropriate fix in my eyes.

I'll draft up a PR for the docs for this some time today as a starter.

[ejunker]

I noticed that Symfony now supports lazy-loaded commands. https://symfony.com/doc/4.0/console/commands_as_services.html#lazy-loading You can declare the name as protected static $defaultName = 'app:sunshine'; I tried to see if this could be used in Laravel and I think we would have to define a CommandLoader which could then be used to resolve the command on demand instead of having to instantiate each command.

Here you can see how the CommandLoader is used:
https://github.com/symfony/console/blob/master/Application.php#L614

[mfn]

Nice find!

It seems to me, Laravel could support both approaches: if a command has this static property, we treat it lazy, otherwise eager as it is right now.

As a best practice, the associated make command should default to the lazy version. If people need eager loaded commands, they can still do it the traditional way (e.g. for dynamic signatures).

[ejunker]

Here is a gist that I put together that shows how the CommandLoader could be used. I overrode the Kernel::load() method. If the command has a protected static $defaultName then we add it to $this->commandMap otherwise we resolve it as usual. Then in getArtisan() it calls setCommandLoader() and passes in $this->commandMap that was built in load();

The CommandLoader is almost the same as Symfony's ContainerCommandLoader but with one minor change in has().

With this code, if you run an artisan command it won't run the constructors of the other artisan commands that have a protected static $defaultName. If you want to use $signature in your commands you can use that with $defaultName and it works fine but ofcourse you have to specify the name twice. A bigger refactor would be to make a static version of $signature. This code also doesn't handle manually registered commands in Kernel::$commands.

https://gist.github.com/ejunker/e084a1c5b812bbb48afd3717ec167bbe

Anybody feel free to take this and submit a PR. I don't think I will have time.

[thiagorb]

Wouldn’t it be an option to define the signature in a static method instead of a static property? Then you can still have dynamic signatures, but wouldn’t need to instantiate the command to retrieve the signature.