Problem/Motivation

RFC accepted https://wiki.php.net/rfc/deprecations_php_8_4#deprecate_passing_e_user_e...

Steps to reproduce

core$ git grep E_USER_ERROR
core/includes/errors.inc:34:    E_USER_ERROR => ['User error', RfcLogLevel::ERROR],
core/includes/errors.inc:70:    $to_string = $error_level == E_USER_ERROR && str_ends_with($caller['function'], '__toString()');
core/lib/Drupal/Component/Diff/DiffFormatter.php:137:        trigger_error('Unknown edit type', E_USER_ERROR);
core/lib/Drupal/Component/Utility/ToStringTrait.php:20:      trigger_error(get_class($e) . ' thrown while calling __toString on a ' . static::class . ' object in ' . $e->getFile() . ' on line ' . $e->getLine() . ': ' . $e->getMessage(), E_USER_ERROR);
core/lib/Drupal/Component/Utility/ToStringTrait.php:22:      // E_USER_ERROR, we terminate execution. However, for test purposes allow
core/modules/big_pipe/src/Render/BigPipe.php:375:          trigger_error($e, E_USER_ERROR);
core/modules/big_pipe/src/Render/BigPipe.php:411:          trigger_error($e, E_USER_ERROR);
core/modules/big_pipe/src/Render/BigPipe.php:562:            trigger_error($e, E_USER_ERROR);
core/tests/Drupal/Tests/Core/StringTranslation/TranslatableMarkupTest.php:82:    $this->assertEquals(E_USER_ERROR, $this->lastErrorNumber);

Proposed resolution

clean-up usage or make it conditional

- ToStringTrait is deprecated as any exception still will be caught by error hadler
- BigPipe using it instead of properly logging errors

This technique is used to log errors mostly in core because Drupal does not stop execution for user-level errors, allowing the application to log and continue. Maybe it needs changes but it's out of scope of this issue

Remaining tasks

- file MR and change record
- commit

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3465827

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:

About issue forks

Comments

andypost created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

ToStringTrait might be tricky, because exceptions and __toString() methods don't like each other, unless that has changed somehow?

andypost’s picture

andypost
he/him
Primary language Russian
commented

And the suggestion in RFC is to use proper exceptions

Using exceptions instead solves all the above problems, and allows catching the error outside the problematic code path.

If the desired outcome is to terminate the program with no possible way to recover one should use the exit() function with a string argument.

Therefore we propose to deprecate passing E_USER_ERROR to trigger_error()

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#2562487: Get better error reporting from __toString
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Looks like exception handling in __toString() methods got fixed in PHP 7.4: https://3v4l.org/5ZJ2i. That means we can drop the try/catch and even consider to deprecate that trait? It's only used in two classes in core.

andypost opened merge request !9085

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review

Yes, just 2 implementations to add, and deprecated, now needs to clean-up _drupal_error_handler_real(), maybe better to split out fixes for the remaining usage for scope

andypost’s picture

andypost
he/him
Primary language Russian
commented

@Berdir thank you! it works and the only test needs to be adjusted 

Drupal\Tests\Core\StringTranslation\TranslatableMarkupTest::testToString
Failed asserting that null matches expected 256.

core/tests/Drupal/Tests/Core/StringTranslation/TranslatableMarkupTest.php:77
andypost’s picture

andypost
he/him
Primary language Russian
commented

There's just 4 usages in contrib, so needs to improve message and decide with test, polish CR

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +blocker

blocker for PHP 8.4 adoption

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +Needs change record updates

Probably deprecation of ToStringTrait better to move to separate issue or it needs second change record

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes

Added to summary why most of time it needs to decide if code needs exception or logging instead of throwing user error as everyone thinks it should be fatal but there's no exit() or exception re-thrown in handler

Drupal does not stop execution for user-level errors, allowing the application to log and continue

catch’s picture

catch
he/him
Primary language English
commented

I think for 10.4/11.1 we should change all these to E_USER_WARNING and then open follow-ups for each case to consider adding:

1. an assert() so it fails hard during development
2. An exception from 12.x onwards if that's appropriate

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#3304746: BigPipe cannot handle (GET) form redirects (EnforcedResponseException)

Related required for BigPipe fix

kim.pepper made their first commit to this issue’s fork.

kim.pepper opened merge request !9924

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

Created an MR for #13

andypost’s picture

andypost
he/him
Primary language Russian
commented

Some failures looks fixable

arunkumark made their first commit to this issue’s fork.

andypost’s picture

andypost
he/him
Primary language Russian
commented

MR!9924 is green now, so it needs follow-up to deprecate ToStringTrait

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

I think we should go with https://git.drupalcode.org/project/drupal/-/merge_requests/9924 and then open a follow-up to decide what to do with those individual cases, possibly based off the other MR.

