Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BuildCommand::run() does not return exit code #432

Merged
merged 1 commit into from Jan 11, 2021
Merged

BuildCommand::run() does not return exit code #432

merged 1 commit into from Jan 11, 2021

Conversation

@CupOfTea696
Copy link
Contributor

@CupOfTea696 CupOfTea696 commented Jan 10, 2021

BuildCommand::run() should return the command exit code, but instead returns void. This violates the return type of the parent class.

`BuildCommand::run()` should return the command exit code, but instead returns void. This violates the return type of the parent class.
@nunomaduro
Copy link
Member

@nunomaduro nunomaduro commented Jan 10, 2021

This actually makes the code fail?

Loading

@CupOfTea696
Copy link
Contributor Author

@CupOfTea696 CupOfTea696 commented Jan 10, 2021

Any code that expects a return value from BuildCommand::run() would fail, yes. For example, if one were to use an event dispatcher on the console application, Symfony\Component\Console\Application::doRunCommand() will generate an exception for trying to construct Symfony\Component\Console\Event\ConsoleTerminateEvent without an exit code.

Loading

@nunomaduro nunomaduro requested a review from owenvoke Jan 10, 2021
@nunomaduro
Copy link
Member

@nunomaduro nunomaduro commented Jan 10, 2021

Gonna actually let @owenvoke validate this one.

Loading

Copy link
Member

@owenvoke owenvoke left a comment

This is true that the run() method should return an integer exit code. I wonder why this wasn't picked up by PhpStorm or PHPStan for me. 🤔

https://github.com/symfony/symfony/blob/665222cf4dde7da5d27931eb18a5c8b244119797/src/Symfony/Component/Console/Command/Command.php#L264

(Oh, PHPStan only displays the issue on level 2, perhaps it'd be worth trying to update the PHPStan levels in future?)

Loading

@owenvoke owenvoke merged commit f54e1f8 into laravel-zero:master Jan 11, 2021
9 checks passed
Loading
@nunomaduro
Copy link
Member

@nunomaduro nunomaduro commented Jan 11, 2021

@owenvoke Indeed.

Loading

@CupOfTea696 CupOfTea696 deleted the patch-1 branch Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants