Skip to content

Commit

Permalink
Code Modernization: Fix trigger_error() with E_USER_ERROR deprecation…
Browse files Browse the repository at this point in the history
… in wp_trigger_error().

PHP 8.4 deprecates the use of `trigger_errror()` with `E_USER_ERROR` as the error level, as there are a number of gotchas to this way of creating a `Fatal Error` (`finally` blocks not executing, destructors not executing). The recommended replacements are either to use exceptions or to do a hard `exit`.

WP has its own `wp_trigger_error()` function, which under the hood calls `trigger_error()`. If passed `E_USER_ERROR` as the `$error_level`, this will hit the PHP 8.4 deprecation.

Now, there were basically three options:
* Silence the deprecation until PHP 9.0 and delay properly solving this until then. This would lead to an awkward solution, as prior to PHP 8.0, error silencing would apply to all errors, while, as of PHP 8.0, it will no longer apply to fatal errors. It also would only buy us some time and wouldn't actually solve anything.

* Use `exit($status)` when `wp_trigger_error()` is called with `E_USER_ERROR`. This would make the code untestable and would disable handling of these errors via custom error handlers, which makes this an undesirable solution.

* Throw an exception when `wp_trigger_error()` is called with `E_USER_ERROR`. This makes for the most elegant solution with the least BC-breaking impact, though it does open it up to the error potential being "caught" via a `try-catch`. That's not actually a bad thing and is likely to only happen for those errors which can be worked around, in which case, it's a bonus that that's now possible.

The third option is implemented which:
* Introduces a new `WP_Exception` class.
* Starts using `WP_Exception` in the `wp_trigger_error()` function when the `$error_level` is set to `E_USER_ERROR`.

This change is covered by pre-existing tests, which have been updated to expect the exception instead of a PHP error.

Why not use `WP_Error`?

Well, for one, this would lead to completely different behaviour (BC).

As `WP_Error` doesn't extend `Exception`, the program would not be stopped, but would continue running, which would be a much bigger breaking change and carries security risks. `WP_Error` also doesn't natively trigger displaying/logging of the error message, so in that case, it would still need an `exit` with the error message, bringing us back to point 2 above.

Introducing `WP_Exception` provides (essentially) the same behaviour in that it retains the fatal error and error message displaying/logging behaviors. It also introduces a base Exception class, from which future exception classes can extend.

References:
* https://wiki.php.net/rfc/deprecations_php_8_4#deprecate_passing_e_user_error_to_trigger_error
* https://www.php.net/manual/en/migration80.incompatible.php

Follow-up to [56530].

Props jrf, hellofromTonya.
See #62061.

git-svn-id: https://develop.svn.wordpress.org/trunk@59107 602fd350-edb4-49c9-b593-d223f7449a82
  • Loading branch information
hellofromtonya committed Sep 27, 2024
1 parent d9fe74b commit b0e35ec
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 3 deletions.
16 changes: 16 additions & 0 deletions src/wp-includes/class-wp-exception.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?php
/**
* WP_Exception class
*
* @package WordPress
* @since 6.7.0
*/

/**
* Core base Exception class.
*
* Future, more specific, Exceptions should always extend this base class.
*
* @since 6.7.0
*/
class WP_Exception extends Exception {}
4 changes: 4 additions & 0 deletions src/wp-includes/functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -6092,6 +6092,10 @@ function wp_trigger_error( $function_name, $message, $error_level = E_USER_NOTIC
array( 'http', 'https' )
);

if ( E_USER_ERROR === $error_level ) {
throw new WP_Exception( $message );
}

trigger_error( $message, $error_level );
}

Expand Down
1 change: 1 addition & 0 deletions src/wp-settings.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@

// Include files required for initialization.
require ABSPATH . WPINC . '/class-wp-paused-extensions-storage.php';
require ABSPATH . WPINC . '/class-wp-exception.php';
require ABSPATH . WPINC . '/class-wp-fatal-error-handler.php';
require ABSPATH . WPINC . '/class-wp-recovery-mode-cookie-service.php';
require ABSPATH . WPINC . '/class-wp-recovery-mode-key-service.php';
Expand Down
6 changes: 3 additions & 3 deletions tests/phpunit/tests/functions/wpTriggerError.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ class Tests_Functions_WpTriggerError extends WP_UnitTestCase {
* @param string $message The message to test.
* @param string $expected_message The expected error message.
*/
public function test_should_trigger_error( $function_name, $message, $expected_message ) {
$this->expectError();
$this->expectErrorMessage( $expected_message );
public function test_should_throw_exception( $function_name, $message, $expected_message ) {
$this->expectException( WP_Exception::class );
$this->expectExceptionMessage( $expected_message );

wp_trigger_error( $function_name, $message, E_USER_ERROR );
}
Expand Down

0 comments on commit b0e35ec

Please sign in to comment.