removed the "if" condition support on the "for" tag

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

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
+    implement the ``Countable`` interface.
-    looping with a condition.
 
 Adding a condition
 ------------------
@@ -91,22 +90,28 @@ items. The following example skips all the users which are not active:
 .. code-block:: twig
 
     <ul>
-        {% for user in users if user.active %}
+        {% for user in users|filter(user => user.active) %}
             <li>{{ user.username|e }}</li>
         {% endfor %}
     </ul>
 
 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
+counting the users not iterated over.
-``loop.last`` will not be defined when using loop conditions.
 
-.. 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
+.. code-block:: twig
-    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
+    <ul>
-    incremented when the condition is true (so the condition will never
+        {% set c = false %}
-    match).
+        {% for i, user in users %}
+            {% if i is even or c %}
+                <li>{{ user.username|e }}</li>
+                {% set c = i > 5 %}
+            {% endif %}
+        {% endfor %}
+    </ul>
 
 The `else` Clause
 -----------------

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")
-                ;
-            }
         }
     }
 }

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 ")

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) {
+        return new ForNode($keyTarget, $valueTarget, $seq, null, $body, $else, $lineno, $this->getTag());
-            $this->checkLoopUsageCondition($stream, $ifexpr);
-            $this->checkLoopUsageBody($stream, $body);
-        }
-
-        return new ForNode($keyTarget, $valueTarget, $seq, $ifexpr, $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';

test/Twig/Tests/Fixtures/tags/for/condition.legacy.test

@@ -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

test/Twig/Tests/Fixtures/tags/for/loop_not_defined.test

@@ -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.

test/Twig/Tests/Fixtures/tags/for/loop_not_defined_cond.test

@@ -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.

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->assertEquals($body, $node->getNode('body')->getNode(0));
-        $this->assertInstanceOf(IfNode::class, $node->getNode('body'));
-        $this->assertEquals($body, $node->getNode('body')->getNode('tests')->getNode(1)->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, <<<EOF
@@ -74,10 +68,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 = 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', true);
 
         $tests[] = [$node, <<<EOF
@@ -117,10 +110,9 @@ EOF
         $keyTarget = new AssignNameExpression('k', 1);
         $valueTarget = new AssignNameExpression('v', 1);
         $seq = new NameExpression('values', 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', true);
 
         $tests[] = [$node, <<<EOF
@@ -133,12 +125,22 @@ EOF
   'index'  => 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')};
-        echo {$this->getVariableGetter('foo')};
+    ++\$context['loop']['index0'];
-        ++\$context['loop']['index0'];
+    ++\$context['loop']['index'];
-        ++\$context['loop']['index'];
+    \$context['loop']['first'] = false;
-        \$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, <<<EOF