[9.x] Return non-zero exit code for uncaught exceptions

[bert-w]

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

[taylorotwell]

Honestly for a simple change I would just leave all tests unchanged. Just creates more maintenance overhead than it's worth.

[crynobone]

Wouldn't this cause Octane to exit the worker on every uncaught exception, which is not desired?

laravel/octane#654

[bert-w]

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.

[bert-w]

I moved the exit(1) call a few lines up now (inside the $app->runningInConsole() since this is a command line related issue)

[crynobone]

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.

framework/src/Illuminate/Foundation/Application.php

Line 702 in 6316655

$this->isRunningInConsole = Env::get('APP_RUNNING_IN_CONSOLE') ?? (\PHP_SAPI === 'cli' || \PHP_SAPI === 'phpdbg');

[bert-w]

@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 php artisan ... command; the trait MakesArtisanScript is the most straightforward solution to have the correct boilerplate in the test(s).

[bert-w]

@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?

[crynobone]

@bert-w Can you share an artisan command example?

[bert-w]

@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 artisan file for inside the Testbench but it is a bit convoluted that way.

[crynobone]

@bert-w See bert-w#1

[taylorotwell]

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 handleException method is called?

[bert-w]

I don't see how that could be possible since that function is set using set_exception_handler(...). This handleException function thus is only executed on an uncaught exception. PHP says "Execution will stop after the callback is called." (see https://www.php.net/manual/en/function.set-exception-handler.php)

Then once exit(1) is called, only shutdown handlers register_shutdown_function(...) and destructors __destruct() are called (see https://www.php.net/manual/en/function.exit.php).

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 exit() or because of an object dereference shouldnt make a difference.

[macropay-solutions]

@bert-w @taylorotwell @crynobone This PR unintentionally introduced the exit(1) call in a method (handleException) used also for register_shutdown_function, making any other shutdown functions to not be executed.

We discovered this while doing a POC for this issue #58207 (comment)

Question is: Is the exit(1) needed in the php error case? Maybe not.

[bert-w]

@bert-w @taylorotwell @crynobone This PR unintentionally introduced the exit(1) call in a method (handleException) used also for register_shutdown_function, making any other shutdown functions to not be executed.

We discovered this while doing a POC for this issue #58207 (comment)

Question is: Is the exit(1) needed in the php error case? Maybe not.

I thought about this for a moment: according to the manual https://www.php.net/manual/en/function.register-shutdown-function.php a shutdown handler can register its own shutdown handler as well:

Shutdown functions may also call register_shutdown_function() themselves to add a shutdown function to the end of the queue.

So perhaps instead of calling exit(1) we can register a new shutdown handler that gets pushed to the end of the stack and that one calls exit(1). But it feels somewhat odd...

[marius-ciclistu]

@bert-w that works, but is it needed in the fatal error case?