Overriding console command constructor is bad practice

[adideas]

My solution will protect developers from implicit code invocation!

To initialize the console command class, it would be more correct to initialize it in the "handle" method. Overriding a constructor can be a big hassle. This can be seen on large applications.

Causes:

1. Not everyone who uses frameworks is a good programmer.
2. Code execution occurs that was not intended.
3. Throwing Implicit Errors
4. Execute code snippets only when needed.

Examples:

• Example 1

<?php

namespace App\Console\Commands;

use Illuminate\Console\Command;

class TestCommand extends Command
{
    protected $signature   = 'test_command';

    protected $description = 'Error example';

    // Is bad practic
    public function __construct()
    {
        parent::__construct();
        dd("This code is always executed");
    }

    public function handle(): void
    {
        // TODO logic
    }
}

If you run any command

php artisan | php arisan cache:clear | php artisan queue:work | etc. 

In console output

"This code is always executed"

"Worker log" or "Horizon log" or "Supervisor log"

"This code is always executed"

If you run cron, this task will run many times (/var/log/syslog)

"This code is always executed"

In this example, we see that the code in the constructor is executed many times and everywhere. It would be nice if this was done upon request. When it is necessary. This example shows that it is very easy to get confused. On the one hand, the initialization of the class is described in the constructor. On the other hand, we get a time bomb.

• Example 2

TestCommand.php

<?php

namespace App\Console\Commands;

use App\Models\User;
use Illuminate\Console\Command;
use Illuminate\Support\Collection;
use Illuminate\Support\Facades\Event;

/**
 * The task is to check who has not paid for a subscription for a long time
 * @package App\Console\Commands
 */
class TestCommand extends Command
{
    protected $signature   = 'test_command';

    protected $description = 'Error example';

    /**
     * @var Collection<User>
     *
     *     User |- <int>    id
     *          |- <string> name
     *          |- <bool>   paid
     *          |- <Carbon> paid_at
     *          |- <int>    tariff_id -> relation
     *          <etc...>
     *
     *     User relations
     *              hasOne Tariff
     */
    protected Collection $user;

    public function __construct()
    {
        parent::__construct();
        // class variable initialization : return collect 10000+ items.
        $this->user = User::with('tariff')
            ->where('id', '>', now()->addMonths(-5))
            ->get();


    }

    /**
     * This logic is just a demonstration.
     * The task is to compare and update the model.
     * According to the task, we have to iterate over the users
     */
    public function handle(): void
    {
        // Loop (model iteration) 10000+ items
        $this->user->each(
            function (User $user) {
                // Any comparison
                $user->paid = $user->tariff->value == 'VALUE';
                $user->save();
            }
        );
    }
}

EventServiceProvider.php

<?php

namespace App\Providers;

use Illuminate\Auth\Events\Registered;
use Illuminate\Auth\Listeners\SendEmailVerificationNotification;
use Illuminate\Foundation\Support\Providers\EventServiceProvider as ServiceProvider;
use Illuminate\Support\Facades\Event;

class EventServiceProvider extends ServiceProvider
{
    /**
     * The event listener mappings for the application.
     *
     * @var array<class-string, array<int, class-string>>
     */
    protected $listen = [
        Registered::class => [
            SendEmailVerificationNotification::class,
        ],
    ];

    /**
     * Register any events for your application.
     *
     * @return void
     */
    public function boot()
    {
        // Register event ( ONLY FOR TEST )
        Event::listen("*", function(...$params) {
            dump($params[0]);
        });
    }
}

As you can see, we have created a console command and registered an event listener in the EventServiceProvider.
In the EventServiceProvider::boot method, we registered a listener just so we can monitor how the application is running. We are only interested in the name of the event.

If you run any command

php artisan | php arisan cache:clear | php artisan queue:work | etc. 

In console output

"bootstrapped: Illuminate\Foundation\Bootstrap\BootProviders"
"Illuminate\Console\Events\ArtisanStarting"
# "eloquent.booting: App\Models\User" <= I don't think it's good.
# "eloquent.booting: App\Models\User" <= I don't think it's good.
# "eloquent.booted: App\Models\User" <= I don't think it's good.
# "eloquent.booted: App\Models\User" <= I don't think it's good.
# "Illuminate\Database\Events\StatementPrepared" <= I don't think it's good.
# "Illuminate\Database\Events\StatementPrepared" <= I don't think it's good.
# "Illuminate\Database\Events\QueryExecuted" <= I don't think it's good.
# "Illuminate\Database\Events\QueryExecuted" <= I don't think it's good.
"Illuminate\Console\Events\CommandStarting"
"Illuminate\Console\Events\CommandStarting"
"Illuminate\Console\Events\CommandFinished"
"Illuminate\Console\Events\CommandFinished"

"Worker log" or "Horizon log" or "Supervisor log"

