From 310986eb2aad85ae9556fed47bef5919a29832a1 Mon Sep 17 00:00:00 2001 From: Kamil Kokot Date: Thu, 11 Feb 2021 15:29:20 +0000 Subject: [PATCH] Fix timeout on 404 Not Found errors when using templating and Symfony 4.4 --- phpstan.neon | 6 + psalm.xml | 2 + ...ependencyBreakingExceptionListenerPass.php | 40 +++++ ...ircularDependencyBreakingErrorListener.php | 5 +- ...larDependencyBreakingExceptionListener.php | 96 ++++++++++++ .../Bundle/CoreBundle/SyliusCoreBundle.php | 2 + ...ependencyBreakingErrorListenerPassTest.php | 5 +- ...dencyBreakingExceptionListenerPassTest.php | 45 ++++++ ...larDependencyBreakingErrorListenerTest.php | 148 ++++++++++++++++++ ...ependencyBreakingExceptionListenerTest.php | 16 +- 10 files changed, 354 insertions(+), 11 deletions(-) create mode 100644 src/Sylius/Bundle/CoreBundle/DependencyInjection/Compiler/CircularDependencyBreakingExceptionListenerPass.php create mode 100644 src/Sylius/Bundle/CoreBundle/EventListener/CircularDependencyBreakingExceptionListener.php create mode 100644 src/Sylius/Bundle/CoreBundle/Tests/DependencyInjection/CircularDependencyBreakingExceptionListenerPassTest.php create mode 100644 src/Sylius/Bundle/CoreBundle/Tests/Listener/CircularDependencyBreakingErrorListenerTest.php diff --git a/phpstan.neon b/phpstan.neon index c170f33e950..6b05506b2a5 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -23,5 +23,11 @@ parameters: - 'src/Sylius/Bundle/ApiBundle/DependencyInjection/Compiler/ReflectionExtractorHotfixPass.php' - 'src/Sylius/Bundle/ApiBundle/PropertyInfo/Extractor/ReflectionExtractor.php' + # Symfony 4.4-specific issues + - 'src/Sylius/Bundle/CoreBundle/DependencyInjection/Compiler/CircularDependencyBreakingExceptionListenerPass.php' + - 'src/Sylius/Bundle/CoreBundle/EventListener/CircularDependencyBreakingExceptionListener.php' + - 'src/Sylius/Bundle/CoreBundle/Tests/DependencyInjection/CircularDependencyBreakingExceptionListenerPassTest.php' + - 'src/Sylius/Bundle/CoreBundle/Tests/Listener/CircularDependencyBreakingExceptionListenerTest.php' + ignoreErrors: - '/Access to an undefined property Doctrine\\Common\\Collections\\ArrayCollection/' diff --git a/psalm.xml b/psalm.xml index af5b8ed3049..b2e9d1456a7 100644 --- a/psalm.xml +++ b/psalm.xml @@ -91,6 +91,8 @@ + + diff --git a/src/Sylius/Bundle/CoreBundle/DependencyInjection/Compiler/CircularDependencyBreakingExceptionListenerPass.php b/src/Sylius/Bundle/CoreBundle/DependencyInjection/Compiler/CircularDependencyBreakingExceptionListenerPass.php new file mode 100644 index 00000000000..5327c416b56 --- /dev/null +++ b/src/Sylius/Bundle/CoreBundle/DependencyInjection/Compiler/CircularDependencyBreakingExceptionListenerPass.php @@ -0,0 +1,40 @@ +has('twig.exception_listener')) { + return; + } + + $definition = new Definition(CircularDependencyBreakingExceptionListener::class); + $definition->setDecoratedService('twig.exception_listener'); + $definition->addArgument(new Reference(CircularDependencyBreakingExceptionListener::class .'.inner')); + + $container->setDefinition( + CircularDependencyBreakingExceptionListener::class, + $definition + ); + } +} diff --git a/src/Sylius/Bundle/CoreBundle/EventListener/CircularDependencyBreakingErrorListener.php b/src/Sylius/Bundle/CoreBundle/EventListener/CircularDependencyBreakingErrorListener.php index 0d49886084d..8aa0a078a80 100644 --- a/src/Sylius/Bundle/CoreBundle/EventListener/CircularDependencyBreakingErrorListener.php +++ b/src/Sylius/Bundle/CoreBundle/EventListener/CircularDependencyBreakingErrorListener.php @@ -19,6 +19,8 @@ use Symfony\Component\HttpKernel\EventListener\ErrorListener; /** + * For Symfony 5+. + * * `Symfony\Component\HttpKernel\EventListener\ErrorListener::onKernelException` happens to set previous * exception for a wrapper exception. This is meant to improve DX while debugging nested exceptions, * but also creates some issues. @@ -45,7 +47,7 @@ */ final class CircularDependencyBreakingErrorListener extends ErrorListener { - /** @var CircularDependencyBreakingErrorListener */ + /** @var ErrorListener */ private $decoratedListener; public function __construct(ErrorListener $decoratedListener) @@ -61,6 +63,7 @@ public function logKernelException(ExceptionEvent $event): void public function onKernelException(ExceptionEvent $event, string $eventName = null, EventDispatcherInterface $eventDispatcher = null): void { try { + /** @psalm-suppress TooManyArguments */ $this->decoratedListener->onKernelException($event, $eventName, $eventDispatcher); } catch (\Throwable $throwable) { $this->breakCircularDependency($throwable); diff --git a/src/Sylius/Bundle/CoreBundle/EventListener/CircularDependencyBreakingExceptionListener.php b/src/Sylius/Bundle/CoreBundle/EventListener/CircularDependencyBreakingExceptionListener.php new file mode 100644 index 00000000000..741b536b43a --- /dev/null +++ b/src/Sylius/Bundle/CoreBundle/EventListener/CircularDependencyBreakingExceptionListener.php @@ -0,0 +1,96 @@ +getPrevious()->getPrevious()->getPrevious() === $exception` + * + * That exception is rethrown and other listeners like `Symfony\Component\Security\Http\Firewall\ExceptionListener` + * try to deal with an exception and all their previous ones, causing infinite loops. + * + * This fix only works if "framework.templating" setting DOES NOT include "twig". Otherwise, TwigBundle + * registers deprecated `Symfony\Component\HttpKernel\EventListener\ExceptionListener`, removes the non-deprecated + * "exception_listener" service, so that the issue still persists. + * + * This listener behaves as a decorator, but also extends the original ExceptionListener, because of yet another + * listener `ApiPlatform\Core\EventListener\ExceptionListener` requires the original class. + * + * @internal + * + * @see \Symfony\Component\HttpKernel\EventListener\ExceptionListener + * + * @psalm-suppress UndefinedClass + * @psalm-suppress MissingDependency + */ +final class CircularDependencyBreakingExceptionListener extends ExceptionListener +{ + /** @var ExceptionListener */ + private $decoratedListener; + + public function __construct(ExceptionListener $decoratedListener) + { + $this->decoratedListener = $decoratedListener; + } + + public function logKernelException(GetResponseForExceptionEvent $event): void + { + $this->decoratedListener->logKernelException($event); + } + + public function onKernelException(GetResponseForExceptionEvent $event): void + { + try { + $this->decoratedListener->onKernelException($event); + } catch (\Throwable $throwable) { + $this->breakCircularDependency($throwable); + + throw $throwable; + } + } + + private function breakCircularDependency(\Throwable $throwable): void + { + $throwables = []; + + do { + $throwables[] = $throwable; + + if (in_array($throwable->getPrevious(), $throwables, true)) { + $this->removePreviousFromThrowable($throwable); + } + + $throwable = $throwable->getPrevious(); + } while (null !== $throwable); + } + + private function removePreviousFromThrowable(\Throwable $throwable): void + { + $previous = new \ReflectionProperty($throwable instanceof \Exception ? \Exception::class : \Error::class, 'previous'); + $previous->setAccessible(true); + $previous->setValue($throwable, null); + } +} diff --git a/src/Sylius/Bundle/CoreBundle/SyliusCoreBundle.php b/src/Sylius/Bundle/CoreBundle/SyliusCoreBundle.php index b7ffad871a4..0290d5caf25 100644 --- a/src/Sylius/Bundle/CoreBundle/SyliusCoreBundle.php +++ b/src/Sylius/Bundle/CoreBundle/SyliusCoreBundle.php @@ -22,6 +22,7 @@ use Doctrine\Inflector\Rules\Word; use Sylius\Bundle\CoreBundle\DependencyInjection\Compiler\BackwardsCompatibility\ResolveShopUserTargetEntityPass; use Sylius\Bundle\CoreBundle\DependencyInjection\Compiler\CircularDependencyBreakingErrorListenerPass; +use Sylius\Bundle\CoreBundle\DependencyInjection\Compiler\CircularDependencyBreakingExceptionListenerPass; use Sylius\Bundle\CoreBundle\DependencyInjection\Compiler\IgnoreAnnotationsPass; use Sylius\Bundle\CoreBundle\DependencyInjection\Compiler\LazyCacheWarmupPass; use Sylius\Bundle\CoreBundle\DependencyInjection\Compiler\LiipImageFiltersPass; @@ -62,6 +63,7 @@ public function build(ContainerBuilder $container): void parent::build($container); $container->addCompilerPass(new CircularDependencyBreakingErrorListenerPass()); + $container->addCompilerPass(new CircularDependencyBreakingExceptionListenerPass()); $container->addCompilerPass(new LazyCacheWarmupPass()); $container->addCompilerPass(new RegisterTaxCalculationStrategiesPass()); $container->addCompilerPass(new TranslatableEntityLocalePass()); diff --git a/src/Sylius/Bundle/CoreBundle/Tests/DependencyInjection/CircularDependencyBreakingErrorListenerPassTest.php b/src/Sylius/Bundle/CoreBundle/Tests/DependencyInjection/CircularDependencyBreakingErrorListenerPassTest.php index fd2926d6923..cfbabf80a1f 100644 --- a/src/Sylius/Bundle/CoreBundle/Tests/DependencyInjection/CircularDependencyBreakingErrorListenerPassTest.php +++ b/src/Sylius/Bundle/CoreBundle/Tests/DependencyInjection/CircularDependencyBreakingErrorListenerPassTest.php @@ -17,6 +17,7 @@ use Sylius\Bundle\CoreBundle\DependencyInjection\Compiler\CircularDependencyBreakingErrorListenerPass; use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\DependencyInjection\Definition; +use Sylius\Bundle\CoreBundle\EventListener\CircularDependencyBreakingErrorListener; final class CircularDependencyBreakingErrorListenerPassTest extends AbstractCompilerPassTestCase { @@ -27,14 +28,14 @@ public function it_register_circular_dependency_breaking_error_listener_when_exc $this->compile(); - $this->assertContainerBuilderHasService('Sylius\Bundle\CoreBundle\EventListener\CircularDependencyBreakingErrorListener'); + $this->assertContainerBuilderHasService(CircularDependencyBreakingErrorListener::class); } public function it_does_nothing_when_exception_listener_is_not_registered(): void { $this->compile(); - $this->assertContainerBuilderNotHasService('Sylius\Bundle\CoreBundle\EventListener\CircularDependencyBreakingErrorListener'); + $this->assertContainerBuilderNotHasService(CircularDependencyBreakingErrorListener::class); } protected function registerCompilerPass(ContainerBuilder $container): void diff --git a/src/Sylius/Bundle/CoreBundle/Tests/DependencyInjection/CircularDependencyBreakingExceptionListenerPassTest.php b/src/Sylius/Bundle/CoreBundle/Tests/DependencyInjection/CircularDependencyBreakingExceptionListenerPassTest.php new file mode 100644 index 00000000000..a360acbacad --- /dev/null +++ b/src/Sylius/Bundle/CoreBundle/Tests/DependencyInjection/CircularDependencyBreakingExceptionListenerPassTest.php @@ -0,0 +1,45 @@ +container->setDefinition('twig.exception_listener', new Definition('ExceptionListener')); + + $this->compile(); + + $this->assertContainerBuilderHasService(CircularDependencyBreakingExceptionListener::class); + } + + public function it_does_nothing_when_exception_listener_is_not_registered(): void + { + $this->compile(); + + $this->assertContainerBuilderNotHasService(CircularDependencyBreakingExceptionListener::class); + } + + protected function registerCompilerPass(ContainerBuilder $container): void + { + $container->addCompilerPass(new CircularDependencyBreakingExceptionListenerPass()); + } +} diff --git a/src/Sylius/Bundle/CoreBundle/Tests/Listener/CircularDependencyBreakingErrorListenerTest.php b/src/Sylius/Bundle/CoreBundle/Tests/Listener/CircularDependencyBreakingErrorListenerTest.php new file mode 100644 index 00000000000..0d1d8eb6733 --- /dev/null +++ b/src/Sylius/Bundle/CoreBundle/Tests/Listener/CircularDependencyBreakingErrorListenerTest.php @@ -0,0 +1,148 @@ +createMock(ErrorListener::class); + $listener = new CircularDependencyBreakingErrorListener($decoratedListener); + + $event = $this->createExceptionEvent(); + + $secondException = new \Exception('Second'); + $firstException = new \Exception('First', 0, $secondException); + $this->setPreviousForException($secondException, $firstException); + + $decoratedListener->method('onKernelException')->willThrowException($firstException); + + // Pre-assert + Assert::assertSame($secondException, $firstException->getPrevious()); + Assert::assertSame($firstException, $secondException->getPrevious()); + + // Act + $throwable = null; + + try { + $listener->onKernelException($event); + } catch (\Throwable $throwable) { + } + + // Assert + Assert::assertNotNull($throwable); + Assert::assertSame($firstException, $throwable); + Assert::assertSame($secondException, $firstException->getPrevious()); + Assert::assertSame(null, $secondException->getPrevious()); + } + + /** @test */ + public function it_breaks_more_complex_circular_dependencies_in_exceptions(): void + { + // Arrange + $decoratedListener = $this->createMock(ErrorListener::class); + $listener = new CircularDependencyBreakingErrorListener($decoratedListener); + + $event = $this->createExceptionEvent(); + + $fourthException = new \Exception('Fourth'); + $thirdException = new \Exception('Third', 0, $fourthException); + $secondException = new \Exception('Second', 0, $thirdException); + $firstException = new \Exception('First', 0, $secondException); + $this->setPreviousForException($fourthException, $secondException); + + $decoratedListener->method('onKernelException')->willThrowException($firstException); + + // Pre-assert + Assert::assertSame($secondException, $firstException->getPrevious()); + Assert::assertSame($thirdException, $secondException->getPrevious()); + Assert::assertSame($fourthException, $thirdException->getPrevious()); + Assert::assertSame($secondException, $fourthException->getPrevious()); + + // Act + $throwable = null; + + try { + $listener->onKernelException($event); + } catch (\Throwable $throwable) { + } + + // Assert + Assert::assertNotNull($throwable); + Assert::assertSame($firstException, $throwable); + Assert::assertSame($secondException, $firstException->getPrevious()); + Assert::assertSame($thirdException, $secondException->getPrevious()); + Assert::assertSame($fourthException, $thirdException->getPrevious()); + Assert::assertSame(null, $fourthException->getPrevious()); + } + + /** @test */ + public function it_does_nothing_when_circular_dependencies_are_not_found(): void + { + // Arrange + $decoratedListener = $this->createMock(ErrorListener::class); + $listener = new CircularDependencyBreakingErrorListener($decoratedListener); + + $event = $this->createExceptionEvent(); + + $secondException = new \Exception('Second'); + $firstException = new \Exception('First', 0, $secondException); + + $decoratedListener->method('onKernelException')->willThrowException($firstException); + + // Pre-assert + Assert::assertSame($secondException, $firstException->getPrevious()); + Assert::assertSame(null, $secondException->getPrevious()); + + // Act + $throwable = null; + + try { + $listener->onKernelException($event); + } catch (\Throwable $throwable) { + } + + // Assert + Assert::assertNotNull($throwable); + Assert::assertSame($firstException, $throwable); + Assert::assertSame($secondException, $firstException->getPrevious()); + Assert::assertSame(null, $secondException->getPrevious()); + } + + private function setPreviousForException(\Exception $exception, ?\Exception $previous): void + { + $property = new \ReflectionProperty(\Exception::class, 'previous'); + $property->setAccessible(true); + $property->setValue($exception, $previous); + } + + private function createExceptionEvent(): ExceptionEvent + { + $kernel = $this->createMock(HttpKernelInterface::class); + $request = $this->createMock(Request::class); + $exception = new \Exception(); + + return new ExceptionEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST, $exception); + } +} diff --git a/src/Sylius/Bundle/CoreBundle/Tests/Listener/CircularDependencyBreakingExceptionListenerTest.php b/src/Sylius/Bundle/CoreBundle/Tests/Listener/CircularDependencyBreakingExceptionListenerTest.php index 732756bf412..d191bc028a1 100644 --- a/src/Sylius/Bundle/CoreBundle/Tests/Listener/CircularDependencyBreakingExceptionListenerTest.php +++ b/src/Sylius/Bundle/CoreBundle/Tests/Listener/CircularDependencyBreakingExceptionListenerTest.php @@ -15,10 +15,10 @@ use PHPUnit\Framework\Assert; use PHPUnit\Framework\TestCase; -use Sylius\Bundle\CoreBundle\EventListener\CircularDependencyBreakingErrorListener; +use Sylius\Bundle\CoreBundle\EventListener\CircularDependencyBreakingExceptionListener; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpKernel\Event\ExceptionEvent; -use Symfony\Component\HttpKernel\EventListener\ErrorListener; +use Symfony\Component\HttpKernel\EventListener\ExceptionListener; use Symfony\Component\HttpKernel\HttpKernelInterface; final class CircularDependencyBreakingExceptionListenerTest extends TestCase @@ -27,8 +27,8 @@ final class CircularDependencyBreakingExceptionListenerTest extends TestCase public function it_breaks_circular_dependencies_in_exceptions(): void { // Arrange - $decoratedListener = $this->createMock(ErrorListener::class); - $listener = new CircularDependencyBreakingErrorListener($decoratedListener); + $decoratedListener = $this->createMock(ExceptionListener::class); + $listener = new CircularDependencyBreakingExceptionListener($decoratedListener); $event = $this->createExceptionEvent(); @@ -61,8 +61,8 @@ public function it_breaks_circular_dependencies_in_exceptions(): void public function it_breaks_more_complex_circular_dependencies_in_exceptions(): void { // Arrange - $decoratedListener = $this->createMock(ErrorListener::class); - $listener = new CircularDependencyBreakingErrorListener($decoratedListener); + $decoratedListener = $this->createMock(ExceptionListener::class); + $listener = new CircularDependencyBreakingExceptionListener($decoratedListener); $event = $this->createExceptionEvent(); @@ -101,8 +101,8 @@ public function it_breaks_more_complex_circular_dependencies_in_exceptions(): vo public function it_does_nothing_when_circular_dependencies_are_not_found(): void { // Arrange - $decoratedListener = $this->createMock(ErrorListener::class); - $listener = new CircularDependencyBreakingErrorListener($decoratedListener); + $decoratedListener = $this->createMock(ExceptionListener::class); + $listener = new CircularDependencyBreakingExceptionListener($decoratedListener); $event = $this->createExceptionEvent();