Skip to content

[5.5] Force handle function for commands#20024

Merged
taylorotwell merged 1 commit into
laravel:masterfrom
laurencei:master
Jul 13, 2017
Merged

[5.5] Force handle function for commands#20024
taylorotwell merged 1 commit into
laravel:masterfrom
laurencei:master

Conversation

@laurencei

@laurencei laurencei commented Jul 12, 2017

Copy link
Copy Markdown
Contributor

Recently PR #19827 was pulled into 5.5. This PR changes all the core artisan console commands to always use handle() instead of fire().

While this doesnt seem like a big change - it will have unintended on all packages or classes that do any sort of extension of a core artisan command. Further - it could actually be a silent failure on upgrades in certain situations.

Consider this pseudo artisan command scenario:

Class CoreInspire
{
    public function fire()
    {
         echo 'Original';
    }
}

Class MyInspire extends CoreInspire
{
     public function fire()
     {
           echo 'New';
     }
}

In Laravel <=5.4 you would get New printed on the screen as expected.

But in Laravel 5.5 - you will now get Original with no errors. That is because fire() has been renamed to handle() in the CoreInspire (example) class, and calls to function handle() take priority.

Obviously you can change all your packages/classes that extend a core artisan command to now use handle() - but at the moment it is possible they will silently stop being extended (depending on the quality of your test suites).

If we are going to go down this route - then we might as well remove any reference to fire() in the base artisan console command class - and have it explicitly listed as a breaking change in the 5.5 upgrade path to rename any artisan commands containing fire() to now be handle().

The alternative is we leave it to accept both fire() and handle() - but then if we are doing that, what is the point of having this breaking change in some situations in the first place? Either we should go all the way, or not at all IMO.

@laurencei

Copy link
Copy Markdown
Contributor Author

Ping @lucasmichot @browner12

@browner12

Copy link
Copy Markdown
Contributor

I agree, should remove.

@taylorotwell
taylorotwell merged commit ac9f29d into laravel:master Jul 13, 2017
@laurencei
laurencei deleted the master branch July 13, 2017 09:26
daftspunk added a commit to octobercms/library that referenced this pull request Jul 14, 2017
@barryvdh

Copy link
Copy Markdown
Contributor

Why not deprecate it, put it in the upgrade guide for 5.5 and remove it in 5.6? Like proposed here: laravel/ideas#383

@lucasmichot

Copy link
Copy Markdown
Contributor

See related #20342 for jobs and queues

@Cayan

Cayan commented Mar 5, 2018

Copy link
Copy Markdown

This broke new relic support to monitor workers on APM.

I contacted them and their solution was a patch to add it back lol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants