Fix timeout on 404 Not Found errors when using templating and Symfony…

… 4.4

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/'

psalm.xml

@@ -91,6 +91,8 @@
                 <referencedClass name="Payum\Core\Security\GenericTokenFactoryInterface" />
                 <referencedClass name="Sylius\Component\Core\Calculator\ProductVariantPriceCalculatorInterface" />
                 <referencedClass name="Symfony\Bundle\FrameworkBundle\Templating\EngineInterface" /> <!-- deprecated in Symfony 4.3 -->
+                <referencedClass name="Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent" /> <!-- deprected in Symfony 4.4 -->
+                <referencedClass name="Symfony\Component\HttpKernel\EventListener\ExceptionListener" /> <!-- deprected in Symfony 4.4 -->
                 <referencedClass name="Symfony\Component\Routing\RouteCollectionBuilder" /> <!-- deprecated in Symfony 5.1 -->
                 <referencedClass name="Symfony\Component\Security\Core\Role\Role" /> <!-- deprecated in Symfony 4.3 -->
                 <referencedClass name="Symfony\Component\Security\Http\Logout\DefaultLogoutSuccessHandler" /> <!-- deprecated in Symfony 5.1 -->

src/Sylius/Bundle/CoreBundle/DependencyInjection/Compiler/CircularDependencyBreakingExceptionListenerPass.php

@@ -0,0 +1,40 @@
+<?php
+
+/*
+ * This file is part of the Sylius package.
+ *
+ * (c) Paweł Jędrzejewski
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+declare(strict_types=1);
+
+namespace Sylius\Bundle\CoreBundle\DependencyInjection\Compiler;
+
+use Sylius\Bundle\CoreBundle\EventListener\CircularDependencyBreakingExceptionListener;
+use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Definition;
+use Symfony\Component\DependencyInjection\Reference;
+
+final class CircularDependencyBreakingExceptionListenerPass implements CompilerPassInterface
+{
+    /** @psalm-suppress MissingDependency */
+    public function process(ContainerBuilder $container): void
+    {
+        if (!$container->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
+        );
+    }
+}

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);

src/Sylius/Bundle/CoreBundle/EventListener/CircularDependencyBreakingExceptionListener.php

@@ -0,0 +1,96 @@
+<?php
+
+/*
+ * This file is part of the Sylius package.
+ *
+ * (c) Paweł Jędrzejewski
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+declare(strict_types=1);
+
+namespace Sylius\Bundle\CoreBundle\EventListener;
+
+use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent;
+use Symfony\Component\HttpKernel\EventListener\ExceptionListener;
+
+/**
+ * For Symfony 4.
+ *
+ * `Symfony\Component\HttpKernel\EventListener\ExceptionListener::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.
+ *
+ * After upgrading to ResourceBundle v1.7 and GridBundle v1.8, the test suite started to fail
+ * because of a timeout. By artifically setting the previous exception, in some cases it created
+ * an exception with circular dependencies, so that:
+ *
+ *   `$exception->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);
+    }
+}

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());

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

src/Sylius/Bundle/CoreBundle/Tests/DependencyInjection/CircularDependencyBreakingExceptionListenerPassTest.php

@@ -0,0 +1,45 @@
+<?php
+
+/*
+ * This file is part of the Sylius package.
+ *
+ * (c) Paweł Jędrzejewski
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+declare(strict_types=1);
+
+namespace Sylius\Bundle\CoreBundle\Tests\DependencyInjection;
+
+use Matthias\SymfonyDependencyInjectionTest\PhpUnit\AbstractCompilerPassTestCase;
+use Sylius\Bundle\CoreBundle\DependencyInjection\Compiler\CircularDependencyBreakingExceptionListenerPass;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Definition;
+use Sylius\Bundle\CoreBundle\EventListener\CircularDependencyBreakingExceptionListener;
+
+final class CircularDependencyBreakingExceptionListenerPassTest extends AbstractCompilerPassTestCase
+{
+
+    public function it_register_circular_dependency_breaking_error_listener_when_exception_listener_is_registered(): void
+    {
+        $this->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());
+    }
+}