From ccca32a3f2dabab99605536c889bb02256aea8d0 Mon Sep 17 00:00:00 2001
From: catch <6915-catch@users.noreply.drupalcode.org>
Date: Thu, 21 Nov 2024 12:19:12 +0000
Subject: [PATCH] Issue #3487031 by larowlan, alexpott, themodularlab,
ericgsmith, longwave, spokje: Performance Degraded after update to twig
3.14.2
(cherry picked from commit 457b1dfb538dc1fe7a9ef4b1d26848904878d855)
---
.../RemoveCheckToStringNodeVisitor.php | 56 +++++++++++++++++++
.../Drupal/Core/Template/TwigExtension.php | 10 +++-
.../Core/Template/TwigSandboxPolicy.php | 28 +++++++---
.../Template/TwigSimpleCheckToStringNode.php | 33 +++++++++++
4 files changed, 117 insertions(+), 10 deletions(-)
create mode 100644 core/lib/Drupal/Core/Template/RemoveCheckToStringNodeVisitor.php
create mode 100644 core/lib/Drupal/Core/Template/TwigSimpleCheckToStringNode.php
diff --git a/core/lib/Drupal/Core/Template/RemoveCheckToStringNodeVisitor.php b/core/lib/Drupal/Core/Template/RemoveCheckToStringNodeVisitor.php
new file mode 100644
index 000000000000..568b22ace9d4
--- /dev/null
+++ b/core/lib/Drupal/Core/Template/RemoveCheckToStringNodeVisitor.php
@@ -0,0 +1,56 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Drupal\Core\Template;
+
+use Twig\Environment;
+use Twig\Node\CheckToStringNode;
+use Twig\Node\Node;
+use Twig\NodeVisitor\NodeVisitorInterface;
+
+/**
+ * Defines a TwigNodeVisitor that replaces CheckToStringNodes.
+ *
+ * Twig 3.14.1 resulted in a performance regression in Drupal due to checking if
+ * __toString is an allowed method on objects. __toString is allowed on all
+ * objects when Drupal's default SandboxPolicy is active. Therefore, Twig's
+ * SandboxExtension checks are unnecessary.
+ */
+final class RemoveCheckToStringNodeVisitor implements NodeVisitorInterface {
+
+ /**
+ * {@inheritdoc}
+ */
+ public function enterNode(Node $node, Environment $env): Node {
+ if ($node instanceof CheckToStringNode) {
+ // Replace CheckToStringNode with the faster equivalent, __toString is an
+ // allowed method so any checking of __toString on a per-object basis is
+ // performance overhead.
+ $new = new TwigSimpleCheckToStringNode($node->getNode('expr'));
+ // @todo https://www.drupal.org/project/drupal/issues/3488584 Update for
+ // Twig 4 as the spread attribute has been removed there.
+ if ($node->hasAttribute('spread')) {
+ $new->setAttribute('spread', $node->getAttribute('spread'));
+ }
+ return $new;
+ }
+ return $node;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function leaveNode(Node $node, Environment $env): ?Node {
+ return $node;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getPriority() {
+ // Runs after sandbox visitor.
+ return 1;
+ }
+
+}
diff --git a/core/lib/Drupal/Core/Template/TwigExtension.php b/core/lib/Drupal/Core/Template/TwigExtension.php
index 9200d0254177..6ebbfecbfb9d 100644
--- a/core/lib/Drupal/Core/Template/TwigExtension.php
+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -157,10 +157,18 @@ public function getFilters() {
public function getNodeVisitors() {
// The node visitor is needed to wrap all variables with
// render_var -> TwigExtension->renderVar() function.
- return [
+ $visitors = [
new TwigNodeVisitor(),
new TwigNodeVisitorCheckDeprecations(),
];
+ if (\in_array('__toString', TwigSandboxPolicy::getMethodsAllowedOnAllObjects(), TRUE)) {
+ // When __toString is an allowed method, there is no point in running
+ // \Twig\Extension\SandboxExtension::ensureToStringAllowed, so we add a
+ // node visitor to remove any CheckToStringNode nodes added by the
+ // sandbox extension.
+ $visitors[] = new RemoveCheckToStringNodeVisitor();
+ }
+ return $visitors;
}
/**
diff --git a/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
index 42d1811877e8..7774f715e4f6 100644
--- a/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
+++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
@@ -54,15 +54,7 @@ public function __construct() {
// Flip the array so we can check using isset().
$this->allowed_classes = array_flip($allowed_classes);
- $allowed_methods = Settings::get('twig_sandbox_allowed_methods', [
- // Only allow idempotent methods.
- 'id',
- 'label',
- 'bundle',
- 'get',
- '__toString',
- 'toString',
- ]);
+ $allowed_methods = static::getMethodsAllowedOnAllObjects();
// Flip the array so we can check using isset().
$this->allowed_methods = array_flip($allowed_methods);
@@ -109,4 +101,22 @@ public function checkMethodAllowed($obj, $method): void {
throw new SecurityError(sprintf('Calling "%s" method on a "%s" object is not allowed.', $method, get_class($obj)));
}
+ /**
+ * Gets the list of allowed methods on all objects.
+ *
+ * @return string[]
+ * The list of allowed methods on all objects.
+ */
+ public static function getMethodsAllowedOnAllObjects(): array {
+ return Settings::get('twig_sandbox_allowed_methods', [
+ // Only allow idempotent methods.
+ 'id',
+ 'label',
+ 'bundle',
+ 'get',
+ '__toString',
+ 'toString',
+ ]);
+ }
+
}
diff --git a/core/lib/Drupal/Core/Template/TwigSimpleCheckToStringNode.php b/core/lib/Drupal/Core/Template/TwigSimpleCheckToStringNode.php
new file mode 100644
index 000000000000..42f32a5d4694
--- /dev/null
+++ b/core/lib/Drupal/Core/Template/TwigSimpleCheckToStringNode.php
@@ -0,0 +1,33 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Drupal\Core\Template;
+
+use Twig\Compiler;
+use Twig\Node\CheckToStringNode;
+
+/**
+ * Defines a twig node for simplifying CheckToStringNode.
+ *
+ * Drupal's sandbox policy is very permissive with checking whether an object
+ * can be converted to a string. We allow any object with a __toString method.
+ * This means that the array traversal in the default SandboxExtension
+ * implementation added by the parent class is a performance overhead we don't
+ * need.
+ *
+ * @see \Drupal\Core\Template\TwigSandboxPolicy
+ * @see \Drupal\Core\Template\RemoveCheckToStringNodeVisitor
+ */
+final class TwigSimpleCheckToStringNode extends CheckToStringNode {
+
+ /**
+ * {@inheritdoc}
+ */
+ public function compile(Compiler $compiler): void {
+ $expr = $this->getNode('expr');
+ $compiler
+ ->subcompile($expr);
+ }
+
+}
--
GitLab