Introduce a CorrectnessNodeVisitor to validate that templates are semantically correct

CHANGELOG

@@ -1,5 +1,7 @@
 # 3.15.0 (2024-XX-XX)
 
+ * Deprecate the possibility to use a `block` tag within a capture node (like `set`)
+ * Throw an exception when a `macro`, `extends`, or `use` tag is used outside the root of a template
  * Improve the way one can deprecate a Twig callable (use `deprecation_info` instead of the other callable options)
 
 # 3.14.0 (2024-09-09)

doc/deprecated.rst

@@ -188,6 +188,29 @@ Templates
   ``Template::loadTemplate()``); pass instances of ``Twig\TemplateWrapper``
   instead.
 
+* Having a "block" definition nested in another node that captures the output
+  (like "set") is deprecated in Twig 3.14 and will throw in Twig 4.0. Such use
+  cases should be avoided as the "block" tag is used to both define the block
+  AND display it in place. Here is how you can decouple both easily:
+
+  Before::
+
+    {% extends "layout.twig" %}
+
+    {% set str %}
+        {% block content %}Some content{% endblock %}
+    {% endset %}
+
+  After::
+
+    {% extends "layout.twig" %}
+
+    {% block content %}Some content{% endblock %}
+
+    {% set str %}
+        {{ block('content') }}
+    {% endset %}
+
 Filters
 -------
 

src/Extension/CoreExtension.php

@@ -64,6 +64,7 @@
 use Twig\Node\Expression\Unary\NotUnary;
 use Twig\Node\Expression\Unary\PosUnary;
 use Twig\Node\Node;
+use Twig\NodeVisitor\CorrectnessNodeVisitor;
 use Twig\NodeVisitor\MacroAutoImportNodeVisitor;
 use Twig\Parser;
 use Twig\Source;
@@ -285,7 +286,10 @@ public function getTests(): array
 
     public function getNodeVisitors(): array
     {
-        return [new MacroAutoImportNodeVisitor()];
+        return [
+            new CorrectnessNodeVisitor(),
+            new MacroAutoImportNodeVisitor(),
+        ];
     }
 
     public function getOperators(): array

src/Node/ConfigNode.php

@@ -0,0 +1,30 @@
+<?php
+
+/*
+ * This file is part of Twig.
+ *
+ * (c) Fabien Potencier
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Twig\Node;
+
+use Twig\Attribute\YieldReady;
+
+/**
+ * Represents a node that has global side effects but does not generate template code.
+ *
+ * Such nodes must be at the root level of the body of a template.
+ *
+ * @author Fabien Potencier <[email protected]>
+ */
+#[YieldReady]
+final class ConfigNode extends Node
+{
+    public function __construct(int $lineno)
+    {
+        parent::__construct([], [], $lineno);
+    }
+}

src/Node/TextNode.php

@@ -38,4 +38,20 @@ public function compile(Compiler $compiler): void
             ->raw(";\n")
         ;
     }
+
+    public function isBlank(): bool
+    {
+        if (ctype_space($this->getAttribute('data'))) {
+            return true;
+        }
+
+        if (str_contains((string) $this, \chr(0xEF).\chr(0xBB).\chr(0xBF))) {
+            $t = substr($this->getAttribute('data'), 3);
+            if ('' === $t || ctype_space($t)) {
+                return true;
+            }
+        }
+
+        return false;
+    }
 }

src/NodeVisitor/CorrectnessNodeVisitor.php