Could the other MR be closed here and the issue summary updated?

andypost’s picture

andypost
he/him
Primary language Russian
commented

Official migration docs said different

Calling trigger_error() with error_level being equal to E_USER_ERROR is now deprecated.

Such usages should be replaced by either throwing an exception, or calling exit(), whichever is more appropriate.

Moreover exit() (die() too) now a userland function

Ref https://www.php.net/manual/en/migration84.incompatible.php#migration84.i...

catch’s picture

catch
he/him
Primary language English
commented

The migration docs might say different but we are often dealing with extreme edge cases where we have a choice between failing hard for end users vs informing site owners, this is why I would go with E_USER_WARNING here then a followup for switching to exceptions or whatever else we want to do.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs change record updates, -Needs issue summary update

Filed follow-up #3490454: [META] Replace E_USER_WARNING with exception handling for edge cases in error reporting

Updated summary and CR

andypost’s picture

andypost
he/him
Primary language Russian
commented

There's green 8.4 pipeline using !9924 as patch

andypost opened merge request !10379

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +needs backport to 10.x

it needs more work for 10.4.x/10.5.x

andypost’s picture

andypost
he/him
Primary language Russian
commented

10.5 failed one unit test https://git.drupalcode.org/issue/drupal-3427903/-/jobs/3523954

---- Drupal\Tests\Core\Database\ConditionTest ----
Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Fail      Other      phpunit-77.xml       0 Drupal\Tests\Core\Database\Conditio
    PHPUnit Test failed to complete; Error: PHPUnit 9.6.21 by Sebastian
    Bergmann and contributors.
    
    Testing Drupal\Tests\Core\Database\ConditionTest
    .......................                                           23 / 23
    (100%)
    
    Time: 00:00.120, Memory: 8.00 MB
    
    OK (23 tests, 33 assertions)
    
    Remaining self deprecation notices (4)
    
      1x: Invalid characters in query operator: IS NOT NULL) ;INSERT INTO
    {test} (name) VALUES ('test12345678'); -- 
        1x in ConditionTest::testCompileWithSqlInjectionForOperator from
    Drupal\Tests\Core\Database
    
      1x: Invalid characters in query operator: IS NOT NULL) UNION ALL SELECT
    name, pass FROM {users_field_data} -- 
        1x in ConditionTest::testCompileWithSqlInjectionForOperator from
    Drupal\Tests\Core\Database
    
      1x: Invalid characters in query operator: IS NOT NULL) UNION ALL SELECT
    name FROM {TEST_UPPERCASE} -- 
        1x in ConditionTest::testCompileWithSqlInjectionForOperator from
    Drupal\Tests\Core\Database
    
      1x: Invalid characters in query operator: = 1 UNION ALL SELECT password
    FROM user WHERE uid =
        1x in ConditionTest::testCompileWithSqlInjectionForOperator from
    Drupal\Tests\Core\Database
andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes

Passed as addition https://git.drupalcode.org/project/drupal/-/merge_requests/10379/diffs?c...

andypost’s picture

andypost
he/him
Primary language Russian
commented

both 10.4/10.5 passed https://git.drupalcode.org/issue/drupal-3427903/-/pipelines/353325

quietone’s picture

quietone commented
Status: Needs review » Reviewed & tested by the community

The issue summary proposed resolution is incorrect in that the changes for - ToStringTrait and BigPipe are being done in a separate issue, #3490454: [META] Replace E_USER_WARNING with exception handling for edge cases in error reporting. And I see that all the needed followups have been made.

I reviewed the MR, and with the scope change it is much more straightforward so setting to RTBC. I applied the patch locally and all instances were changed on 10.5.x and 11.x

I read the change record and it is good to go. The versions numbers will likely need an update but that can be done on commit or I will catch it when I review the change records.

  • catch committed 1c764a35 on 11.1.x 
    Issue #3465827 by andypost, kim.pepper, arunkumark, catch, berdir,...

  • catch committed 1bf85911 on 11.x 
    Issue #3465827 by andypost, kim.pepper, arunkumark, catch, berdir,...

catch closed merge request !9924

  • catch committed dcdc50ac on 10.4.x 
    Issue #3465827 by andypost, kim.pepper, arunkumark, catch, berdir,...

  • catch committed 3e036033 on 10.5.x 
    Issue #3465827 by andypost, kim.pepper, arunkumark, catch, berdir,...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed/cherry-picked to 11.x/11.1.x and 10.5.x/10.4.x, respectively. Let's continue in the follow-ups. Thanks everyone.

catch closed merge request !10379

Status: Fixed » Closed (fixed)

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

andypost’s picture

andypost
he/him
Primary language Russian
commented

Fatal errors in 8.5 can have backtrace https://wiki.php.net/rfc/error_backtraces_v2

andypost closed merge request !9085