"bootstrapped: Illuminate\Foundation\Bootstrap\BootProviders"
"Illuminate\Console\Events\ArtisanStarting"
# "eloquent.booting: App\Models\User" <= I don't think it's good.
# "eloquent.booting: App\Models\User" <= I don't think it's good.
# "eloquent.booted: App\Models\User" <= I don't think it's good.
# "eloquent.booted: App\Models\User" <= I don't think it's good.
# "Illuminate\Database\Events\StatementPrepared" <= I don't think it's good.
# "Illuminate\Database\Events\StatementPrepared" <= I don't think it's good.
# "Illuminate\Database\Events\QueryExecuted" <= I don't think it's good.
# "Illuminate\Database\Events\QueryExecuted" <= I don't think it's good.
"Illuminate\Console\Events\CommandStarting"
"Illuminate\Console\Events\CommandStarting"
"Illuminate\Console\Events\CommandFinished"
"Illuminate\Console\Events\CommandFinished"

If you run cron, this task will run many times (/var/log/syslog)

"bootstrapped: Illuminate\Foundation\Bootstrap\BootProviders"
"Illuminate\Console\Events\ArtisanStarting"
# "eloquent.booting: App\Models\User" <= I don't think it's good.
# "eloquent.booting: App\Models\User" <= I don't think it's good.
# "eloquent.booted: App\Models\User" <= I don't think it's good.
# "eloquent.booted: App\Models\User" <= I don't think it's good.
# "Illuminate\Database\Events\StatementPrepared" <= I don't think it's good.
# "Illuminate\Database\Events\StatementPrepared" <= I don't think it's good.
# "Illuminate\Database\Events\QueryExecuted" <= I don't think it's good.
# "Illuminate\Database\Events\QueryExecuted" <= I don't think it's good.
"Illuminate\Console\Events\CommandStarting"
"Illuminate\Console\Events\CommandStarting"
"Illuminate\Console\Events\CommandFinished"
"Illuminate\Console\Events\CommandFinished"

As you can see, we have created a load that we did not expect. In all practices, variables are initialized in the class constructor. But this place is so unique.
I understand why this is happening. The Illuminate\Foundation\Console\Kernel::load() function is called in the App\Console\Kernel::commands() method.

    /**
     * Register the commands for the application.
     *
     * @return void
     */
    protected function commands(): void
    {
        $this->load(__DIR__.'/Commands'); // <= THIS

        require base_path('routes/console.php');
    }

After running the $this->laravel->make($command) function, the constructor will be called

    protected function parseCommand($command, $parameters)
    {
        ...
        $command = $this->laravel->make($command)->getName(); // <= THIS
        ...
    }
    public function resolve($command)
    {
        return $this->add($this->laravel->make($command)); // <= OR THIS
    }

This is how all commands are initialized
I consider this a good reason to fix

• Example 3

TestCommand.php

class TestCommand extends Command
{
    protected $signature   = 'test_command';

    protected $description = 'Error example';

    public function __construct()
    {
        parent::__construct();
        // I was in a hurry and forgot to remove
        // I needed this for testing or for development
        Auth::loginUsingId(1);
        // From now on, the whole application thinks that the user with id 1 is authorized
    }

    public function handle(): void
    {
        // The task requires it
        User::find(99)->update([ "paid_at" => now() ]);
    }
}

User.php

<?php

namespace App\Models;

use Illuminate\Contracts\Auth\MustVerifyEmail;
use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Foundation\Auth\User as Authenticatable;
use Illuminate\Notifications\Notifiable;

class User extends Authenticatable
{
    use HasFactory;
    use Notifiable;

    public function __construct(array $attributes = [])
    {
        parent::__construct($attributes);
    }

    /**
     * The attributes that are mass assignable.
     *
     * @var array<int, string>
     */
    protected $fillable = [
        'name',
        'email',
        'password',
    ];

    /**
     * The attributes that should be hidden for serialization.
     *
     * @var array<int, string>
     */
    protected $hidden = [
        'password',
        'remember_token',
    ];

    /**
     * The attributes that should be cast.
     *
     * @var array<string, string>
     */
    protected $casts = [
        'email_verified_at' => 'datetime',
    ];

    protected static function boot()
    {
        parent::boot();

        // Or register Observer [ .env QUEUE_CONNECTION=sync ]
        static::updating(
            function (User $user) {

                // Let's pretend I'm using a helper "auth"
                if (auth()->check() /* TRUE */) {

                    // Log::class - changelog
                    Log::create(
                        [
                            "user_id"   => auth()->id(), // THIS ERROR
                            "entity"    => self::class,
                            ...
                        ]
                    );

                }
            }
        );
    }
}

In this task, we record that who made changes to a particular model.
As you can see, we have now received an implicit error. The search for which can take a very long time.

I agree that this behavior is normal for Laravel Framework. But this is very difficult to explain to an inexperienced developer. In my opinion, this could be a security and failover hole. I think this fix will help avoid a lot of bugs.