@@ -0,0 +1,131 @@
+<?php
+
+/*
+ * This file is part of Twig.
+ *
+ * (c) Fabien Potencier
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Twig\NodeVisitor;
+
+use Twig\Environment;
+use Twig\Error\SyntaxError;
+use Twig\Node\BlockReferenceNode;
+use Twig\Node\ConfigNode;
+use Twig\Node\ModuleNode;
+use Twig\Node\Node;
+use Twig\Node\NodeCaptureInterface;
+use Twig\Node\TextNode;
+
+/**
+ * @author Fabien Potencier <[email protected]>
+ *
+ * @internal
+ */
+final class CorrectnessNodeVisitor implements NodeVisitorInterface
+{
+    private ?\SplObjectStorage $rootNodes = null;
+    // in a tag node that does not support "block" nodes (all of them except "block")
+    private ?Node $currentTagNode = null;
+    private bool $hasParent = false;
+    private ?\SplObjectStorage $blockNodes = null;
+    private int $currentBlockNodeLevel = 0;
+
+    public function enterNode(Node $node, Environment $env): Node
+    {
+        if ($node instanceof ModuleNode) {
+            $this->rootNodes = new \SplObjectStorage();
+            $this->hasParent = $node->hasNode('parent');
+
+            // allows to identify when we enter/leave the block nodes
+            $this->blockNodes = new \SplObjectStorage();
+            foreach ($node->getNode('blocks') as $n) {
+                $this->blockNodes->attach($n);
+            }
+
+            $body = $node->getNode('body')->getNode('0');
+            // see Parser::subparse() which does not wrap the parsed Nodes if there is only one node
+            foreach (count($body) ? $body : new Node([$body]) as $k => $n) {
+                // check that this root node of a child template only contains empty output nodes
+                if ($this->hasParent && !$this->isEmptyOutputNode($n)) {
+                    throw new SyntaxError('A template that extends another one cannot include content outside Twig blocks. Did you forget to put the content inside a {% block %} tag?', $n->getTemplateLine(), $n->getSourceContext());
+                }
+                $this->rootNodes->attach($n);
+            }
+
+            return $node;
+        }
+
+        if ($this->blockNodes->contains($node)) {
+            ++$this->currentBlockNodeLevel;
+        }
+
+        if ($this->hasParent && $node->getNodeTag() && !$node instanceof BlockReferenceNode) {
+            $this->currentTagNode = $node;
+        }
+
+        if ($node instanceof ConfigNode && !$this->rootNodes->contains($node)) {
+            throw new SyntaxError(sprintf('The "%s" tag must always be at the root of the body of a template.', $node->getNodeTag()), $node->getTemplateLine(), $node->getSourceContext());
+        }
+
+        if ($this->currentTagNode && $node instanceof BlockReferenceNode) {
+            if ($this->currentTagNode instanceof NodeCaptureInterface || count($this->blockNodes) > 1) {
+                trigger_deprecation('twig/twig', '3.14', \sprintf('Having a "block" tag under a "%s" tag (line %d) is deprecated in %s at line %d.', $this->currentTagNode->getNodeTag(), $this->currentTagNode->getTemplateLine(), $node->getSourceContext()->getName(), $node->getTemplateLine()));
+            } else {
+                throw new SyntaxError(\sprintf('A "block" tag cannot be under a "%s" tag (line %d).', $this->currentTagNode->getNodeTag(), $this->currentTagNode->getTemplateLine()), $node->getTemplateLine(), $node->getSourceContext());
+            }
+        }
+
+        return $node;
+    }
+
+    public function leaveNode(Node $node, Environment $env): Node
+    {
+        if ($node instanceof ModuleNode) {
+            $this->rootNodes = null;
+            $this->hasParent = false;
+            $this->blockNodes = null;
+            $this->currentBlockNodeLevel = 0;
+        }
+        if ($this->hasParent && $node->getNodeTag() && !$node instanceof BlockReferenceNode) {
+            $this->currentTagNode = null;
+        }
+        if ($this->hasParent && $this->blockNodes->contains($node)) {
+            --$this->currentBlockNodeLevel;
+        }
+
+        return $node;
+    }
+
+    public function getPriority(): int
+    {
+        return -255;
+    }
+
+    /**
+     * Returns true if the node never outputs anything or if the output is empty.
+     */
+    private function isEmptyOutputNode(Node $node): bool
+    {
+        if ($node instanceof NodeCaptureInterface) {
+            // a "block" tag in such a node will serve as a block definition AND be displayed in place as well
+            return true;
+        }
+
+        // Can the text be considered "empty" (only whitespace)?
+        if ($node instanceof TextNode) {
+            return $node->isBlank();
+        }
+
+        foreach ($node as $n) {
+            if (!$this->isEmptyOutputNode($n)) {
+                return false;
+            }
+        }
+
+        return true;
+    }
+}

src/Parser.php

@@ -20,8 +20,6 @@
 use Twig\Node\MacroNode;
 use Twig\Node\ModuleNode;
 use Twig\Node\Node;
-use Twig\Node\NodeCaptureInterface;
-use Twig\Node\NodeOutputInterface;
 use Twig\Node\PrintNode;
 use Twig\Node\TextNode;
 use Twig\TokenParser\TokenParserInterface;
