bug #25835 [HttpKernel] DebugHandlersListener should always replace the existing exception handler (nicolas-grekas)

This PR was merged into the 2.7 branch.

Discussion
----------

[HttpKernel] DebugHandlersListener should always replace the existing exception handler

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

The current logic is inconsistent because replacing or not depends whether an ExceptionHandler is registered or not.
Embeds tests for the previous PR on the same topic, Debug component's side.

Commits
-------

a4ddcc2 [HttpKernel] DebugHandlersListener should always replace the existing exception handler

.travis.yml

@@ -181,7 +181,7 @@ install:
           elif [[ $deps = low ]]; then
               echo "$COMPONENTS" | parallel --gnu -j10% "tfold {} 'cd {} && $COMPOSER_UP --prefer-lowest --prefer-stable && $PHPUNIT_X'"
           elif [[ $PHP = hhvm* ]]; then
-              $PHPUNIT --exclude-group benchmark,intl-data
+              $PHPUNIT --exclude-group no-hhvm,benchmark,intl-data
           else
               echo "$COMPONENTS" | parallel --gnu "tfold {} $PHPUNIT_X {}"
               tfold tty-group $PHPUNIT --group tty

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

@@ -293,6 +293,9 @@ class ErrorHandlerTest extends TestCase
         @$handler->handleError(E_USER_DEPRECATED, 'Foo deprecation', __FILE__, __LINE__, array());
     }
 
+    /**
+     * @group no-hhvm
+     */
     public function testHandleException()
     {
         try {
@@ -375,6 +378,9 @@ class ErrorHandlerTest extends TestCase
         }
     }
 
+    /**
+     * @group no-hhvm
+     */
     public function testHandleFatalError()
     {
         try {
@@ -434,6 +440,9 @@ class ErrorHandlerTest extends TestCase
         $this->assertStringStartsWith("Attempted to load class \"Foo\" from the global namespace.\nDid you forget a \"use\" statement", $args[0]->getMessage());
     }
 
+    /**
+     * @group no-hhvm
+     */
     public function testHandleFatalErrorOnHHVM()
     {
         try {

src/Symfony/Component/Debug/Tests/phpt/decorate_exception_hander.phpt

@@ -0,0 +1,47 @@
+--TEST--
+Test catching fatal errors when handlers are nested
+--FILE--
+<?php
+
+namespace Symfony\Component\Debug;
+
+$vendor = __DIR__;
+while (!file_exists($vendor.'/vendor')) {
+    $vendor = dirname($vendor);
+}
+require $vendor.'/vendor/autoload.php';
+
+set_error_handler('var_dump');
+set_exception_handler('var_dump');
+
+ErrorHandler::register(null, false);
+
+if (true) {
+    class foo extends missing
+    {
+    }
+}
+
+?>
+--EXPECTF--
+Fatal error: Class 'Symfony\Component\Debug\missing' not found in %s on line %d
+object(Symfony\Component\Debug\Exception\ClassNotFoundException)#%d (8) {
+  ["message":protected]=>
+  string(131) "Attempted to load class "missing" from namespace "Symfony\Component\Debug".
+Did you forget a "use" statement for another namespace?"
+  ["string":"Exception":private]=>
+  string(0) ""
+  ["code":protected]=>
+  int(0)
+  ["file":protected]=>
+  string(%d) "%s"
+  ["line":protected]=>
+  int(%d)
+  ["trace":"Exception":private]=>
+  array(0) {
+  }
+  ["previous":"Exception":private]=>
+  NULL
+  ["severity":protected]=>
+  int(1)
+}

src/Symfony/Component/Debug/Tests/phpt/fatal_with_nested_handlers.phpt

@@ -35,7 +35,7 @@ array(1) {
   [0]=>
   string(37) "Error and exception handlers do match"
 }
-object(Symfony\Component\Debug\Exception\FatalErrorException)#4 (8) {
+object(Symfony\Component\Debug\Exception\FatalErrorException)#%d (8) {
   ["message":protected]=>
   string(199) "Error: Class Symfony\Component\Debug\Broken contains 2 abstract methods and must therefore be declared abstract or implement the remaining methods (Serializable::serialize, Serializable::unserialize)"
 %a

src/Symfony/Component/HttpKernel/EventListener/DebugHandlersListener.php

@@ -117,9 +117,13 @@ class DebugHandlersListener implements EventSubscriberInterface
         }
         if ($this->exceptionHandler) {
             if ($handler instanceof ErrorHandler) {
-                $h = $handler->setExceptionHandler('var_dump') ?: $this->exceptionHandler;
-                $handler->setExceptionHandler($h);
-                $handler = is_array($h) ? $h[0] : null;
+                $h = $handler->setExceptionHandler('var_dump');
+                if (is_array($h) && $h[0] instanceof ExceptionHandler) {
+                    $handler->setExceptionHandler($h);
+                    $handler = $h[0];
+                } else {
+                    $handler->setExceptionHandler($this->exceptionHandler);
+                }
             }
             if ($handler instanceof ExceptionHandler) {
                 $handler->setHandler($this->exceptionHandler);

src/Symfony/Component/HttpKernel/Tests/EventListener/DebugHandlersListenerTest.php

@@ -29,8 +29,6 @@ use Symfony\Component\HttpKernel\HttpKernelInterface;
 use Symfony\Component\HttpKernel\KernelEvents;
 
 /**
- * DebugHandlersListenerTest.
- *
  * @author Nicolas Grekas <p@tchwork.com>
  */
 class DebugHandlersListenerTest extends TestCase
@@ -132,4 +130,26 @@ class DebugHandlersListenerTest extends TestCase
 
         $xHandler(new \Exception());
     }
+
+    public function testReplaceExistingExceptionHandler()
+    {
+        $userHandler = function () {};
+        $listener = new DebugHandlersListener($userHandler);
+        $eHandler = new ErrorHandler();
+        $eHandler->setExceptionHandler('var_dump');
+
+        $exception = null;
+        set_exception_handler(array($eHandler, 'handleException'));
+        try {
+            $listener->configure();
+        } catch (\Exception $exception) {
+        }
+        restore_exception_handler();
+
+        if (null !== $exception) {
+            throw $exception;
+        }
+
+        $this->assertSame($userHandler, $eHandler->setExceptionHandler('var_dump'));
+    }
 }