Problem/Motivation
PHP 8.5 introduces get_error_handler. Before this, the only way to get the handler was to set it to something else, then restore it.
Steps to reproduce
See \Drupal\Core\Utility\Error::currentErrorHandler:
public static function currentErrorHandler(): ?callable {
$currentHandler = set_error_handler('var_dump');
restore_error_handler();
return $currentHandler;
}
There are a few other places that use set_error_handler to get the current handler, but they are also intentionally setting the error handler at the same time. It would probably be clearer to use get_error_handler first.
$ grep -r " = set_error_handler" core
core/lib/Drupal/Core/Utility/Error.php: $currentHandler = set_error_handler('var_dump');
core/lib/Drupal/Core/Render/ElementInfoManager.php: $previous_error_handler = set_error_handler(function ($severity, $message, $file, $line) use (&$previous_error_handler) {
core/tests/Drupal/KernelTests/Core/Render/Element/DeprecatedElementTest.php: $previous_error_handler = set_error_handler(function ($severity, $message, $file, $line) use (&$previous_error_handler) {
core/tests/Drupal/KernelTests/Core/Render/Element/DeprecatedElementTest.php: $previous_error_handler = set_error_handler(function ($severity, $message, $file, $line) use (&$previous_error_handler) {
core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryTest.php: $previous_error_handler = set_error_handler(function ($severity, $message, $file, $line) use (&$previous_error_handler) {
Proposed resolution
- Add symfony/polyfill-php85
- Update above instances to use
get_error_handler
- Deprecate
\Drupal\Core\Utility\Error::currentErrorHandler
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Comments
Comment #3
mstrelan commentedComment #4
mondrakeAdded inline comments
Comment #5
mstrelan commentedThanks for the review. I initially thought about addressing the missing return and the type hints but figured it was out of scope, since arguably we don't necessarily need to change the existing set_error_handler calls in the first place. The reference would make sense to address though. If we are going to refactor the anonymous function it would probably make sense to make it a static method on the Error class, since we're repeating it 4 times.
Comment #6
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #7
mstrelan commentedRebased and deprecated
\Drupal\Core\Utility\Error::currentErrorHandler. Let's leave fixing upset_error_handlercalls to another issue so we can consolidate similar callbacks and refine them there. We may need to fix_drupal_error_handler_realto return a bool as per the expected method signature.Comment #8
mstrelan commentedIn fact let's use #3000229: Move error handlers to an Error class to fix the set_error_handler calls
Comment #9
nicxvan commentedDo we need to do a dependency evaluation since this adds the polyfill.
Or is it different since this is a symfony library?
Comment #10
nicxvan commentedJust to make sure I understand this only replaced the instances where the error handler is swapped only to find the current handler.
I still have to pull down and confirm it's complete.
Comment #11
mstrelan commentedI think most symfony libraries would still need one, but not polyfills for php versions. We didn't for
polyfill-php84in #3523284: Update remaining Composer dependencies for 11.2.0 nor forpolyfill-php83in #3413268: Add PHP 8.3 requirement to Drupal 11.0.x.I replaced all calls to
set_error_handlerthat used the result, to useget_error_handlerinstead. In some cases the callback passed toset_error_handleronly existed so we could get the result. In other cases we were setting the error handler to something else, so we now get and set the handler in separate operations.Comment #12
nicxvan commentedThank you, I asked for confirmation in slack on the dependency.
Other than that I grepped for " = set_error_handler" and confirmed that all items were addressed.
I also confirmed that all instances of currentErrorHandler were replaced and the deprecation looks right.
Once the dependency question is resolved I think this is ready!
Comment #13
nicxvan commented@catch confirmed on slack that polyfills can just be added and removed as needed.
Comment #15
larowlanCredits
Comment #16
larowlanWe'll need a new hash in the composer.lock after #3532698: phpstan dev constraints make it difficult for modules to support 11.2 and below - fine to self RTBC after that
Comment #17
mstrelan commentedRebased and regenerated the lock file
Comment #20
larowlanCommitted to 11.x and published the change record
Comment #21
xjmd.o ate Lee's credit. Fixing.