@@ -81,10 +79,6 @@ public function parse(TokenStream $stream, $test = null, bool $dropNeedle = fals
 
         try {
             $body = $this->subparse($test, $dropNeedle);
-
-            if (null !== $this->parent && null === $body = $this->filterBodyNodes($body)) {
-                $body = new Node();
-            }
         } catch (SyntaxError $e) {
             if (!$e->getSourceContext()) {
                 $e->setSourceContext($this->stream->getSourceContext());
@@ -97,6 +91,10 @@ public function parse(TokenStream $stream, $test = null, bool $dropNeedle = fals
             throw $e;
         }
 
+        if ($this->parent) {
+            $this->cleanupBodyForChildTemplates($body);
+        }
+
         $node = new ModuleNode(new BodyNode([$body]), $this->parent, new Node($this->blocks), new Node($this->macros), new Node($this->traits), $this->embeddedTemplates, $stream->getSourceContext());
 
         $traverser = new NodeTraverser($this->env, $this->visitors);
@@ -334,50 +332,16 @@ public function getCurrentToken(): Token
         return $this->stream->getCurrent();
     }
 
-    private function filterBodyNodes(Node $node, bool $nested = false): ?Node
+    private function cleanupBodyForChildTemplates(Node $body): void
     {
-        // check that the body does not contain non-empty output nodes
-        if (
-            ($node instanceof TextNode && !ctype_space($node->getAttribute('data')))
-            || (!$node instanceof TextNode && !$node instanceof BlockReferenceNode && $node instanceof NodeOutputInterface)
-        ) {
-            if (str_contains((string) $node, \chr(0xEF).\chr(0xBB).\chr(0xBF))) {
-                $t = substr($node->getAttribute('data'), 3);
-                if ('' === $t || ctype_space($t)) {
-                    // bypass empty nodes starting with a BOM
-                    return null;
-                }
-            }
-
-            throw new SyntaxError('A template that extends another one cannot include content outside Twig blocks. Did you forget to put the content inside a {% block %} tag?', $node->getTemplateLine(), $this->stream->getSourceContext());
-        }
-
-        // bypass nodes that "capture" the output
-        if ($node instanceof NodeCaptureInterface) {
-            // a "block" tag in such a node will serve as a block definition AND be displayed in place as well
-            return $node;
-        }
-
-        // "block" tags that are not captured (see above) are only used for defining
-        // the content of the block. In such a case, nesting it does not work as
-        // expected as the definition is not part of the default template code flow.
-        if ($nested && $node instanceof BlockReferenceNode) {
-            throw new SyntaxError('A block definition cannot be nested under non-capturing nodes.', $node->getTemplateLine(), $this->stream->getSourceContext());
-        }
-
-        if ($node instanceof NodeOutputInterface) {
-            return null;
-        }
-
-        // here, $nested means "being at the root level of a child template"
-        // we need to discard the wrapping "Node" for the "body" node
-        $nested = $nested || Node::class !== \get_class($node);
-        foreach ($node as $k => $n) {
-            if (null !== $n && null === $this->filterBodyNodes($n, $nested)) {
-                $node->removeNode($k);
+        foreach ($body as $k => $node) {
+            if ($node instanceof BlockReferenceNode) {
+                // as it has a parent, the block reference won't be used
+                $body->removeNode($k);
+            } elseif ($node instanceof TextNode && $node->isBlank()) {
+                // remove nodes considered as "empty"
+                $body->removeNode($k);
             }
         }
-
-        return $node;
     }
 }

src/Test/IntegrationTestCase.php

@@ -165,7 +165,7 @@ public function getLegacyTests()
         return $this->getTests('testLegacyIntegration', true);
     }
 
-    protected function doIntegrationTest($file, $message, $condition, $templates, $exception, $outputs, $deprecation = '')
+    protected function doIntegrationTest($file, $message, $condition, $templateSources, $exception, $outputs, $deprecation = '')
     {
         if (!$outputs) {
             $this->markTestSkipped('no tests to run');
@@ -185,10 +185,10 @@ protected function doIntegrationTest($file, $message, $condition, $templates, $e
                 'strict_variables' => true,
             ], $match[2] ? eval($match[2].';') : []);
             // make sure that template are always compiled even if they are the same (useful when testing with more than one data/expect sections)
-            foreach ($templates as $j => $template) {
-                $templates[$j] = $template.str_repeat(' ', $i);
+            foreach ($templateSources as $name => $template) {
+                $templateSources[$name] = $template.str_repeat(' ', $i);
             }
-            $loader = new ArrayLoader($templates);
+            $loader = new ArrayLoader($templateSources);
             $twig = new Environment($loader, $config);
             $twig->addGlobal('global', 'global');
             foreach ($this->getRuntimeLoaders() as $runtimeLoader) {
@@ -212,6 +212,7 @@ protected function doIntegrationTest($file, $message, $condition, $templates, $e
             }
 
             $deprecations = [];
+            $templates = [];
             try {
                 $prevHandler = set_error_handler(function ($type, $msg, $file, $line, $context = []) use (&$deprecations, &$prevHandler) {
                     if (\E_USER_DEPRECATED === $type) {
@@ -223,7 +224,9 @@ protected function doIntegrationTest($file, $message, $condition, $templates, $e
                     return $prevHandler ? $prevHandler($type, $msg, $file, $line, $context) : false;
                 });
 
-                $template = $twig->load('index.twig');
+                foreach (array_keys($templateSources) as $templateName) {
+                    $templates[$templateName] = $twig->load($templateName);
+                }
             } catch (\Exception $e) {
                 if (false !== $exception) {
                     $message = $e->getMessage();
@@ -239,7 +242,7 @@ protected function doIntegrationTest($file, $message, $condition, $templates, $e
                 restore_error_handler();
             }
 
-            $this->assertSame($deprecation, implode("\n", $deprecations));
+            $template = $templates['index.twig'];
 
             try {
                 $output = trim($template->render(eval($match[1].';')), "\n ");
@@ -266,12 +269,14 @@ protected function doIntegrationTest($file, $message, $condition, $templates, $e
             if ($expected !== $output) {
                 printf("Compiled templates that failed on case %d:\n", $i + 1);
 
-                foreach (array_keys($templates) as $name) {
+                foreach (array_keys($templateSources) as $name) {
                     echo "Template: $name\n";
                     echo $twig->compile($twig->parse($twig->tokenize($twig->getLoader()->getSourceContext($name))));
                 }
             }
             $this->assertEquals($expected, $output, $message.' (in '.$file.')');
+
+            $this->assertSame($deprecation, implode("\n", $deprecations));
         }
     }