twigphp / Twig

Twig, the flexible, fast, and secure template language for PHP
https://twig.symfony.com/
BSD 3-Clause "New" or "Revised" License
8.17k stars 1.25k forks source link

Twig doesn't support alternative PHP servers like RoadRunner #4007

Closed speller closed 2 months ago

speller commented 8 months ago

Alternative PHP servers like RoadRunner don't unload PHP processes and process multiple requests per process allowing to get a significant performance increase. Unfortunately, Twig is not fully compatible with such servers because it relies on stateful classes that store some data. The intention is good - to cache data during the request. However, if the PHP process is not unloaded between HTTP requests, the internal state becomes an issue. On the second and further requests, Twig gets data from the first one which easily can be broken and make the request fail.

From my investigation, I understood that the issue is mostly in the global variables. They're cached in the \Twig\Environment::$resolvedGlobals property. Some extensions like EasyAdmin may add global variables conditionally. For example, if the first request doesn't meet EasyAdmin's criteria, the ea global variable is not loaded and not added to $resolvedGlobals. On the next request, Twig sees the non-null $resolvedGlobals and doesn't try to reload global variables. So any subsequent request that tries to render EasyAdmin's stuff will fail because the ea variable is not loaded.

Currently, to fix this issue, I'm using the following workaround in my project:

Define a custom Twig class:

class Twig extends Environment implements EventSubscriberInterface
{
    public function onKernelTerminate(TerminateEvent $event): void
    {
        // Clear the resolvedGlobals cache to reinitialize global variables on the next RR request
        $this->resolvedGlobals = null;
    }

    #[\Override]
    public static function getSubscribedEvents(): array
    {
        return [
            KernelEvents::TERMINATE => 'onKernelTerminate',
        ];
    }
}

Add a compiler pass to replace the Twig object with the custom one:

class ReplaceTwigCompilerPass implements CompilerPassInterface
{

    #[\Override]
    public function process(ContainerBuilder $container): void
    {
        $twigDefinition = $container->getDefinition('twig');
        $twigDefinition->setClass(Twig::class);
        $twigDefinition->addTag('kernel.event_subscriber');
    }
}

The custom class subscribes to Symfony's kernel.terminate event and cleans up globals cache. So on the next request, Twig properly reinitializes it with variables for the current request.

And the last point - to allow the Environment's $resolvedGlobals field modification, I apply the following patch with cweagans/composer-patches:

--- a/src/Environment.php
+++ b/src/Environment.php
@@ -57,7 +57,7 @@
     private $compiler;
     /** @var array<string, mixed> */
     private $globals = [];
-    private $resolvedGlobals;
+    protected $resolvedGlobals;
     private $loadedTemplates;
     private $strictVariables;
     private $templateClassPrefix = '__TwigTemplate_';

For my project, this works well. But I would be more than happy to see native support to not apply custom workarounds. The fix doesn't look complex so I believe it can be implemented quickly with no breaking changes.

GromNaN commented 8 months ago

Symfony has the ResetInterface::reset() that can be called between requests. This is something that could be implemented in Twig. This means that we iterate on every extension to get the globals that may be defined by them. But I believe all Twig configuration should be static, including the config provided by extensions.

The underlying issue is having a global variable defined conditionally in EasyAdmin. Instead it could be injected as parameter when templates are rendered (in one place in AbstractCrudController::render()); or the ea global variable always defined with a proxy object.

speller commented 7 months ago

Proxy object for the ea variable is definitely an option. But I think it will be a workaround. The root issue here in Twig itself, it should be able to handle multiple requests without script unload by design.

GromNaN commented 7 months ago

Twig is perfectly able to handle multiple request: it can render several templates with the same Twig\Environment instance; the environment is idempotent. It's because there's a condition in the EasyAdmin extension that there's a problem. It's like initializing 2 different Twig environments depending on whether it is in an EasyAdmin context or not.

speller commented 7 months ago

@GromNaN Twig allows EasyAdmin to do this. So this is a Twig issue as well. The current case with EasyAdmin can be resolved by introducing a workaround in it. But any other Twig extension can fall into the same issue. This is why I suggest solving the root cause instead of relying on specific extensions to follow some unwritten agreement of not doing something.

speller commented 7 months ago

@GromNaN While I agree that Symfony has some tools to react to new requests in the same app, Twig is abstract from frameworks and, from my point of view, should support resetting its environment between requests. So then the Symfony bundle will be able to call this public Twig API with no hacks, workarounds, or extensions rewriting and solve the root cause for all cases, not for EasyAdmin specifically.

GromNaN commented 7 months ago

Resetting the environment is almost the same as reinstantiating the Twig\Environment class. The definition of what should be reset or not vague. That would lead to more unexpected behaviors and performance issues.

Note that even if you reset the Twig environment, if you process 2 requests in parallel (co-routines with Fibers), you can end with an incorrect state for both of them.

speller commented 7 months ago

@GromNaN Why do we need to reinstantiate the whole instance if we need to cleanup only global variables? Is it already allowed in the Symfony integration?

Fibers is a completely different topic and I would not mix them here.

AlexBachmann commented 7 months ago

I experience similar issues, when running shopware/shopware with FrankenPHP. The globals are not reset between requests, which can cause significant rendering problems in Shopware, since the globals are, among others used for URL creation, and marking of page types.

nicolas-grekas commented 6 months ago

The issue is using globals for request-local state. See https://github.com/EasyCorp/EasyAdminBundle/pull/6273 for a discussion about this, and a possible solution, which might not belong to Twig in the end.

nicolas-grekas commented 6 months ago

Maybe twig should deprecate GlobalsInterface?

fabpot commented 2 months ago

Should be fixed by #4286 which introduces a new Environment::resetGlobals() method.

fabpot commented 2 months ago

And for Symfony users, see https://github.com/symfony/symfony/pull/58198/