---

How to use?

1. Run in console

php artisan make:command NameYouCommand

2. Read confirm

Do you want to create a protected command? (yes/no) [no]:

3. Write yes for create Protected command
4. Use new protected console command

New protected console command

<?php

namespace App\Console\Commands;

use Illuminate\Console\Command;
use Illuminate\Console\ProtectedCommand;

/**
* Use "__invoke()" method
* Inject dependencies "__invoke(Application $application)"
*
* This console command is protected.
* To make the current command unprotected change extend class to "Command"
*
* Example: class TestCommand  extends Command {}
* @see Command
*/
class TestCommand extends ProtectedCommand
{
    /**
     * The name and signature of the console command.
     *
     * @var string
     */
    protected $signature = 'test_comand';

    /**
     * The console command description.
     *
     * @var string
     */
    protected $description = 'Command without __construct()';

    /**
     * Execute the console command.
     *
     * @return int
     */
    public function __invoke()
    {
        return 0;
    }
}

[antonkomarev]

Define static class property $defaultName and command would not be instantiated before exact call

[adideas]

Define static class property $defaultName and command would not be instantiated before exact call

I ran a test.

class TestCommand extends Command
{
    protected $signature   = 'test_command';

    protected $description = 'Error example';

    // this not work
    protected static $defaultName = 'test_command';

    public function __construct()
    {
        parent::__construct();
        dd("This code is always executed");
    }

    public function handle(): void
    {
        // TODO Logic
    }
}

In console

> php artisan

< "This code is always executed"

Also the field "$defaultName" is deprecated!

#41555 [9.x] Handle Symfony defaultName deprecation

[GrahamCampbell]

This seems unnecessary to me, and contrary to Laravel's usual approach. Better just to document and educate.

[adideas]

This seems unnecessary to me, and contrary to Laravel's usual approach. Better just to document and educate.

There is an element of learning in the current change.
I didn't say it was necessary. But it will solve the implicit call problem. I'm not talking about encapsulation. There is no information in the documentation about the implicit call.
By default, a regular console command will be created.
Anyone who wants to learn will see the comment I left.

/**
* Use "__invoke()" method
* Inject dependencies "__invoke(Application $application)"
*
* This console command is protected.
* To make the current command unprotected change extend class to "Command"
*
* Example: class {{ class }} extends Command {}
* @see Command
*/

There are 2 examples in the comment.
How to inject dependencies.
How to return to a normal console command.

The goal of a framework is to solve problems, not create vulnerabilities.
Laravel's approach is to help those who know and those who do not know.
My suggestion will help those who have not studied the framework in depth.After all, for this, a framework is needed - so that they don’t think about it.

I will give an example from the java language (Spring). There are also console commands. But they don't have a constructor. And there is no way to replace it. And the constructor is not fired on any other console call. Only on demand.

[driesvints]

Please also revert this.

[dennisprudlo]

I agree with @GrahamCampbell, the fact that there is no empty constructor in the stub anymore (suggesting the put code in there) should be enough. A hint in the docs should be enough if even necessary.

There are a lot of possibilities where writing code can cause unwanted behavior, however laravel doesn't intervenes there as well. I once created a setUpdatedAt method on a model with a $this->save() call in it. I didn't know that this method already existed in eloquent and I got integrity errors all over the place with those models.

[adideas]

I agree with @GrahamCampbell, the fact that there is no empty constructor in the stub anymore (suggesting the put code in there) should be enough. A hint in the docs should be enough if even necessary.

There are a lot of possibilities where writing code can cause unwanted behavior, however laravel doesn't intervenes there as well. I once created a setUpdatedAt method on a model with a $this->save() call in it. I didn't know that this method already existed in eloquent and I got integrity errors all over the place with those models.

I agree too. But it's one thing to write in the constructor in the ServiceProvider and you yourself understand that it will work all the time. The first thought when you write a console command is that it will be executed on demand. But not always, only when necessary. I am not comparing all Laravel objects, but I am saying that it can be confusing at the current location. I can give an example of Laravel entities. When you write code in the model constructor, it only executes when requested.

"A hint in the docs should be enough if even necessary." - There is no such thing in the documentation. Nowhere is it written that console commands work on the principle of ServiceContainer. Until you look in the repository, you think that it works differently.

"I didn't know that this method already existed in eloquent and I got integrity errors all over the place with those models." - In the current situation (Example 2/ Example3) you won't get an error. She will just be.

[adideas]

Worst of all, the console command has the right to modify the application. According to Laravel's logic, this is done through another "ServiceProvider" mechanism. I'm not saying it's bad. On the contrary, I propose a safe mechanism. Which does not break the Laravel system in any way.

Someone who has studied console commands in detail. Easily abandon "ProtectedCommand" in the direction of "Command"

This version of the console command is optional. You have to answer yes. By default, no.