Skip to content

[TwigBridge] Improve error rendering when running tests #58456

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 13 commits into from
4 changes: 2 additions & 2 deletions src/Symfony/Bridge/Twig/ErrorRenderer/TwigErrorRenderer.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,15 @@
*/
class TwigErrorRenderer implements ErrorRendererInterface
{
private HtmlErrorRenderer $fallbackErrorRenderer;
private ErrorRendererInterface $fallbackErrorRenderer;
private \Closure|bool $debug;

/**
* @param bool|callable $debug The debugging mode as a boolean or a callable that should return it
*/
public function __construct(
private Environment $twig,
?HtmlErrorRenderer $fallbackErrorRenderer = null,
?ErrorRendererInterface $fallbackErrorRenderer = null,
bool|callable $debug = false,
) {
$this->fallbackErrorRenderer = $fallbackErrorRenderer ?? new HtmlErrorRenderer();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,44 +13,40 @@

use PHPUnit\Framework\TestCase;
use Symfony\Bridge\Twig\ErrorRenderer\TwigErrorRenderer;
use Symfony\Component\ErrorHandler\ErrorRenderer\HtmlErrorRenderer;
use Symfony\Component\ErrorHandler\ErrorRenderer\CliErrorRenderer;
use Symfony\Component\ErrorHandler\Exception\FlattenException;
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
use Twig\Environment;
use Twig\Loader\ArrayLoader;

class TwigErrorRendererTest extends TestCase
{
public function testFallbackToNativeRendererIfDebugOn()
public function tesFallbackRenderer()
{
$exception = new \Exception();

$twig = $this->createMock(Environment::class);
$nativeRenderer = $this->createMock(HtmlErrorRenderer::class);
$nativeRenderer
->expects($this->once())
->method('render')
->with($exception)
;

(new TwigErrorRenderer($twig, $nativeRenderer, true))->render(new \Exception());
$customRenderer = new class implements ErrorRendererInterface {
public function render(\Throwable $exception): FlattenException
{
return 'This is a custom error renderer.';
}
};

$this->assertSame('This is a custom error renderer.', (new TwigErrorRenderer($twig, $customRenderer, true))->render(new \Exception()));
}

public function testFallbackToNativeRendererIfCustomTemplateNotFound()
public function testCliRenderer()
{
$exception = new NotFoundHttpException();

$twig = new Environment(new ArrayLoader([]));

$nativeRenderer = $this->createMock(HtmlErrorRenderer::class);
$nativeRenderer
->expects($this->once())
->method('render')
->with($exception)
->willReturn(FlattenException::createFromThrowable($exception))
;
$exception = (new TwigErrorRenderer($twig, new CliErrorRenderer(), false))->render($exception);

$exceptionHeaders = $exception->getHeaders();
if (isset($exceptionHeaders['Content-Type'])) {
$this->assertSame('text/plain', $exceptionHeaders['Content-Type'], 'The exception does not return HTML contents (to prevent potential XSS vulnerabilities)');
}

(new TwigErrorRenderer($twig, $nativeRenderer, false))->render($exception);
$this->assertStringNotContainsString('<!DOCTYPE html>', $exception->getAsString(), 'The exception does not include elements of the HTML exception page');
}

public function testRenderCustomErrorTemplate()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<?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\Bundle\TwigBundle\DependencyInjection\Compiler;

use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\ErrorHandler\ErrorRenderer\CliErrorRenderer;

/**
* @author Javier Eguiluz <javier.eguiluz@gmail.com>
* @internal
*/
class ErrorRendererPass implements CompilerPassInterface
{
public function process(ContainerBuilder $container): void
{
// 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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
}
}
2 changes: 2 additions & 0 deletions src/Symfony/Bundle/TwigBundle/TwigBundle.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Bundle\TwigBundle;

use Symfony\Bundle\TwigBundle\DependencyInjection\Compiler\ErrorRendererPass;
use Symfony\Bundle\TwigBundle\DependencyInjection\Compiler\ExtensionPass;
use Symfony\Bundle\TwigBundle\DependencyInjection\Compiler\RuntimeLoaderPass;
use Symfony\Bundle\TwigBundle\DependencyInjection\Compiler\TwigEnvironmentPass;
Expand All @@ -36,6 +37,7 @@ public function build(ContainerBuilder $container): void
$container->addCompilerPass(new TwigEnvironmentPass());
$container->addCompilerPass(new TwigLoaderPass());
$container->addCompilerPass(new RuntimeLoaderPass(), PassConfig::TYPE_BEFORE_REMOVING);
$container->addCompilerPass(new ErrorRendererPass());
}

public function registerCommands(Application $application): void
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Bundle/TwigBundle/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"composer-runtime-api": ">=2.1",
"symfony/config": "^6.4|^7.0",
"symfony/dependency-injection": "^6.4|^7.0",
"symfony/twig-bridge": "^6.4|^7.0",
"symfony/twig-bridge": "^7.2",
"symfony/http-foundation": "^6.4|^7.0",
"symfony/http-kernel": "^6.4|^7.0",
"twig/twig": "^3.12|^4.0"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ protected function supportsColors(): bool
}
};

return FlattenException::createFromThrowable($exception)
return FlattenException::createFromThrowable($exception, headers: ['Content-Type' => 'text/plain'])
->setAsString($dumper->dump($cloner->cloneVar($exception), true));
}
}
Loading