[ErrorHandler] help finish the PR

symfony · Nov 11, 2019 · 7f7312f · 7f7312f
1 parent 6c9157b
commit 7f7312f
Show file tree

Hide file tree

Showing 17 changed files with 108 additions and 111 deletions.
diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/error_renderer.xml b/src/Symfony/Bundle/FrameworkBundle/Resources/config/error_renderer.xml
@@ -16,9 +16,13 @@
 
         <service id="error_handler.error_renderer.serializer" class="Symfony\Component\ErrorHandler\ErrorRenderer\SerializerErrorRenderer">
             <argument type="service" id="serializer" />
-            <argument type="service" id="request_stack" />
+            <argument type="service">
+                <service>
+                    <factory class="Symfony\Component\ErrorHandler\ErrorRenderer\SerializerErrorRenderer" method="getPreferredFormat" />
+                    <argument type="service" id="request_stack" />
+                </service>
+            </argument>
             <argument type="service" id="error_renderer.html" />
-            <argument>%kernel.debug%</argument>
         </service>
 
         <service id="error_renderer.html" alias="error_handler.error_renderer.html" />

diff --git a/src/Symfony/Bundle/TwigBundle/ErrorRenderer/TwigHtmlErrorRenderer.php b/src/Symfony/Bundle/TwigBundle/ErrorRenderer/TwigHtmlErrorRenderer.php
@@ -44,7 +44,7 @@ public function render(\Throwable $exception): FlattenException
     {
         $exception = $this->htmlErrorRenderer->render($exception);
 
-        if ($this->debug || !$template = $this->findTemplate($exception->getStatusCode());
+        if ($this->debug || !$template = $this->findTemplate($exception->getStatusCode())) {
             return $exception;
         }
 

diff --git a/src/Symfony/Bundle/TwigBundle/Tests/DependencyInjection/TwigExtensionTest.php b/src/Symfony/Bundle/TwigBundle/Tests/DependencyInjection/TwigExtensionTest.php
@@ -23,6 +23,7 @@
 use Symfony\Component\DependencyInjection\Loader\YamlFileLoader;
 use Symfony\Component\DependencyInjection\ParameterBag\ParameterBag;
 use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Component\ErrorHandler\ErrorRenderer\HtmlErrorRenderer;
 
 class TwigExtensionTest extends TestCase
 {
@@ -302,6 +303,7 @@ public function testRuntimeLoader()
         $container->register('templating.locator', 'FooClass');
         $container->register('templating.name_parser', 'FooClass');
         $container->register('foo', '%foo%')->addTag('twig.runtime');
+        $container->register('error_renderer.html', HtmlErrorRenderer::class);
         $container->addCompilerPass(new RuntimeLoaderPass(), PassConfig::TYPE_BEFORE_REMOVING);
         $container->getCompilerPassConfig()->setRemovingPasses([]);
         $container->getCompilerPassConfig()->setAfterRemovingPasses([]);

diff --git a/src/Symfony/Bundle/TwigBundle/Tests/ErrorRenderer/TwigHtmlErrorRendererTest.php b/src/Symfony/Bundle/TwigBundle/Tests/ErrorRenderer/TwigHtmlErrorRendererTest.php
@@ -14,6 +14,7 @@
 use PHPUnit\Framework\TestCase;
 use Symfony\Bundle\TwigBundle\ErrorRenderer\TwigHtmlErrorRenderer;
 use Symfony\Component\ErrorHandler\ErrorRenderer\HtmlErrorRenderer;
+use Symfony\Component\ErrorHandler\Exception\FlattenException;
 use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
 use Twig\Environment;
 use Twig\Loader\ArrayLoader;
@@ -46,27 +47,19 @@ public function testFallbackToNativeRendererIfCustomTemplateNotFound()
             ->expects($this->once())
             ->method('render')
             ->with($exception)
+            ->willReturn(FlattenException::createFromThrowable($exception))
         ;
 
         (new TwigHtmlErrorRenderer($twig, $nativeRenderer, false))->render($exception);
     }
 
     public function testRenderCustomErrorTemplate()
     {
-        $exception = new NotFoundHttpException();
-
         $twig = new Environment(new ArrayLoader([
             '@Twig/Exception/error404.html.twig' => '<h1>Page Not Found</h1>',
         ]));
+        $exception = (new TwigHtmlErrorRenderer($twig, new HtmlErrorRenderer()))->render(new NotFoundHttpException());
 
-        $nativeRenderer = $this->createMock(HtmlErrorRenderer::class);
-        $nativeRenderer
-            ->expects($this->never())
-            ->method('render')
-        ;
-
-        $content = (new TwigHtmlErrorRenderer($twig, $nativeRenderer, false))->render($exception);
-
-        $this->assertSame('<h1>Page Not Found</h1>', $content->getAsString());
+        $this->assertSame('<h1>Page Not Found</h1>', $exception->getAsString());
     }
 }
diff --git a/src/Symfony/Bundle/WebProfilerBundle/Tests/DependencyInjection/WebProfilerExtensionTest.php b/src/Symfony/Bundle/WebProfilerBundle/Tests/DependencyInjection/WebProfilerExtensionTest.php
@@ -54,7 +54,7 @@ protected function setUp(): void
         $this->kernel = $this->getMockBuilder('Symfony\\Component\\HttpKernel\\KernelInterface')->getMock();
 
         $this->container = new ContainerBuilder();
-        $this->container->register('error_renderer.html', HtmlErrorRenderer::class)->setPublic(true);
+        $this->container->register('error_handler.error_renderer.html', HtmlErrorRenderer::class)->setPublic(true);
         $this->container->register('event_dispatcher', EventDispatcher::class)->setPublic(true);
         $this->container->register('router', $this->getMockClass('Symfony\\Component\\Routing\\RouterInterface'))->setPublic(true);
         $this->container->register('twig', 'Twig\Environment')->setPublic(true);

diff --git a/src/Symfony/Component/ErrorHandler/ErrorHandler.php b/src/Symfony/Component/ErrorHandler/ErrorHandler.php
@@ -21,7 +21,6 @@
 use Symfony\Component\ErrorHandler\ErrorEnhancer\UndefinedMethodErrorEnhancer;
 use Symfony\Component\ErrorHandler\ErrorRenderer\CliErrorRenderer;
 use Symfony\Component\ErrorHandler\ErrorRenderer\HtmlErrorRenderer;
-use Symfony\Component\ErrorHandler\Exception\FlattenException;
 use Symfony\Component\ErrorHandler\Exception\SilencedErrorContext;
 
 /**
@@ -580,7 +579,12 @@ public function handleException(\Throwable $exception)
         }
 
         $exceptionHandler = $this->exceptionHandler;
-        $this->exceptionHandler = null;
+        $this->exceptionHandler = [$this, 'renderException'];
+
+        if (null === $exceptionHandler || $exceptionHandler === $this->exceptionHandler){
+            $this->exceptionHandler = null;
+        }
+
         try {
             if (null !== $exceptionHandler) {
                 return $exceptionHandler($exception);
@@ -593,7 +597,14 @@ public function handleException(\Throwable $exception)
             throw $exception; // Give back $exception to the native handler
         }
 
-        $this->handleException($handlerException);
+        $loggedErrors = $this->loggedErrors;
+        $this->loggedErrors = $exception === $handlerException ? 0 : $this->loggedErrors;
+
+        try {
+            $this->handleException($handlerException);
+        } finally {
+            $this->loggedErrors = $loggedErrors;
+        }
     }
 
     /**

diff --git a/src/Symfony/Component/ErrorHandler/ErrorRenderer/CliErrorRenderer.php b/src/Symfony/Component/ErrorHandler/ErrorRenderer/CliErrorRenderer.php
@@ -30,7 +30,7 @@ public function render(\Throwable $exception): FlattenException
             protected function supportsColors(): bool
             {
                 $outputStream = $this->outputStream;
-                $this->outputStream = STDOUT;
+                $this->outputStream = fopen('php://stdout', 'w');
 
                 try {
                     return parent::supportsColors();

diff --git a/src/Symfony/Component/ErrorHandler/ErrorRenderer/HtmlErrorRenderer.php b/src/Symfony/Component/ErrorHandler/ErrorRenderer/HtmlErrorRenderer.php
@@ -102,13 +102,13 @@ private function renderException(FlattenException $exception, string $debugTempl
             'statusText' => $statusText,
             'statusCode' => $statusCode,
             'logger' => $this->logger instanceof DebugLoggerInterface ? $this->logger : null,
-            'currentContent' => $request ? $this->getAndCleanOutputBuffering($request->headers->get('X-Php-Ob-Level')) : '',
+            'currentContent' => $request ? $this->getAndCleanOutputBuffering($request->headers->get('X-Php-Ob-Level', -1)) : '',
         ]);
     }
 
-    private function getAndCleanOutputBuffering(?int $startObLevel): string
+    private function getAndCleanOutputBuffering(int $startObLevel): string
     {
-        if (null === $startObLevel || ob_get_level() <= $startObLevel) {
+        if (ob_get_level() <= $startObLevel) {
             return '';
         }
 

diff --git a/src/Symfony/Component/ErrorHandler/ErrorRenderer/SerializerErrorRenderer.php b/src/Symfony/Component/ErrorHandler/ErrorRenderer/SerializerErrorRenderer.php
@@ -11,7 +11,8 @@
 
 namespace Symfony\Component\ErrorHandler\ErrorRenderer;
 
-use Symfony\Component\ErrorRenderer\Exception\FlattenException;
+use Symfony\Component\ErrorHandler\Exception\FlattenException;
+use Symfony\Component\HttpFoundation\RequestStack;
 use Symfony\Component\Serializer\Exception\NotEncodableValueException;
 use Symfony\Component\Serializer\SerializerInterface;
 
@@ -20,31 +21,50 @@
  *
  * @author Nicolas Grekas <p@tchwork.com>
  */
-class SerializerErrorRenderer
+class SerializerErrorRenderer implements ErrorRendererInterface
 {
     private $serializer;
-    private $requestStack;
-    private $debug;
+    private $format;
+    private $fallbackErrorRenderer;
 
-    public function __construct(SerializerInterface $serializer, RequestStack $requestStack, bool $debug = true)
+    /**
+     * @param string|callable $format The format as a string or a callable that should return it
+     */
+    public function __construct(SerializerInterface $serializer, $format, ErrorRendererInterface $fallbackErrorRenderer = null)
     {
+        if (!\is_string($format) && !\is_callable($format)) {
+            throw new \TypeError(sprintf('Argument 2 passed to %s() must be a string or a callable, %s given.', __METHOD__, \is_object($format) ? \get_class($format) : \gettype($format)));
+        }
+
         $this->serializer = $serializer;
-        $this->requestStack = $requestStack;
-        $this->debug = $debug;
+        $this->format = $format;
+        $this->fallbackErrorRenderer = $fallbackErrorRenderer ?? new HtmlErrorRenderer();
     }
 
     /**
      * {@inheritdoc}
      */
     public function render(\Throwable $exception): FlattenException
     {
-        $format = $this->requestStack->getCurrentRequest()->getPreferredFormat();
         $flattenException = FlattenException::createFromThrowable($exception);
 
         try {
+            $format = \is_string($this->format) ? $this->format : ($this->format)();
+
             return $flattenException->setAsString($this->serializer->serialize($flattenException, $format, ['exception' => $exception]));
-        } catch (NotEncodableValueException $_) {
-            return (new HtmlErrorHandler($this->debug))->render($exception);
+        } catch (NotEncodableValueException $e) {
+            return $this->fallbackErrorRenderer->render($exception);
         }
     }
+
+    public static function getPreferredFormat(RequestStack $requestStack): \Closure
+    {
+        return static function () use ($requestStack) {
+            if (!$request = $requestStack->getCurrentRequest()) {
+                throw new NotEncodableValueException();
+            }
+
+            return $request->getPreferredFormat();
+        };
+    }
 }
diff --git a/src/Symfony/Component/ErrorHandler/Tests/ErrorHandlerTest.php b/src/Symfony/Component/ErrorHandler/Tests/ErrorHandlerTest.php
@@ -50,23 +50,13 @@ public function testRegister()
                 $h = set_error_handler('var_dump');
                 restore_error_handler();
                 $this->assertSame([$newHandler, 'handleError'], $h);
-            } catch (\Exception $e) {
+            } finally {
+                restore_error_handler();
+                restore_exception_handler();
             }
