-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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
[9.x] Return non-zero exit code for uncaught exceptions #46541
Conversation
Honestly for a simple change I would just leave all tests unchanged. Just creates more maintenance overhead than it's worth. |
} | ||
|
||
if (static::$app->runningInConsole()) { | ||
$this->renderForConsole($e); | ||
} else { | ||
$this->renderHttpResponse($e); | ||
} | ||
|
||
if ($uncaught) { | ||
exit(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this cause Octane to exit the worker on every uncaught exception, which is not desired?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good question. That could be the case yes.
Anyhow, there is no way to say to php "exit with exitCode=1 on your own accord (when its time to exit)". AFAIK you must call exit(1)
yourself which will then still call any remaining __destruct()
listeners and shutdown handlers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the exit(1)
call a few lines up now (inside the $app->runningInConsole()
since this is a command line related issue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Octane workers still will have $app->runningInConsole() === true
since it is an artisan command.
Octane worker emits APP_RUNNING_IN_CONSOLE=false
so should be okay.
$this->isRunningInConsole = Env::get('APP_RUNNING_IN_CONSOLE') ?? (\PHP_SAPI === 'cli' || \PHP_SAPI === 'phpdbg'); |
@taylorotwell I prefer to leave the test in since the behaviour does need to be tested. If you have a faster/cleaner way to test this with less code then by all means :) One way or another you need to call an external process with a |
@crynobone do you have any idea on how to rewrite the testcase using the testbench such that I can test the exitcode for running an artisan command? |
@bert-w Can you share an artisan command example? |
@crynobone in essence doing the following: /** @var \Illuminate\Foundation\Console\Kernel $kernel */
$exitCode = $kernel->call('throw-exception-command'); However since the command is meant to throw and I want to test the exitcode, this call must happen as a separate process or else the testcase itself will throw the exception. This PR currently includes a wrapper to create an explicit |
Signed-off-by: Mior Muhammad Zaki <[email protected]>
Use `PhpProcess` to run PHP script in isolation to verify `exit()`
Is there any way an application could enter this section of code without being in an unrecoverable / fatal error state? In other words, is this going to break any applications that could have possible kept executing code after this |
I don't see how that could be possible since that function is set using Then once The only destructor being called after this in a default Laravel app is the Monolog handler itself, but whether it is called because of an |
This is a fix for #46306.
Any uncaught exceptions inside the exception handler now explicity exit with code 1, indicating an error state. A test has been included to test for the correct exit code in case of an uncaught exception and in the case of successful execution.
EDIT: thanks to Crynobone for cleaning up the testcase to work neatly with the testbench