Introduce a CorrectnessNodeVisitor to validate that templates are semantically correct #4292
Conversation
…antically correct
|
|
||
| $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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
maybe we need to check for Node::class === \get_class($node) instead, as done in the old filterBodyNodes
There was a problem hiding this comment.
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)
| 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) { |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| if ($this->currentTagNode && $node instanceof BlockReferenceNode) { | ||
| if ($this->currentTagNode instanceof NodeCaptureInterface || count($this->blockNodes) > 1) { |
There was a problem hiding this comment.
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 ?
| 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 |
There was a problem hiding this comment.
Is the filtering of the BOM gone ?
| foreach ($node as $k => $n) { | ||
| if (null !== $n && null === $this->filterBodyNodes($n, $nested)) { | ||
| $node->removeNode($k); | ||
| foreach ($body as $k => $node) { |
There was a problem hiding this comment.
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.
| --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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| $this->currentBlockNodeLevel = 0; | ||
| } | ||
| if ($this->hasParent && $node->getNodeTag() && !$node instanceof BlockReferenceNode) { | ||
| $this->currentTagNode = null; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
or even just some other nested node inside the if followed by a block after the if
| @@ -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 | |||
There was a problem hiding this comment.
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.
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 ?
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 anextendtag is not embedded into a block or a macro. Theextendtag is not the only one that must be at the root of a templateThis PR introduces a new
CorrectnessNodeVisitorthat 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
usetag embedded in another tag (likeifin the mentioned PR) is deprecated and will not be possible in 4.0.