bug #29869 [Debug][ErrorHandler] Preserve our error handler when a lo…

…gger sets another one (fancyweb)

This PR was merged into the 3.4 branch.

Discussion
----------

[Debug][ErrorHandler] Preserve our error handler when a logger sets another one

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

When logging errors handled by the `ErrorHandler::handleError()` method, the logger can temporarily set its own custom error handler. This is for example the case of `Monolog` in the `StreamHandler` class (cf https://github.com/Seldaek/monolog/blob/ebb804e432e8fe0fe96828f30d89c45581d36d07/src/Monolog/Handler/StreamHandler.php#L101).

However, when the previous error handler is restored by the logger, it "skips" the real previous handler (the `ErrorHandler::handleError()` one) in the pile and goes back directly to the one before. I guess this is because the `restore_error_handler()` call is technically done in the error handler itself, so it logically restore it to the one before and not to itself.

Here is an easy small example that shows the PHP behavior : https://3v4l.org/4OZNZ

The only solution I have found to fix it is to set our error handler everytime an error is logged.

Here are the things I discovered while trying to find a cleaner fix :
- Setting the same error handler in the error handler itself doesn't actually add it to the pile. This is why adding a check is useless.
- Checking if the logger modified the error handler is impossible anyway : to get the current error handler, you need to set a new one temporarirly and then revert it. However, when you revert it by calling `restore_error_handler()` you end up having the same problem you are trying to fix...
- Also trying to get the current error handler in the error handler itself will return NULL if it is itself.

Commits
-------

b979fff [Debug][ErrorHandler] Preserve our error handler when a logger set another one

src/Symfony/Component/Debug/ErrorHandler.php

@@ -523,6 +523,10 @@ public function handleError($type, $message, $file, $line)
                 $this->loggers[$type][0]->log($level, $logMessage, $errorAsException ? ['exception' => $errorAsException] : []);
             } finally {
                 $this->isRecursive = false;
+
+                if (!\defined('HHVM_VERSION')) {
+                    set_error_handler([$this, __FUNCTION__]);
+                }
             }
         }
 

src/Symfony/Component/Debug/Tests/ErrorHandlerTest.php

@@ -12,10 +12,13 @@
 namespace Symfony\Component\Debug\Tests;
 
 use PHPUnit\Framework\TestCase;
+use Psr\Log\LoggerInterface;
 use Psr\Log\LogLevel;
+use Psr\Log\NullLogger;
 use Symfony\Component\Debug\BufferingLogger;
 use Symfony\Component\Debug\ErrorHandler;
 use Symfony\Component\Debug\Exception\SilencedErrorContext;
+use Symfony\Component\Debug\Tests\Fixtures\LoggerThatSetAnErrorHandler;
 
 /**
  * ErrorHandlerTest.
@@ -321,6 +324,8 @@ public function testHandleDeprecation()
         $handler = new ErrorHandler();
         $handler->setDefaultLogger($logger);
         @$handler->handleError(E_USER_DEPRECATED, 'Foo deprecation', __FILE__, __LINE__, []);
+
+        restore_error_handler();
     }
 
     /**
@@ -583,4 +588,43 @@ public function testCustomExceptionHandler()
 
         $handler->handleException(new \Exception());
     }
+
+    /**
+     * @dataProvider errorHandlerIsNotLostWhenLoggingProvider
+     */
+    public function testErrorHandlerIsNotLostWhenLogging($customErrorHandlerHasBeenPreviouslyDefined, LoggerInterface $logger)
+    {
+        try {
+            if ($customErrorHandlerHasBeenPreviouslyDefined) {
+                set_error_handler('count');
+            }
+
+            $handler = ErrorHandler::register();
+            $handler->setDefaultLogger($logger);
+
+            @trigger_error('foo', E_USER_DEPRECATED);
+            @trigger_error('bar', E_USER_DEPRECATED);
+
+            $this->assertSame([$handler, 'handleError'], set_error_handler('var_dump'));
+
+            restore_error_handler();
+
+            if ($customErrorHandlerHasBeenPreviouslyDefined) {
+                restore_error_handler();
+            }
+        } finally {
+            restore_error_handler();
+            restore_exception_handler();
+        }
+    }
+
+    public function errorHandlerIsNotLostWhenLoggingProvider()
+    {
+        return [
+            [false, new NullLogger()],
+            [true, new NullLogger()],
+            [false, new LoggerThatSetAnErrorHandler()],
+            [true, new LoggerThatSetAnErrorHandler()],
+        ];
+    }
 }

src/Symfony/Component/Debug/Tests/Fixtures/LoggerThatSetAnErrorHandler.php

@@ -0,0 +1,14 @@
+<?php
+
+namespace Symfony\Component\Debug\Tests\Fixtures;
+
+use Psr\Log\AbstractLogger;
+
+class LoggerThatSetAnErrorHandler extends AbstractLogger
+{
+    public function log($level, $message, array $context = [])
+    {
+        set_error_handler('is_string');
+        restore_error_handler();
+    }
+}