-
+        } finally {
             restore_error_handler();
             restore_exception_handler();
-
-            if (isset($e)) {
-                throw $e;
-            }
-        } catch (\Exception $e) {
-        }
-
-        restore_error_handler();
-        restore_exception_handler();
-
-        if (isset($e)) {
-            throw $e;
         }
     }
 
@@ -86,11 +76,9 @@ public function testErrorGetLast()
                 'line' => __LINE__ - 5,
             ];
             $this->assertSame($expected, error_get_last());
-        } catch (\Exception $e) {
+        } finally {
             restore_error_handler();
             restore_exception_handler();
-
-            throw $e;
         }
     }
 
@@ -323,14 +311,9 @@ public function testHandleError()
             unset($undefVar);
             $line = __LINE__ + 1;
             @$undefVar++;
-
-            restore_error_handler();
-            restore_exception_handler();
-        } catch (\Exception $e) {
+        } finally {
             restore_error_handler();
             restore_exception_handler();
-
-            throw $e;
         }
     }
 
@@ -406,6 +389,7 @@ public function testHandleException(string $expectedMessage, \Throwable $excepti
             ;
 
             $handler->setDefaultLogger($logger, E_ERROR);
+            $handler->setExceptionHandler(null);
 
             try {
                 $handler->handleException($exception);
@@ -530,16 +514,12 @@ public function testHandleFatalError()
             ;
 
             $handler->setDefaultLogger($logger, E_PARSE);
+            $handler->setExceptionHandler(null);
 
             $handler->handleFatalError($error);
-
-            restore_error_handler();
-            restore_exception_handler();
-        } catch (\Exception $e) {
+        } finally {
             restore_error_handler();
             restore_exception_handler();
-
-            throw $e;
         }
     }
 
