Add DrupalErrorHandler wrapping Symfony ErrorHandler component

Giving this a more specific title.

1.    public static function handler($error_level, $message, $filename = NULL, $line = NULL, $context = NULL) {
        if ($error_level & error_reporting()) {
          $types = static::levels();
   -      list($severity_msg, $severity_level) = $types[$error_level];
   +      if (isset($types[$error_level]) {
   +        list($severity_msg, $severity_level) = $types[$error_level];
   +      }
   +      else {
   +        // Default values.
   +        $severity_msg = 'Unknown error';
   +        $severity_level = RfcLogLevel::ERROR;
   +      } 
          $backtrace = debug_backtrace();
          $caller = static::getLastCaller($backtrace);
    
          // We treat recoverable errors as fatal.
          $recoverable = $error_level == E_RECOVERABLE_ERROR;
          // As __toString() methods must not throw exceptions (recoverable errors)
          // in PHP, we allow them to trigger a fatal error by emitting a user error
          // using trigger_error().
          $to_string = $error_level == E_USER_ERROR && substr($caller['function'], -strlen('__toString()')) == '__toString()';
          static::log([
   -        '%type' => isset($types[$error_level]) ? $severity_msg : 'Unknown error',
   +        '%type' => $severity_msg,
            // The standard PHP error handler considers that the error messages
            // are HTML. We mimick this behavior here.
            '@message' => Markup::create(Xss::filterAdmin($message)),
            '%function' => $caller['function'],
            '%file' => $caller['file'],
            '%line' => $caller['line'],
            'severity_level' => $severity_level,
            'backtrace' => $backtrace,
            '@backtrace_string' => (new \Exception())->getTraceAsString(),
          ], $recoverable || $to_string);
   

   Set default values to $severity_msg and $severity_level at begin of the function.

2. -      $error_level = isset($GLOBALS['config']['system.logging']['error_level']) ? $GLOBALS['config']['system.logging']['error_level'] : ERROR_REPORTING_HIDE;
   +      $error_level = $GLOBALS['config']['system.logging']['error_level'] ?? ERROR_REPORTING_HIDE;
   

   More compact (https://wiki.php.net/rfc/isset_ternary)

I think this looks great, but I can't do the required framework manager review.

Looks good, some minor feedback
It also needs deprecation test

1. +++ b/core/lib/Drupal/Core/Utility/Error.php
   @@ -186,4 +189,367 @@ public static function formatBacktrace(array $backtrace) {
   +  public static function levels() {
   +    $types = [
   +      E_ERROR => ['Error', RfcLogLevel::ERROR],
   ...
   +    return $types;
   

   no reason in intermediate variable - just return array

2. +++ b/core/lib/Drupal/Core/Utility/Error.php
   @@ -186,4 +189,367 @@ public static function formatBacktrace(array $backtrace) {
   +    elseif (DRUPAL_TEST_IN_CHILD_SITE && $error_level === E_USER_DEPRECATED) {
   ...
   +    if (DRUPAL_TEST_IN_CHILD_SITE && !headers_sent() && (!defined('SIMPLETEST_COLLECT_ERRORS') || SIMPLETEST_COLLECT_ERRORS)) {
   

   I'd better add `defined('DRUPAL_TEST_IN_CHILD_SITE')` to condition

3. +++ b/core/lib/Drupal/Core/Utility/Error.php
   @@ -186,4 +189,367 @@ public static function formatBacktrace(array $backtrace) {
   +  public static function log(array $error, $fatal = FALSE) {
   +    $is_installer = drupal_installation_attempted();
   

   that makes me think about this function still should be loaded to make this class selfcontained, also a lot of services accesses via \Drupal::

4. +++ b/core/lib/Drupal/Core/Utility/Error.php
   @@ -186,4 +189,367 @@ public static function formatBacktrace(array $backtrace) {
   +      $error_level = isset($GLOBALS['config']['system.logging']['error_level']) ? $GLOBALS['config']['system.logging']['error_level'] : ERROR_REPORTING_HIDE;
   

   Use ?: instead of isset()

+++ b/core/tests/Drupal/Tests/Core/Utility/LegacyErrorTest.php
@@ -0,0 +1,104 @@
+class LegacyErrorTest extends UnitTestCase {
...
+   */
+  protected function setUp() {
+    parent::setUp();
+    include_once $this->root . '/core/includes/bootstrap.inc';
+    include_once $this->root . '/core/includes/errors.inc';
+  }

I don't think unit tests should load include files, would it be able to make this a kernel test

Any chance we could do this instead? #1722694: Fixed Kernel::init() overrides Drupal's error handling configuration

Also see my comment on the issue about how to make Symfony's Error Handling work with our level of granularity: #23

I think there would be significant benefits to doing this, one that comes to mind is a much nicer debugging experience, really I cannot stress enough how nice it is compared to Drupal's.

+++ b/core/includes/bootstrap.inc
@@ -1015,56 +1035,29 @@
  */
 function &drupal_register_shutdown_function($callback = NULL) {
-  // We cannot use drupal_static() here because the static cache is reset during
-  // batch processing, which breaks batch handling.
-  static $callbacks = [];
-

I'm not sure about moving this to the Error class.

It's not specific to errors, it's shutdown stuff that should happen always (including errors) so I feel like that should be its own thing, what is the reason for moving it to Error?

I am rerolling

Not the easiest of rerolls

function from error.inc copied and mutated slight as they move into Error.php

Here are the code changes as I see them. Lets hope I have dealt with everything cleanly.

A)

   if (DRUPAL_TEST_IN_CHILD_SITE && !headers_sent() && (!defined('SIMPLETEST_COLLECT_ERRORS') || SIMPLETEST_COLLECT_ERRORS)) {
> +      static::header($error['@message'], $error['%type'], $error['%function'], $error['%file'], $error['%line']);    }
821,822c812

The logic for this conditional has been updated in core recently.

B)

The try catch pattern in Error:exceptionHandler() has been simplified

   try {
      // Log the message to the watchdog and return an error page to the user.
      static::log(static::decodeException($exception), TRUE);
    }
    // Catch \Throwable, which covers both Error and Exception throwables.
    catch (\Throwable $error) {
      static::exceptionHandlerAdditional($exception, $error);
    }

thank you -- I had half finished my corrections but Mondays are always a little busy.

In term of review this looks good to me. - The Legacy test did need adjustment.

1. +++ b/core/includes/errors.inc
   @@ -3,14 +3,15 @@
     * Functions for error handling.
   + *
   + * @deprecated in drupal:8.8.0 and is removed from drupal:9.0.0.
   + *   See each function in the file for individual deprecation notices with
   + *   upgrade instructions.
   + *
   

   I think so far we didn't explicitly deprecate a file as a while, this is slightly different as it is loaded conditionally but I still think that's covered with deprecating all the functions in it, and when updating that, if someone calls it directly, they can remove that then.

2. +++ b/core/includes/errors.inc
   @@ -19,27 +20,16 @@
   +  return Error::levels();
   

   I think getLevels() would make more sense here, that also matches \Drupal\Core\Logger\RfcLogLevel::getLevels

3. +++ b/core/includes/errors.inc
   @@ -293,32 +119,16 @@ function _drupal_log_error($error, $fatal = FALSE) {
   +  @trigger_error('_drupal_get_error_level() is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use \Drupal\Core\Utility\Error::getLevel() instead. See https://www.drupal.org/node/3009162', E_USER_DEPRECATED);
   +  return Error::getLevel();
   

   Error::getLevel() doesn't seem every explicit in what it returns, what about getCurrentLevel(), that's also better separated from levels/getLevels then.

4. +++ b/core/includes/errors.inc
   @@ -334,25 +144,14 @@ function _drupal_get_error_level() {
   +  @trigger_error('_drupal_error_header() is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use \Drupal\Core\Utility\Error::header() instead. See https://www.drupal.org/node/3009162', E_USER_DEPRECATED);
   +  Error::header($message, $type, $function, $file, $line);
   

   same here, I think methods should generally be verbNoun() or just verb(), header isn't a verb :)

   could be addHeader() or something more verbose, some arguments use emit, so could be emitHeader()? but add is probably easier to found. Also not 100% sure about it being public. Most functions are prefixed with _, but so is _drupal_error_handler_real() and that does need to be called. Still, this function is currently only called from within the other error functions and could be a candidate to make protected?

   We had a similar case in file_unmanaged_prepare(), initial patches hat it as public with a @todo to make it internal, but in the end, we just made it protected and kept the old function in place, easier to clean-up/remove then, it's not that much code after all.

   And in combination with it being a protected helper, I kinda like emitHeader() or emitAssertionHeader()

5. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -563,6 +564,7 @@ public function loadLegacyIncludes() {
        require_once $this->root . '/core/includes/form.inc';
   +    // ToDo: Remove include of errors.inc from drupal:9.0.x.
        require_once $this->root . '/core/includes/errors.inc';
   

   like above, not sure that's necessary, other functions like database.inc and entity.inc didn't get that todo either. If we keep it, then it should be @todo :)

6. +++ b/core/lib/Drupal/Core/EventSubscriber/FinalExceptionSubscriber.php
   @@ -154,21 +154,21 @@ public static function getSubscribedEvents() {
    
      /**
       * Wrapper for error_displayable().
       *
       * @param $error
   -   *   Optional error to examine for ERROR_REPORTING_DISPLAY_SOME.
   +   *   Optional error to examine for Error::REPORTING_DISPLAY_SOME.
       *
       * @return bool
       *
   -   * @see \error_displayable
   +   * @see \Drupal\Core\Utility\Error::isDisplayable()
       */
      protected function isErrorDisplayable($error) {
   -    return error_displayable($error);
   +    return Error::isDisplayable($error);
      }
    
   

   do we still need to wrap it now that it is a static method and not a function? Sometimes unit tests use such wrappers to mock different return values,

   Either we could deprecate this method too or we should update the docblock.

7. +++ b/core/lib/Drupal/Core/Utility/Error.php
   @@ -186,4 +209,411 @@ public static function formatBacktrace(array $backtrace) {
   +  public static function handler($error_level, $message, $filename = NULL, $line = NULL, $context = NULL) {
   +    if ($error_level & error_reporting()) {
   +      $types = static::levels();
   +      if (isset($types[$error_level])) {
   

   are these arguments optional for some kind of PHP BC? Maybe we can simplify it by making them required? $context is also deprecated in PHP 7.2, we could could possibly not define that at all, since we aren't using it anyway.

8. +++ b/core/lib/Drupal/Core/Utility/Error.php
   @@ -186,4 +209,411 @@ public static function formatBacktrace(array $backtrace) {
   +   */
   +  public static function getLevel() {
   +    // @todo: remove following include during drupal_installation_attempted
   +    // function deprecation.
   +    include_once DRUPAL_ROOT . '/core/includes/bootstrap.inc';
   +    // Raise the error level to maximum for the installer, so users are able to
   

   the include is necessary? log() uses it too and I guess we implicitly rely on this being called first?

9. +++ b/core/tests/Drupal/KernelTests/Core/Utility/LegacyErrorTest.php
   @@ -0,0 +1,97 @@
   +  public function testDrupalErrorHeader() {
   +    // Ignore warnings as function tries to set headers.
   +    $this->assertNull(@_drupal_error_header('Test',
   

   that's pretty ugly :) (the ignore warnings part)

It looks ready to go

1. +++ b/core/lib/Drupal/Core/EventSubscriber/FinalExceptionSubscriber.php
   @@ -154,21 +154,26 @@ public static function getSubscribedEvents() {
      protected function isErrorDisplayable($error) {
   -    return error_displayable($error);
   +    @trigger_error(__NAMESPACE__ . '::isErrorDisplayable() is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use \Drupal\Core\Utility\Error::isDisplayable() instead. See https://www.drupal.org/node/3009162', E_USER_DEPRECATED);
   +    return Error::isDisplayable($error);
   

   this method is protected so it needs follow-up to remove
   IIRC we do not remove wrappers on conversion

2. +++ b/core/lib/Drupal/Core/Session/SessionHandler.php
   @@ -85,10 +85,9 @@ public function write($sid, $value) {
   -      require_once DRUPAL_ROOT . '/core/includes/errors.inc';
   
   +++ b/core/tests/Drupal/Tests/Core/Error/DrupalLogErrorTest.php
   @@ -24,7 +24,6 @@ public function testFatalExitCode() {
   -require_once 'core/includes/errors.inc';
   

   yay!

3. +++ b/core/lib/Drupal/Core/Utility/Error.php
   @@ -186,4 +210,405 @@ public static function formatBacktrace(array $backtrace) {
   +  public static function getLevels() {
   

   this is not set @internal (as others) and it looks right

Re-roll #56 cos applies with offset

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -1037,8 +1038,8 @@ public static function bootEnvironment($app_root = NULL) {
-    set_error_handler('_drupal_error_handler');
-    set_exception_handler('_drupal_exception_handler');
+    set_error_handler([Error::class, 'handler']);
+    set_exception_handler([Error::class, 'exceptionHandler']);

so my question here is are we going far enough.

We're replacing one procedural function with a static, and not gaining things like dependency injection, e.g. the ::log method uses the container singleton

is this a first-step towards a proper error handler ala \Symfony\Component\HttpKernel\EventListener\DebugHandlersListener ?

Do we have a larger plan here?

Because if we do, we might find we have to do this again in 9.x.

Fix CS and needs review for 9.0

The patch needs a reroll. I did a very quick review:

+++ b/core/lib/Drupal/Core/Utility/Error.php
@@ -185,4 +220,410 @@ public static function formatBacktrace(array $backtrace) {
+   * @param string|null $filename
...
+   * @param int|null $line
...
+  public static function handler($error_level, $message, $filename, $line) {

Are the parameters $filename and $line optional or not?

I will do the reroll

reroll now .. I will work on 74 next

A) Fixed bug... not sure If I have everything.

B) $filename, $line, $context those parameters are no longer needed/used.. so deleted.

opps

better

a simple fix.

I am not sure that I follow

But what about CS issues?

Do you mean coding standard issues ... The test results do not show any additional CS issues.

I think this issue is complete... well ready for review.

Patch looks good. Some remarks:

1. +++ b/core/includes/bootstrap.inc
   @@ -310,18 +337,16 @@ function _drupal_error_handler($error_level, $message, $filename = NULL, $line =
   + * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0.
   + *    Use \Drupal\Core\Utility\Error::exceptionHandler() instead.
   + *
   + * @see https://www.drupal.org/node/3009162
   + * @see \Drupal\Core\Utility\Error::exceptionHandler()
     */
    function _drupal_exception_handler($exception) {
   

   This function is being deprecated. There is however no deprecation message test.

2. +++ b/core/includes/bootstrap.inc
   @@ -331,16 +356,16 @@ function _drupal_exception_handler($exception) {
    function _drupal_exception_handler_additional($exception, $exception2) {
   

   The same as with _drupal_exception_handler.

3. +++ b/core/lib/Drupal/Core/EventSubscriber/FinalExceptionSubscriber.php
   @@ -154,21 +154,26 @@ public static function getSubscribedEvents() {
   +   * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0.
   +   *   Use \Drupal\Core\Utility\Error::isDisplayable() instead.
   +   *
   +   * @see https://www.drupal.org/node/3009162
   +   * @see \Drupal\Core\Utility\Error::isDisplayable()
       */
      protected function isErrorDisplayable($error) {
   

   Same as with _drupal_exception_handler.

4. +++ b/core/includes/errors.inc
   @@ -51,58 +34,22 @@ function drupal_error_levels() {
   +function _drupal_error_handler_real($error_level, $message, $filename = NULL, $line = NULL, $context = NULL) {
   
   +++ b/core/lib/Drupal/Core/Utility/Error.php
   @@ -185,4 +220,405 @@ public static function formatBacktrace(array $backtrace) {
   +        list($severity_msg, $severity_level) = $types[$error_level];
   

   This can we rewritten to: [$severity_msg, $severity_level] = $types[$error_level];

5. +++ b/core/includes/errors.inc
   @@ -51,58 +34,22 @@ function drupal_error_levels() {
   -  elseif (DRUPAL_TEST_IN_CHILD_SITE && $error_level === E_USER_DEPRECATED) {
   
   +++ b/core/lib/Drupal/Core/Utility/Error.php
   @@ -185,4 +220,405 @@ public static function formatBacktrace(array $backtrace) {
   +    elseif (defined('DRUPAL_TEST_IN_CHILD_SITE') && $error_level === E_USER_DEPRECATED) {
   

   This change is not correct. Now we are only testing if the constant exists. We should also be testing that the value is TRUE.

6. +++ b/core/includes/errors.inc
   @@ -145,147 +89,16 @@ function error_displayable($error = NULL) {
   -  if (DRUPAL_TEST_IN_CHILD_SITE && !headers_sent() && (!defined('SIMPLETEST_COLLECT_ERRORS') || SIMPLETEST_COLLECT_ERRORS)) {
   
   +++ b/core/lib/Drupal/Core/Utility/Error.php
   @@ -185,4 +220,405 @@ public static function formatBacktrace(array $backtrace) {
   +    if (defined('DRUPAL_TEST_IN_CHILD_SITE') && !headers_sent() && (!defined('SIMPLETEST_COLLECT_ERRORS') || SIMPLETEST_COLLECT_ERRORS)) {
   

   Same as previous point.

7. +++ b/core/lib/Drupal/Core/Utility/Error.php
   @@ -185,4 +220,405 @@ public static function formatBacktrace(array $backtrace) {
   +    // When running inside the testing framework, we relay the errors
   +    // to the tested site by the way of HTTP headers.
   +    if (defined('DRUPAL_TEST_IN_CHILD_SITE') && !headers_sent() && (!defined('SIMPLETEST_COLLECT_ERRORS') || SIMPLETEST_COLLECT_ERRORS)) {
   +      static::emitAssertionHeader($error['@message'], $error['%type'], $error['%function'], $error['%file'], $error['%line']);
   +
   +    }
   

   Can we remove this empty line.

8. In the docblock of the function error_displayable() there are 3 mentions of ERROR_REPORTING_DISPLAY_*. Can we replace them.

@daffie thanks for the detailed review.

I am on it.

1. +++ b/core/includes/bootstrap.inc
   @@ -295,8 +315,15 @@ function watchdog_exception($type, Exception $exception, $message = NULL, $varia
   +  @trigger_error('_drupal_error_handler() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use \Drupal\Core\Utility\Error::handler() instead. See https://www.drupal.org/node/3009162', E_USER_DEPRECATED);
      require_once __DIR__ . '/errors.inc';
      _drupal_error_handler_real($error_level, $message, $filename, $line, $context);
   

   Why does this still call _drupal_error_handler_real()? Can't it call Error::handler() directly now, like the deprecation message suggests?

2. +++ b/core/lib/Drupal/Core/Utility/Error.php
   @@ -185,4 +220,407 @@ public static function formatBacktrace(array $backtrace) {
   +  public static function handler($error_level, $message) {
   

   Unsure if this is bikeshedding but we often use verbs as method names so this could just be handle()?

3. +++ b/core/lib/Drupal/Core/Utility/Error.php
   @@ -185,4 +220,407 @@ public static function formatBacktrace(array $backtrace) {
   +  public static function exceptionHandler($exception) {
   

   Similarly this would read better as handleException()?

4. +++ b/core/lib/Drupal/Core/Utility/Error.php
   @@ -185,4 +220,407 @@ public static function formatBacktrace(array $backtrace) {
   +  public static function exceptionHandlerAdditional($exception, $exception2) {
   

   And this could be handleAdditionalException()?

5. +++ b/core/tests/Drupal/Tests/Core/Error/DrupalLogErrorTest.php
   @@ -37,7 +36,7 @@ public function testFatalExitCode() {
   +\Drupal\Core\Utility\Error::log($error, TRUE);
   

   Why not use this class and then only call Error::log() here?

Thanks, looks great now - let's see what framework managers think.

Some thoughts:

1. core/lib/Drupal/Core/Utility/Error.php needs to be added to the classmap part of core/composer.json - this is because it's not going to be needed on every request. Adding it here will make the autoloader have to do less work.
2. I think we leave all the function's that are doing \Drupal::service() or the like in this issue and only move the stuff that is truly static. And then we should open a follow-up to the other stuff properly. With this issue it feels like we trading a compromise API (from an OO design perspective) for the same API. I.e. we moved everything around but not much has really changed. We still struggle to unit test our error handling because it is coupled to the globals like \Drupal::root() or whatever.
3. Have we tested that the @trigger_error in our old error handlers doesn't result in recursive loop? LegacyErrorTest should be setting the old error handlers up and triggering an error to test that they don't break in this situation.

Correcting the failing test case in #94.

Uploaded the wrong patch in #95. Here is a complete fix.

interdiff:

--- a/composer.lock
+++ b/composer.lock
@@ -485,7 +485,7 @@
             "dist": {
                 "type": "path",
                 "url": "core",
-                "reference": "5bd6798a64831fa08a343a14a0ee47127c4cb99f"
+                "reference": "d8a8c903a37bbdc2f7597e0d60c5c7b9b0afcb2e"
             },
             "require": {
                 "asm89/stack-cors": "^1.1",
@@ -707,7 +707,8 @@
                     "lib/Drupal/Core/DrupalKernel.php",
                     "lib/Drupal/Core/DrupalKernelInterface.php",
                     "lib/Drupal/Core/Installer/InstallerRedirectTrait.php",
-                    "lib/Drupal/Core/Site/Settings.php"
+                    "lib/Drupal/Core/Site/Settings.php",
+                    "lib/Drupal/Core/Utility/Error.php"
                 ]
             },
             "scripts": {

The fix for comment #93.1 is good.

The other 2 points #93.2 and #93.3 are still open.

All review points were addressed but #100 no longer applies.

#102 needs reroll again, but I think this can be set to RTBC after rerolling.

I think we leave all the function's that are doing \Drupal::service() or the like in this issue and only move the stuff that is truly static.

Is not addressed. The Error class is still full of \Drupal calls. With this issue it feels like we trading a compromised API (from an OO design perspective) for the same API. I.e. we moved everything around but not much has really changed. We still struggle to unit test our error handling because it is coupled to the globals like \Drupal::root() or whatever.

I think this issue should start with an API design that is fit for purpose. Doing this issue makes it harder to do this until Drupal 10 because it gives us 2 static APIs to support rather than the single one we have today. This is likely to be hard to do because the error system is very tangly.

I think this patch can deprecate drupal_error_levels() and the constants but the rest of it needs work.

Following on from #106 I feel like it would make a lot more sense to focused on #1722694: Fixed Kernel::init() overrides Drupal's error handling configuration instead, as we could just strip out all of the custom error handling that Drupal has and just adapt Symfony's to work for our needs. Thereby reducing overhead and likely solving the issues Alex mentioned in #106.

In comment #1722694-23: Fixed Kernel::init() overrides Drupal's error handling configuration I noted how we could address the issue of Symfony's TRUE/FALSE debugging options to fit our more granular needs:

That might be our only real blocker, and could be addressed by simply initialising the debugger outside of the Symfony Kernel's init function, there's even docs on the Symfony site explaining how to do it, so it's definitely supported.

Related issue changed signature

Rerolled patch against drupal 9.5.x

Needs more than just reroll, #106 needs addressing which is a change in direction.

Adding #3526515: Use get_error_handler() in php 8.5 and deprecate Error::currentErrorHandler as related.