PHP 8.4 | Text_Diff::_check(): fix trigger_error() with E_USER_ERROR is deprecated (Trac 62061) #7375
Conversation
This commit adds a simple test for the `Text_Diff::_check()` method and as that test could never pass with the code as-is, it also fixes the bug discoved by this test. Bug symptom: `Error: Class name must be a valid object or a string` on line 279.
…is deprecated
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`.
Considering this is an unmaintained external dependency, I'm propose we fix this in the WP specific copy of the dependency.
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)`.
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.
This makes for the most elegant solution with the least BC-breaking impact.
This commit implements the third option. It introduces a new `Text_Exception` class and starts using that in the `Text_Diff::_check()` method in all applicable places.
It also adds tests for the first two error conditions.
Unfortunately, I have not been able to get a test set up to test the other three exceptions, as without hacking the object under test, i.e. manually editing the `$_edits` property between the instantiating the object and running the `_check()`, I have not found a way to trigger those exceptions.
Ref:
* 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
…ROR is deprecated 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`. Considering this is an unmaintained external dependency, I'm propose we fix this in the WP specific copy of the dependency. In this case, the `trigger_error()` call looks to be a remnant of the PHP 4 era before a class could be declared as `abstract`, so I've fixed this in the most straight-forward manner: by making both the method as well as the class `abstract` and removing the call to `trigger_error()`.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
| @@ -276,7 +276,7 @@ function _check($from_lines, $to_lines) | |||
|
|
|||
| $prevtype = null; | |||
| foreach ($this->_edits as $edit) { | |||
| if ($edit instanceof $prevtype) { | |||
| if ($prevtype !== null && $edit instanceof $prevtype) { | |||
There was a problem hiding this comment.
Yes, this is a needed defensive change to protect against throwing a fatal error.
Since PHP 5, if $prevtype is null, the following fatal error is thrown:
{{{
Fatal error: Uncaught Error: Class name must be a valid object or a string
}}}
There was a problem hiding this comment.
Created https://core.trac.wordpress.org/ticket/62083 for this specific bugfix.
There was a problem hiding this comment.
Committed via https://core.trac.wordpress.org/changeset/59070 ✅
|
@hellofromtonya Would you like me to rebase to get rid of the merge conflicts ? |
Thanks for offering @jrfnl. But it's not necessary. I'm going commit by commit. |
|
Fixing |
There was a problem hiding this comment.
Text_Diff_Op:reverse() fix 1d059a3 LGTM and is ready for commit ✅
|
Fixed With all commits now committed, closing this PR. Thank you @jrfnl! |
|
Thanks @hellofromtonya! |
Text_Diff::_check(): add basic test + fix a bug
This commit adds a simple test for the
Text_Diff::_check()method and as that test could never pass with the code as-is, it also fixes the bug discoved by this test.Bug symptom:
Error: Class name must be a valid object or a stringon line 279.PHP 8.4 | Text_Diff::_check(): fix trigger_error() with E_USER_ERROR is deprecated
PHP 8.4 deprecates the use of
trigger_errror()withE_USER_ERRORas the error level, as there are a number of gotchas to this way of creating aFatal Error(finallyblocks not executing, destructors not executing).The recommended replacements are either to use exceptions or to do a hard
exit.Considering this is an unmaintained external dependency, I'm propose we fix this in the WP specific copy of the dependency.
Now, there were basically three options:
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.
exit($status).This would make the code untestable and would disable handling of these errors via custom error handlers, which makes this an undesirable solution.
This makes for the most elegant solution with the least BC-breaking impact.
This commit implements the third option. It introduces a new
Text_Exceptionclass and starts using that in theText_Diff::_check()method in all applicable places.It also adds tests for the first two error conditions.
Unfortunately, I have not been able to get a test set up to test the other three exceptions, as without hacking the object under test, i.e. manually editing the
$_editsproperty between the instantiating the object and running the_check(), I have not found a way to trigger those exceptions.Ref:
PHP 8.4 | Text_Diff_Op::reverse(): fix trigger_error() with E_USER_ERROR is deprecated
PHP 8.4 deprecates the use of
trigger_errror()withE_USER_ERRORas the error level, as there are a number of gotchas to this way of creating aFatal Error(finallyblocks not executing, destructors not executing).The recommended replacements are either to use exceptions or to do a hard
exit.Considering this is an unmaintained external dependency, I'm propose we fix this in the WP specific copy of the dependency.
In this case, the
trigger_error()call looks to be a remnant of the PHP 4 era before a class could be declared asabstract, so I've fixed this in the most straight-forward manner: by making both the method as well as the classabstractand removing the call totrigger_error().Trac ticket: https://core.trac.wordpress.org/ticket/62061
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.