Stop passing E_USER_ERROR to trigger_error() on PHP 8.4

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

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()

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.

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

@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

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

blocker for PHP 8.4 adoption

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

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 exit() or exception re-thrown

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

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

Related required for BigPipe fix

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

Created an MR for #13

Some failures looks fixable

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

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?

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

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.

Filed follow-up 🌱 [META] Replace E_USER_WARNING with Exception Handling for Edge Cases in Error Reporting Active

Updated summary and CR

There's green 8.4 pipeline using !9924 as patch

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

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

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

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

The issue summary proposed resolution is incorrect in that the changes for - ToStringTrait and BigPipe are being done in a separate issue, 🌱 [META] Replace E_USER_WARNING with Exception Handling for Edge Cases in Error Reporting Active . 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 → closed merge request !9924

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

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