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

CommentFileSizeAuthor
#6 3526515-nr-bot.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3526515

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mstrelan created an issue. See original summary.

mstrelan’s picture

Status: Active » Needs review
mondrake’s picture

Added inline comments

mstrelan’s picture

Thanks 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.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new91 bytes

The 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.

mstrelan’s picture

Title: Use get_error_handler() in php 8.5 » Use get_error_handler() in php 8.5 and deprecate Error::currentErrorHandler
Issue summary: View changes
Status: Needs work » Needs review

Rebased and deprecated \Drupal\Core\Utility\Error::currentErrorHandler. Let's leave fixing up set_error_handler calls to another issue so we can consolidate similar callbacks and refine them there. We may need to fix _drupal_error_handler_real to return a bool as per the expected method signature.

mstrelan’s picture

In fact let's use #3000229: Move error handlers to an Error class to fix the set_error_handler calls

nicxvan’s picture

Do we need to do a dependency evaluation since this adds the polyfill.

Or is it different since this is a symfony library?

nicxvan’s picture

Just 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.

mstrelan’s picture

Do we need to do a dependency evaluation since this adds the polyfill.

Or is it different since this is a symfony library?

I think most symfony libraries would still need one, but not polyfills for php versions. We didn't for polyfill-php84 in #3523284: Update remaining Composer dependencies for 11.2.0 nor for polyfill-php83 in #3413268: Add PHP 8.3 requirement to Drupal 11.0.x.

Just to make sure I understand this only replaced the instances where the error handler is swapped only to find the current handler.

I replaced all calls to set_error_handler that used the result, to use get_error_handler instead. In some cases the callback passed to set_error_handler only 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.

nicxvan’s picture

Thank 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!

nicxvan’s picture

Status: Needs review » Reviewed & tested by the community

@catch confirmed on slack that polyfills can just be added and removed as needed.

larowlan credited catch.

larowlan’s picture

Credits

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

We'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

mstrelan’s picture

Status: Needs work » Reviewed & tested by the community

Rebased and regenerated the lock file

  • larowlan committed 3023ed6d on 11.x
    Issue #3526515 by mstrelan, nicxvan, mondrake, catch: Use...

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 11.x and published the change record

xjm’s picture

d.o ate Lee's credit. Fixing.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.