Introduce a CorrectnessNodeVisitor to validate that templates are semantically correct #4292

fabpot · 2024-09-09T20:12:15Z

This PR addresses several issues around the correctness of templates.
Being able to parse and compile a template does not mean that it is semantically correct. To enforce correctness, we currently have several places where we deal with it:

Parser::filterBodyNodes(): This method is a mix of ensuring the correctness of a template, but it also changes the body node of a child template (something that is always needed and not part of the correctness checks)
ExtendsTokenParser: It checks that an extend tag is not embedded into a block or a macro. The extend tag is not the only one that must be at the root of a template

This PR introduces a new CorrectnessNodeVisitor that has the responsibility to check that a template is semantically correct. It's the continuation of work that started a long time ago in #2687 (where I mentioned the weirdness of some supported templates like those mentioned in #3926 and deprecated by this PR).

Closes #3698: Having a use tag embedded in another tag (like if in the mentioned PR) is deprecated and will not be possible in 4.0.

…antically correct

stof · 2024-09-10T09:36:54Z

src/NodeVisitor/CorrectnessNodeVisitor.php

+
+            $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) {


what if there is a single parsed node with children ? count($body) would be truthy so it would iterate over the children of that node instead of checking the body

maybe we need to check for Node::class === \get_class($node) instead, as done in the old filterBodyNodes

maybe we should introduce a StatementListNode (name to be bikeshed) to represent such list of other nodes to allow identifying this case more easily and deprecate instantiating the Node class itself (making it abstract in Twig 4)

stof · 2024-09-10T09:44:12Z

src/NodeVisitor/CorrectnessNodeVisitor.php

+            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) {


shouldn't this check $this->currentBlockNodeLevel ? Once we are inside a block, we can have block definitions anywhere as those are not top-level blocks anymore.

stof · 2024-09-10T09:45:31Z

src/NodeVisitor/CorrectnessNodeVisitor.php

+        }
+
+        if ($this->currentTagNode && $node instanceof BlockReferenceNode) {
+            if ($this->currentTagNode instanceof NodeCaptureInterface || count($this->blockNodes) > 1) {


what is the goal of this count($this->blockNodes) condition ? why would we have a behavior that depends on the number of blocks defined in the template ?

stof · 2024-09-10T09:50:48Z

src/Parser.php

-            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


Is the filtering of the BOM gone ?

stof · 2024-09-10T09:53:37Z

src/Parser.php

-        foreach ($node as $k => $n) {
-            if (null !== $n && null === $this->filterBodyNodes($n, $nested)) {
-                $node->removeNode($k);
+        foreach ($body as $k => $node) {


What happens when $body is not a Node but a child class because subparse did not wrap it ? AFAICT, this logic would fail to remove the node in such case.

stof · 2024-09-10T10:04:00Z

tests/Fixtures/tags/inheritance/conditional_block_nested.legacy.test

+--TEST--
+conditional "block" tag with "extends" tag (nested)
+--DEPRECATION--
+Since twig/twig 3.14: Having a "block" tag under a "if" tag (line 7) is deprecated in layout.twig at line 8.


This is a tag inside a non-top-level condition, so I don't think we need to deprecate this. Such tag is useful when you want to reuse part of the rendering between the if and else clause by defining the block in the if and referencing it in the else.
Having to move it to a top-level block (that does not override a parent block) makes the intent a lot less clear as it is easy to miss that this block is related to the rendering inside the condition (and so that it can be removed if the condition body does not need it anymore).

For the same reason, it can make sense to define a block inside the body of a loop, which would probably also trigger such a deprecation.

To me, once entering a top-level block of a child template, we should not have any restriction on blocks anymore, just like there is no such restriction in a standalone template.

stof · 2024-09-10T15:23:46Z

src/NodeVisitor/CorrectnessNodeVisitor.php

+            $this->currentBlockNodeLevel = 0;
+        }
+        if ($this->hasParent && $node->getNodeTag() && !$node instanceof BlockReferenceNode) {
+            $this->currentTagNode = null;


This looks wrong to me, as it will reset the current tag node to null rather than resetting it to its state before entering that node (try putting a block inside a nested if in a template followed by a block after the inner if but inside the outer one, and your current code would not report the second block as being incorrect)

or even just some other nested node inside the if followed by a block after the if

smnandre · 2024-09-14T21:16:46Z

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


Not answering on the "why", i have'nt tried this PR locally yet... but i feel the before/after examples do not represent the same usage/need.

For instance (please don't be too harsh on this code 🙏 ) this is a pattern i used from time to time, and i feel this is often how the set/block was used.

https://github.com/symfony/ux/blob/8a66e74d5e7070e5cbf60042cae535fae28d3e7a/ux.symfony.com/templates/components/Code/CodeLine.html.twig

I don't mind we cannot in 4.0 and will find a better way, but the "after" here would not help in this situation, right ?

…ode and Nodes (fabpot) This PR was merged into the 3.x branch. Discussion ---------- Deprecate instantiating Node directly, introduce EmptyNode and Nodes Based on some comments from `@stof`: See #4292 (comment) See #4333 (comment) First interesting usage here: 65ee72a Commits ------- 8b27898 Deprecate using Node directly, introduce EmptyNode and Nodes

stof · 2024-10-14T12:00:11Z

@fabpot what is the state of this PR ? Do you plan to fix it or to abandon it ?

fabpot · 2024-10-14T12:14:13Z

@fabpot what is the state of this PR ? Do you plan to fix it or to abandon it ?

I plan to still work on this (soon).

Introduce a CorrectnessNodeVisitor to validate that templates are sem…

695a853

…antically correct

stof reviewed Sep 10, 2024

View reviewed changes

smnandre reviewed Sep 14, 2024

View reviewed changes

stof mentioned this pull request Sep 24, 2024

Optimize compiled code for "set" tag #4333

Merged

fabpot mentioned this pull request Sep 24, 2024

Deprecate instantiating Node directly, introduce EmptyNode and Nodes #4335

Merged

fabpot force-pushed the 3.x branch from 6df7e6d to 1876635 Compare October 4, 2024 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a CorrectnessNodeVisitor to validate that templates are semantically correct #4292

Introduce a CorrectnessNodeVisitor to validate that templates are semantically correct #4292

fabpot commented Sep 9, 2024

stof Sep 10, 2024

stof Sep 10, 2024

stof Sep 10, 2024

stof Sep 10, 2024

stof Sep 10, 2024

stof Sep 10, 2024

stof Sep 10, 2024

stof Sep 10, 2024 •

edited

Loading

stof Sep 10, 2024

stof Sep 10, 2024

stof Sep 10, 2024

smnandre Sep 14, 2024

stof commented Oct 14, 2024

fabpot commented Oct 14, 2024

Introduce a CorrectnessNodeVisitor to validate that templates are semantically correct #4292

Are you sure you want to change the base?

Introduce a CorrectnessNodeVisitor to validate that templates are semantically correct #4292

Conversation

fabpot commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Oct 14, 2024

fabpot commented Oct 14, 2024

stof Sep 10, 2024 •

edited

Loading