[TwigBridge] Improve error rendering when running tests #58456

javiereguiluz · 2024-10-04T09:03:57Z

Q	A
Branch?	7.2
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	-
License	MIT

Exception handling in Symfony is fantastic. E.g. I add a $foo = 0 / 0; statement in the Symfony Demo app and I see this nice and useful exception page in debug mode:

The problem is that if I run tests, that exception will output the same content in the terminal. Thousands and thousands of lines of HTML + CSS + JS 😐

This is easily one of the worst and most frustrating parts of Symfony. In #31514 we tried to improve this by adding an HTML comment at the beginning and the end of the long HTML exception page to quickly figure out the exception in the console.

This PR proposes to always use the CliRenderer when the exception is going to be rendered in a console/terminal context.

For example, the same exception as before now would look like this 🙌

stof

Returning Content-type: text/html when using the CliErrorRenderer is wrong as it does not return HTML (and could lead to XSS if some of the content looks like HTML tags). The CliErrorRenderer should be updated to add a header setting the content type to text/plain in the FLattenException it returns (which won't change anything when using it for actual CLI usage, but will behave properly when using the CliErrorRenderer to return an HTTP response)

src/Symfony/Bridge/Twig/ErrorRenderer/TwigErrorRenderer.php

javiereguiluz · 2024-10-07T07:11:32Z

I made the changes requested in the review. Thanks!

alexandre-daubois

Related to #58070, so it's a 👍 for me, definitely!

Edit: approved once tests are green of course 😄

src/Symfony/Bridge/Twig/ErrorRenderer/TwigErrorRenderer.php

nicolas-grekas · 2024-10-09T14:32:02Z

src/Symfony/Bridge/Twig/ErrorRenderer/TwigErrorRenderer.php

        bool|callable $debug = false,
    ) {
-        $this->fallbackErrorRenderer = $fallbackErrorRenderer ?? new HtmlErrorRenderer();
+        $this->fallbackErrorRenderer = \in_array(\PHP_SAPI, ['cli', 'phpdbg', 'embed'], true) ? new CliErrorRenderer() : ($fallbackErrorRenderer ?? new HtmlErrorRenderer());


This is breaking DI: if one dared to pass a $fallbackErrorRenderer, we have to honor it.
In order to pass a CliErrorRenderer, we should use DI instead.
If this targets tests, then we might pass a CliErrorRenderer service when framework.test is enabled?

What if we change that line by:

$this->fallbackErrorRenderer = $fallbackErrorRenderer ?? (\in_array(\PHP_SAPI, ['cli', 'phpdbg', 'embed'], true) ? new CliErrorRenderer() : new HtmlErrorRenderer());

My suggestion in #58456 (review) was about changing the default fallback renderer, which is indeed what your new suggestion does.

would this achieve what you want?
we do define a $fallbackErrorRenderer so I guess the new code would be dead code

You are right. The feature no longer works after changing that line.

nicolas-grekas · 2024-10-09T15:06:48Z

What about improving the HtmlErrorRendered instead so that it dumps a text version after the HTML one in an HTML comment? (we'd just need to ensure the dump cannot break the HTML comment to make this XSS-safe)

javiereguiluz · 2024-10-09T15:14:05Z

@nicolas-grekas that would be like iterating over the idea implemented in #31514. But, this proposal tries to fix this for real and never output HTML content in the console.

I pushed a commit a043bf7 trying to do some of the things that you said, but honestly I don't know what I'm doing 😐

javiereguiluz · 2024-10-09T15:37:29Z

Nicolas gave me more clues about how to solve this so I submitted a new commit with a different approach. Thanks.

src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/ErrorRendererPass.php

stof · 2024-10-09T15:52:27Z

src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/ErrorRendererPass.php

+use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
+use Symfony\Component\Workflow\Workflow;
+use Symfony\Component\Yaml\Yaml;
+use function Symfony\Component\DependencyInjection\Loader\Configurator\inline_service;


this is unused (like many other use statements)

stof · 2024-10-09T15:53:33Z

src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/ErrorRendererPass.php

+        // in the 'test' environment, use the CLI error renderer as the default one
+        if ($container->hasDefinition('test.client')) {
+            $container->getDefinition('twig.error_renderer.html')
+                ->setArgument(1, new Definition(CliErrorRenderer::class));


wouldn't this break if the error-handler component is not installed (which is not a dependency of the bundle right now) ?

and maybe we need a conflict with older versions of the component to ensure we don't inject a version that does not properly report a plaintext content type (and so is subject to XSS)

yceruto · 2024-10-09T16:01:17Z

src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/ErrorRendererPass.php

+        // in the 'test' environment, use the CLI error renderer as the default one
+        if ($container->hasDefinition('test.client')) {
+            $container->getDefinition('twig.error_renderer.html')
+                ->setArgument(1, new Definition(CliErrorRenderer::class));


I think the decision of which fallback error renderer is the best does not depend on the environment but on the SAPI value, as you did at the beginning. What about creating a service factory that injects the proper error renderer depending on the SAPI value?

yceruto · 2024-10-09T16:11:02Z

By the way, the issue described (PR description) also applies when the twig-bundle is not installed, so updating the TwigErrorRenderer service is not enough.

nicolas-grekas · 2024-10-09T16:24:52Z

Solving this in a bundle looks appropriate to me. DI is the proper way to improve the DX here. Let's keep things simple.

yceruto · 2024-10-09T19:34:46Z

My 5 cents on achieving this by using a factory to determine which renderer is most appropriate, depending on the SAPI value:

Patch (Click to open)

diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/error_renderer.php b/src/Symfony/Bundle/FrameworkBundle/Resources/config/error_renderer.php
index 67f28ce44d..a40fb2aa97 100644
--- a/src/Symfony/Bundle/FrameworkBundle/Resources/config/error_renderer.php
+++ b/src/Symfony/Bundle/FrameworkBundle/Resources/config/error_renderer.php
@@ -11,7 +11,10 @@
 
 namespace Symfony\Component\DependencyInjection\Loader\Configurator;
 
+use Symfony\Component\ErrorHandler\ErrorRenderer\CliErrorRenderer;
+use Symfony\Component\ErrorHandler\ErrorRenderer\ErrorRendererInterface;
 use Symfony\Component\ErrorHandler\ErrorRenderer\HtmlErrorRenderer;
+use Symfony\Component\ErrorHandler\ErrorRenderer\SapiErrorRendererFactory;
 
 return static function (ContainerConfigurator $container) {
     $container->services()
@@ -32,7 +35,18 @@ return static function (ContainerConfigurator $container) {
                 service('logger')->nullOnInvalid(),
             ])
 
+        ->set('error_handler.error_renderer.cli', CliErrorRenderer::class)
+
+        ->set('error_handler.error_renderer.default', ErrorRendererInterface::class)
+            ->factory([SapiErrorRendererFactory::class, 'create'])
+            ->args([
+                service('error_renderer.cli'),
+                service('error_renderer.html'),
+            ])
+
         ->alias('error_renderer.html', 'error_handler.error_renderer.html')
-        ->alias('error_renderer', 'error_renderer.html')
+        ->alias('error_renderer.cli', 'error_handler.error_renderer.cli')
+        ->alias('error_renderer.default', 'error_handler.error_renderer.default')
+        ->alias('error_renderer', 'error_renderer.default')
     ;
 };
diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/serializer.php b/src/Symfony/Bundle/FrameworkBundle/Resources/config/serializer.php
index d689d42995..eadc71db13 100644
--- a/src/Symfony/Bundle/FrameworkBundle/Resources/config/serializer.php
+++ b/src/Symfony/Bundle/FrameworkBundle/Resources/config/serializer.php
@@ -213,7 +213,7 @@ return static function (ContainerConfigurator $container) {
                 inline_service()
                     ->factory([SerializerErrorRenderer::class, 'getPreferredFormat'])
                     ->args([service('request_stack')]),
-                service('error_renderer.html'),
+                service('error_renderer.default'),
                 inline_service()
                     ->factory([HtmlErrorRenderer::class, 'isDebug'])
                     ->args([service('request_stack'), param('kernel.debug')]),
diff --git a/src/Symfony/Bundle/FrameworkBundle/composer.json b/src/Symfony/Bundle/FrameworkBundle/composer.json
index af83a9a13f..fe49a27735 100644
--- a/src/Symfony/Bundle/FrameworkBundle/composer.json
+++ b/src/Symfony/Bundle/FrameworkBundle/composer.json
@@ -23,7 +23,7 @@
         "symfony/config": "^6.4|^7.0",
         "symfony/dependency-injection": "^7.2",
         "symfony/deprecation-contracts": "^2.5|^3",
-        "symfony/error-handler": "^6.4|^7.0",
+        "symfony/error-handler": "^7.2",
         "symfony/event-dispatcher": "^6.4|^7.0",
         "symfony/http-foundation": "^6.4|^7.0",
         "symfony/http-kernel": "^6.4|^7.0",
diff --git a/src/Symfony/Component/ErrorHandler/ErrorRenderer/SapiErrorRendererFactory.php b/src/Symfony/Component/ErrorHandler/ErrorRenderer/SapiErrorRendererFactory.php
new file mode 100644
index 0000000000..89b6e5b75c
--- /dev/null
+++ b/src/Symfony/Component/ErrorHandler/ErrorRenderer/SapiErrorRendererFactory.php
@@ -0,0 +1,20 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\ErrorHandler\ErrorRenderer;
+
+final class SapiErrorRendererFactory
+{
+    public static function create(ErrorRendererInterface $cliErrorRenderer, ErrorRendererInterface $htmlErrorRenderer): ErrorRendererInterface
+    {
+        return \in_array(\PHP_SAPI, ['cli', 'phpdbg', 'embed'], true) ? $cliErrorRenderer : $htmlErrorRenderer;
+    }
+}

DI graph after the patch:

In fact, no change is required in TwigErrorRenderer as it's just a decorator of the html renderer.

javiereguiluz · 2025-03-25T08:20:16Z

Let's close this in favor of #60033, which tries to achieve the same goals but with a different strategy. Thanks Yonel!

javiereguiluz added Feature TwigBridge labels Oct 4, 2024

javiereguiluz added this to the 7.2 milestone Oct 4, 2024

carsonbot changed the title ~~Improve error rendering when running tests~~ [TwigBridge] Improve error rendering when running tests Oct 4, 2024

carsonbot added the Status: Needs Review label Oct 4, 2024

stof reviewed Oct 4, 2024

View reviewed changes

src/Symfony/Bridge/Twig/ErrorRenderer/TwigErrorRenderer.php Outdated Show resolved Hide resolved

javiereguiluz requested a review from yceruto as a code owner October 7, 2024 07:11

javiereguiluz added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 7, 2024

alexandre-daubois approved these changes Oct 8, 2024

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 8, 2024

yceruto reviewed Oct 8, 2024

View reviewed changes

src/Symfony/Bridge/Twig/ErrorRenderer/TwigErrorRenderer.php Outdated Show resolved Hide resolved

javiereguiluz added 3 commits October 9, 2024 08:32

Improve error rendering when running tests

32d771e

Fixes and improvements

b372dd7

Fix the property type

1ce9351

javiereguiluz force-pushed the cli_errors branch from 52d8157 to 1ce9351 Compare October 9, 2024 06:32

javiereguiluz added 5 commits October 9, 2024 08:39

Fix the property type for real

6b9fa3d

Update tests

fc8cbe1

Update tests

3895881

Fix code syntax

04882bd

Fix tests in some Symfony versions

7eaf087

nicolas-grekas reviewed Oct 9, 2024

View reviewed changes

Changes according to reviews

a043bf7

Another iteration

332ee7e

javiereguiluz added 2 commits October 9, 2024 17:43

Fix code syntax

aeac330

Add the missing compiler pass file

181729c

stof requested changes Oct 9, 2024

View reviewed changes

src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/ErrorRendererPass.php Show resolved Hide resolved

src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/ErrorRendererPass.php Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Reviewed labels Oct 9, 2024

stof reviewed Oct 9, 2024

View reviewed changes

Fixes

00575a5

yceruto reviewed Oct 9, 2024

View reviewed changes

fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024

yceruto mentioned this pull request Mar 24, 2025

[ErrorHandler] Add SapiErrorRendererSelector for context-based error rendering #60033

Open

javiereguiluz closed this Mar 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TwigBridge] Improve error rendering when running tests #58456

[TwigBridge] Improve error rendering when running tests #58456

javiereguiluz commented Oct 4, 2024 •

edited

Loading

stof left a comment

javiereguiluz commented Oct 7, 2024

alexandre-daubois left a comment •

edited

Loading

nicolas-grekas Oct 9, 2024 •

edited

Loading

javiereguiluz Oct 9, 2024

stof Oct 9, 2024

nicolas-grekas Oct 9, 2024

javiereguiluz Oct 9, 2024

nicolas-grekas commented Oct 9, 2024 •

edited

Loading

javiereguiluz commented Oct 9, 2024

javiereguiluz commented Oct 9, 2024

stof Oct 9, 2024 •

edited by OskarStark

Loading

stof Oct 9, 2024

stof Oct 9, 2024

yceruto Oct 9, 2024

yceruto commented Oct 9, 2024 •

edited

Loading

nicolas-grekas commented Oct 9, 2024

yceruto commented Oct 9, 2024 •

edited

Loading

javiereguiluz commented Mar 25, 2025

[TwigBridge] Improve error rendering when running tests #58456

[TwigBridge] Improve error rendering when running tests #58456

Conversation

javiereguiluz commented Oct 4, 2024 • edited Loading

stof left a comment

Choose a reason for hiding this comment

javiereguiluz commented Oct 7, 2024

alexandre-daubois left a comment • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

javiereguiluz Oct 9, 2024

Choose a reason for hiding this comment

stof Oct 9, 2024

Choose a reason for hiding this comment

nicolas-grekas Oct 9, 2024

Choose a reason for hiding this comment

javiereguiluz Oct 9, 2024

Choose a reason for hiding this comment

nicolas-grekas commented Oct 9, 2024 • edited Loading

javiereguiluz commented Oct 9, 2024

javiereguiluz commented Oct 9, 2024

stof Oct 9, 2024 • edited by OskarStark Loading

Choose a reason for hiding this comment

stof Oct 9, 2024

Choose a reason for hiding this comment

stof Oct 9, 2024

Choose a reason for hiding this comment

yceruto Oct 9, 2024

Choose a reason for hiding this comment

yceruto commented Oct 9, 2024 • edited Loading

nicolas-grekas commented Oct 9, 2024

yceruto commented Oct 9, 2024 • edited Loading

javiereguiluz commented Mar 25, 2025

javiereguiluz commented Oct 4, 2024 •

edited

Loading

alexandre-daubois left a comment •

edited

Loading

nicolas-grekas Oct 9, 2024 •

edited

Loading

nicolas-grekas commented Oct 9, 2024 •

edited

Loading

stof Oct 9, 2024 •

edited by OskarStark

Loading

yceruto commented Oct 9, 2024 •

edited

Loading

yceruto commented Oct 9, 2024 •

edited

Loading