[5.5] Force handle function for commands #20024
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Recently PR #19827 was pulled into 5.5. This PR changes all the core artisan console commands to always use
handle()
instead offire()
.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:
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 becausefire()
has been renamed tohandle()
in theCoreInspire
(example) class, and calls to functionhandle()
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 containingfire()
to now behandle()
.The alternative is we leave it to accept both
fire()
andhandle()
- 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.