@@ -563,6 +543,7 @@ public function testCustomExceptionHandler()
         $this->expectException('Exception');
         $handler = new ErrorHandler();
         $handler->setExceptionHandler(function ($e) use ($handler) {
+            $handler->setExceptionHandler(null);
             $handler->handleException($e);
         });
 
@@ -572,7 +553,7 @@ public function testCustomExceptionHandler()
     public function testSendPhpResponse()
     {
         $handler = new ErrorHandler();
-        $handler->setExceptionHandler([$handler, 'sendPhpResponse']);
+        $handler->setExceptionHandler([$handler, 'renderException']);
 
         ob_start();
         $handler->handleException(new \RuntimeException('Class Foo not found'));

diff --git a/src/Symfony/Component/ErrorHandler/Tests/ErrorRenderer/HtmlErrorRendererTest.php b/src/Symfony/Component/ErrorHandler/Tests/ErrorRenderer/HtmlErrorRendererTest.php
@@ -20,7 +20,7 @@ class HtmlErrorRendererTest extends TestCase
     /**
      * @dataProvider getRenderData
      */
-    public function testRender(FlattenException $exception, HtmlErrorRenderer $errorRenderer, string $expected)
+    public function testRender(\Throwable $exception, HtmlErrorRenderer $errorRenderer, string $expected)
     {
         $this->assertStringMatchesFormat($expected, $errorRenderer->render($exception)->getAsString());
     }
@@ -44,13 +44,13 @@ public function getRenderData(): iterable
 HTML;
 
         yield '->render() returns the HTML content WITH stack traces in debug mode' => [
-            FlattenException::createFromThrowable(new \RuntimeException('Foo')),
+            new \RuntimeException('Foo'),
             new HtmlErrorRenderer(true),
             $expectedDebug,
         ];
 
         yield '->render() returns the HTML content WITHOUT stack traces in non-debug mode' => [
-            FlattenException::createFromThrowable(new \RuntimeException('Foo')),
+            new \RuntimeException('Foo'),
             new HtmlErrorRenderer(false),
             $expectedNonDebug,
         ];