From dca3f74e49d8a2a3ffebeba826daeb8de014d286 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Thu, 9 May 2019 17:38:13 +0200 Subject: [PATCH] removed the "if" condition support on the "for" tag --- CHANGELOG | 1 + doc/tags/for.rst | 27 +++++---- src/Node/ForLoopNode.php | 19 +++--- src/Node/ForNode.php | 32 ++++------ src/TokenParser/ForTokenParser.php | 60 +------------------ .../Fixtures/tags/for/condition.legacy.test | 16 ----- .../Fixtures/tags/for/loop_not_defined.test | 10 ---- .../tags/for/loop_not_defined_cond.test | 9 --- test/Twig/Tests/Node/ForTest.php | 43 ++++++------- 9 files changed, 58 insertions(+), 159 deletions(-) delete mode 100644 test/Twig/Tests/Fixtures/tags/for/condition.legacy.test delete mode 100644 test/Twig/Tests/Fixtures/tags/for/loop_not_defined.test delete mode 100644 test/Twig/Tests/Fixtures/tags/for/loop_not_defined_cond.test diff --git a/CHANGELOG b/CHANGELOG index d38d3635b..f8d1b0d2c 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,5 +1,6 @@ * 3.0.0 (2019-XX-XX) + * removed the "if" condition support on the "for" tag * made the in, <, >, <=, >=, ==, and != operators more strict when comparing strings and integers/floats * removed the "filter" tag * added type hints everywhere diff --git a/doc/tags/for.rst b/doc/tags/for.rst index 77b1dc82d..a30cdee5d 100644 --- a/doc/tags/for.rst +++ b/doc/tags/for.rst @@ -78,8 +78,7 @@ Variable Description The ``loop.length``, ``loop.revindex``, ``loop.revindex0``, and ``loop.last`` variables are only available for PHP arrays, or objects that - implement the ``Countable`` interface. They are also not available when - looping with a condition. + implement the ``Countable`` interface. Adding a condition ------------------ @@ -91,22 +90,28 @@ items. The following example skips all the users which are not active: .. code-block:: twig The advantage is that the special loop variable will count correctly thus not -counting the users not iterated over. Keep in mind that properties like -``loop.last`` will not be defined when using loop conditions. +counting the users not iterated over. -.. note:: +If you need to skip items "dynamically", use an ``if`` tag within the ``for``'s +body: - Using the ``loop`` variable within the condition is not recommended as it - will probably not be doing what you expect it to. For instance, adding a - condition like ``loop.index > 4`` won't work as the index is only - incremented when the condition is true (so the condition will never - match). +.. code-block:: twig + + The `else` Clause ----------------- diff --git a/src/Node/ForLoopNode.php b/src/Node/ForLoopNode.php index 7aec737d5..d5ce845a7 100644 --- a/src/Node/ForLoopNode.php +++ b/src/Node/ForLoopNode.php @@ -36,19 +36,14 @@ class ForLoopNode extends Node ->write("++\$context['loop']['index0'];\n") ->write("++\$context['loop']['index'];\n") ->write("\$context['loop']['first'] = false;\n") + ->write("if (isset(\$context['loop']['length'])) {\n") + ->indent() + ->write("--\$context['loop']['revindex0'];\n") + ->write("--\$context['loop']['revindex'];\n") + ->write("\$context['loop']['last'] = 0 === \$context['loop']['revindex0'];\n") + ->outdent() + ->write("}\n") ; - - if (!$this->getAttribute('ifexpr')) { - $compiler - ->write("if (isset(\$context['loop']['length'])) {\n") - ->indent() - ->write("--\$context['loop']['revindex0'];\n") - ->write("--\$context['loop']['revindex'];\n") - ->write("\$context['loop']['last'] = 0 === \$context['loop']['revindex0'];\n") - ->outdent() - ->write("}\n") - ; - } } } } diff --git a/src/Node/ForNode.php b/src/Node/ForNode.php index ac65adc66..fd7ea689e 100644 --- a/src/Node/ForNode.php +++ b/src/Node/ForNode.php @@ -25,20 +25,16 @@ class ForNode extends Node { private $loop; - public function __construct(AssignNameExpression $keyTarget, AssignNameExpression $valueTarget, AbstractExpression $seq, AbstractExpression $ifexpr = null, Node $body, Node $else = null, int $lineno, string $tag = null) + public function __construct(AssignNameExpression $keyTarget, AssignNameExpression $valueTarget, AbstractExpression $seq, $ifexpr = null, Node $body, Node $else = null, int $lineno, string $tag = null) { $body = new Node([$body, $this->loop = new ForLoopNode($lineno, $tag)]); - if (null !== $ifexpr) { - $body = new IfNode(new Node([$ifexpr, $body]), null, $lineno, $tag); - } - $nodes = ['key_target' => $keyTarget, 'value_target' => $valueTarget, 'seq' => $seq, 'body' => $body]; if (null !== $else) { $nodes['else'] = $else; } - parent::__construct($nodes, ['with_loop' => true, 'ifexpr' => null !== $ifexpr], $lineno, $tag); + parent::__construct($nodes, ['with_loop' => true], $lineno, $tag); } public function compile(Compiler $compiler): void @@ -63,26 +59,20 @@ class ForNode extends Node ->write(" 'index' => 1,\n") ->write(" 'first' => true,\n") ->write("];\n") + ->write("if (is_array(\$context['_seq']) || (is_object(\$context['_seq']) && \$context['_seq'] instanceof \Countable)) {\n") + ->indent() + ->write("\$length = count(\$context['_seq']);\n") + ->write("\$context['loop']['revindex0'] = \$length - 1;\n") + ->write("\$context['loop']['revindex'] = \$length;\n") + ->write("\$context['loop']['length'] = \$length;\n") + ->write("\$context['loop']['last'] = 1 === \$length;\n") + ->outdent() + ->write("}\n") ; - - if (!$this->getAttribute('ifexpr')) { - $compiler - ->write("if (is_array(\$context['_seq']) || (is_object(\$context['_seq']) && \$context['_seq'] instanceof \Countable)) {\n") - ->indent() - ->write("\$length = count(\$context['_seq']);\n") - ->write("\$context['loop']['revindex0'] = \$length - 1;\n") - ->write("\$context['loop']['revindex'] = \$length;\n") - ->write("\$context['loop']['length'] = \$length;\n") - ->write("\$context['loop']['last'] = 1 === \$length;\n") - ->outdent() - ->write("}\n") - ; - } } $this->loop->setAttribute('else', $this->hasNode('else')); $this->loop->setAttribute('with_loop', $this->getAttribute('with_loop')); - $this->loop->setAttribute('ifexpr', $this->getAttribute('ifexpr')); $compiler ->write("foreach (\$context['_seq'] as ") diff --git a/src/TokenParser/ForTokenParser.php b/src/TokenParser/ForTokenParser.php index df49b2fa3..0a6b740f5 100644 --- a/src/TokenParser/ForTokenParser.php +++ b/src/TokenParser/ForTokenParser.php @@ -12,15 +12,10 @@ namespace Twig\TokenParser; -use Twig\Error\SyntaxError; use Twig\Node\Expression\AssignNameExpression; -use Twig\Node\Expression\ConstantExpression; -use Twig\Node\Expression\GetAttrExpression; -use Twig\Node\Expression\NameExpression; use Twig\Node\ForNode; use Twig\Node\Node; use Twig\Token; -use Twig\TokenStream; /** * Loops over each item of a sequence. @@ -41,13 +36,6 @@ final class ForTokenParser extends AbstractTokenParser $stream->expect(/* Token::OPERATOR_TYPE */ 8, 'in'); $seq = $this->parser->getExpressionParser()->parseExpression(); - $ifexpr = null; - if ($stream->nextIf(/* Token::NAME_TYPE */ 5, 'if')) { - @trigger_error(sprintf('Using an "if" condition on "for" tag is deprecated since Twig 2.10.0, use a "filter" filter or an "if" condition inside the "for" body instead (if your condition depends on a variable updated inside the loop).', __CLASS__), E_USER_DEPRECATED); - - $ifexpr = $this->parser->getExpressionParser()->parseExpression(); - } - $stream->expect(/* Token::BLOCK_END_TYPE */ 3); $body = $this->parser->subparse([$this, 'decideForFork']); if ('else' == $stream->next()->getValue()) { @@ -69,12 +57,7 @@ final class ForTokenParser extends AbstractTokenParser $valueTarget = new AssignNameExpression($valueTarget->getAttribute('name'), $valueTarget->getTemplateLine()); } - if ($ifexpr) { - $this->checkLoopUsageCondition($stream, $ifexpr); - $this->checkLoopUsageBody($stream, $body); - } - - return new ForNode($keyTarget, $valueTarget, $seq, $ifexpr, $body, $else, $lineno, $this->getTag()); + return new ForNode($keyTarget, $valueTarget, $seq, null, $body, $else, $lineno, $this->getTag()); } public function decideForFork(Token $token): bool @@ -87,47 +70,6 @@ final class ForTokenParser extends AbstractTokenParser return $token->test('endfor'); } - // the loop variable cannot be used in the condition - private function checkLoopUsageCondition(TokenStream $stream, Node $node): void - { - if ($node instanceof GetAttrExpression && $node->getNode('node') instanceof NameExpression && 'loop' == $node->getNode('node')->getAttribute('name')) { - throw new SyntaxError('The "loop" variable cannot be used in a looping condition.', $node->getTemplateLine(), $stream->getSourceContext()); - } - - foreach ($node as $n) { - if (!$n) { - continue; - } - - $this->checkLoopUsageCondition($stream, $n); - } - } - - // check usage of non-defined loop-items - // it does not catch all problems (for instance when a for is included into another or when the variable is used in an include) - private function checkLoopUsageBody(TokenStream $stream, Node $node): void - { - if ($node instanceof GetAttrExpression && $node->getNode('node') instanceof NameExpression && 'loop' == $node->getNode('node')->getAttribute('name')) { - $attribute = $node->getNode('attribute'); - if ($attribute instanceof ConstantExpression && \in_array($attribute->getAttribute('value'), ['length', 'revindex0', 'revindex', 'last'])) { - throw new SyntaxError(sprintf('The "loop.%s" variable is not defined when looping with a condition.', $attribute->getAttribute('value')), $node->getTemplateLine(), $stream->getSourceContext()); - } - } - - // should check for parent.loop.XXX usage - if ($node instanceof ForNode) { - return; - } - - foreach ($node as $n) { - if (!$n) { - continue; - } - - $this->checkLoopUsageBody($stream, $n); - } - } - public function getTag(): string { return 'for'; diff --git a/test/Twig/Tests/Fixtures/tags/for/condition.legacy.test b/test/Twig/Tests/Fixtures/tags/for/condition.legacy.test deleted file mode 100644 index 690207e42..000000000 --- a/test/Twig/Tests/Fixtures/tags/for/condition.legacy.test +++ /dev/null @@ -1,16 +0,0 @@ ---TEST-- -"for" tag takes a condition ---DEPRECATION-- -Using an "if" condition on "for" tag is deprecated since Twig 2.10.0, use a "filter" filter or an "if" condition inside the "for" body instead (if your condition depends on a variable updated inside the loop). ---TEMPLATE-- -{% for i in 1..5 if i is odd -%} - {{ loop.index }}.{{ i }}{{ foo.bar }} -{% endfor %} ---DATA-- -return ['foo' => ['bar' => 'X']] ---CONFIG-- -return ['strict_variables' => false] ---EXPECT-- -1.1X -2.3X -3.5X diff --git a/test/Twig/Tests/Fixtures/tags/for/loop_not_defined.test b/test/Twig/Tests/Fixtures/tags/for/loop_not_defined.test deleted file mode 100644 index 2d8c0b873..000000000 --- a/test/Twig/Tests/Fixtures/tags/for/loop_not_defined.test +++ /dev/null @@ -1,10 +0,0 @@ ---TEST-- -"for" tag ---TEMPLATE-- -{% for i, item in items if i > 0 %} - {{ loop.last }} -{% endfor %} ---DATA-- -return ['items' => ['a', 'b']] ---EXCEPTION-- -Twig\Error\SyntaxError: The "loop.last" variable is not defined when looping with a condition in "index.twig" at line 3. diff --git a/test/Twig/Tests/Fixtures/tags/for/loop_not_defined_cond.test b/test/Twig/Tests/Fixtures/tags/for/loop_not_defined_cond.test deleted file mode 100644 index e90e96b4a..000000000 --- a/test/Twig/Tests/Fixtures/tags/for/loop_not_defined_cond.test +++ /dev/null @@ -1,9 +0,0 @@ ---TEST-- -"for" tag ---TEMPLATE-- -{% for i, item in items if loop.last > 0 %} -{% endfor %} ---DATA-- -return ['items' => ['a', 'b']] ---EXCEPTION-- -Twig\Error\SyntaxError: The "loop" variable cannot be used in a looping condition in "index.twig" at line 2. diff --git a/test/Twig/Tests/Node/ForTest.php b/test/Twig/Tests/Node/ForTest.php index 47aefac4f..c868a04ef 100644 --- a/test/Twig/Tests/Node/ForTest.php +++ b/test/Twig/Tests/Node/ForTest.php @@ -10,10 +10,8 @@ */ use Twig\Node\Expression\AssignNameExpression; -use Twig\Node\Expression\ConstantExpression; use Twig\Node\Expression\NameExpression; use Twig\Node\ForNode; -use Twig\Node\IfNode; use Twig\Node\Node; use Twig\Node\PrintNode; use Twig\Test\NodeTestCase; @@ -25,22 +23,19 @@ class Twig_Tests_Node_ForTest extends NodeTestCase $keyTarget = new AssignNameExpression('key', 1); $valueTarget = new AssignNameExpression('item', 1); $seq = new NameExpression('items', 1); - $ifexpr = new ConstantExpression(true, 1); $body = new Node([new PrintNode(new NameExpression('foo', 1), 1)], [], 1); $else = null; - $node = new ForNode($keyTarget, $valueTarget, $seq, $ifexpr, $body, $else, 1); + $node = new ForNode($keyTarget, $valueTarget, $seq, null, $body, $else, 1); $node->setAttribute('with_loop', false); $this->assertEquals($keyTarget, $node->getNode('key_target')); $this->assertEquals($valueTarget, $node->getNode('value_target')); $this->assertEquals($seq, $node->getNode('seq')); - $this->assertTrue($node->getAttribute('ifexpr')); - $this->assertInstanceOf(IfNode::class, $node->getNode('body')); - $this->assertEquals($body, $node->getNode('body')->getNode('tests')->getNode(1)->getNode(0)); + $this->assertEquals($body, $node->getNode('body')->getNode(0)); $this->assertFalse($node->hasNode('else')); $else = new PrintNode(new NameExpression('foo', 1), 1); - $node = new ForNode($keyTarget, $valueTarget, $seq, $ifexpr, $body, $else, 1); + $node = new ForNode($keyTarget, $valueTarget, $seq, null, $body, $else, 1); $node->setAttribute('with_loop', false); $this->assertEquals($else, $node->getNode('else')); } @@ -52,10 +47,9 @@ class Twig_Tests_Node_ForTest extends NodeTestCase $keyTarget = new AssignNameExpression('key', 1); $valueTarget = new AssignNameExpression('item', 1); $seq = new NameExpression('items', 1); - $ifexpr = null; $body = new Node([new PrintNode(new NameExpression('foo', 1), 1)], [], 1); $else = null; - $node = new ForNode($keyTarget, $valueTarget, $seq, $ifexpr, $body, $else, 1); + $node = new ForNode($keyTarget, $valueTarget, $seq, null, $body, $else, 1); $node->setAttribute('with_loop', false); $tests[] = [$node, <<setAttribute('with_loop', true); $tests[] = [$node, <<setAttribute('with_loop', true); $tests[] = [$node, << 1, 'first' => true, ]; +if (is_array(\$context['_seq']) || (is_object(\$context['_seq']) && \$context['_seq'] instanceof \Countable)) { + \$length = count(\$context['_seq']); + \$context['loop']['revindex0'] = \$length - 1; + \$context['loop']['revindex'] = \$length; + \$context['loop']['length'] = \$length; + \$context['loop']['last'] = 1 === \$length; +} foreach (\$context['_seq'] as \$context["k"] => \$context["v"]) { - if (true) { - echo {$this->getVariableGetter('foo')}; - ++\$context['loop']['index0']; - ++\$context['loop']['index']; - \$context['loop']['first'] = false; + echo {$this->getVariableGetter('foo')}; + ++\$context['loop']['index0']; + ++\$context['loop']['index']; + \$context['loop']['first'] = false; + if (isset(\$context['loop']['length'])) { + --\$context['loop']['revindex0']; + --\$context['loop']['revindex']; + \$context['loop']['last'] = 0 === \$context['loop']['revindex0']; } } \$_parent = \$context['_parent']; @@ -150,10 +152,9 @@ EOF $keyTarget = new AssignNameExpression('k', 1); $valueTarget = new AssignNameExpression('v', 1); $seq = new NameExpression('values', 1); - $ifexpr = null; $body = new Node([new PrintNode(new NameExpression('foo', 1), 1)], [], 1); $else = new PrintNode(new NameExpression('foo', 1), 1); - $node = new ForNode($keyTarget, $valueTarget, $seq, $ifexpr, $body, $else, 1); + $node = new ForNode($keyTarget, $valueTarget, $seq, null, $body, $else, 1); $node->setAttribute('with_loop', true); $tests[] = [$node, <<