Repository: PebbleTemplates/pebble
Pull request #262 (2017-04-17 17:44:29)

fix for Variable scope bug (#250)
Fix for bug #250 


Patch:
diff --git a/src/main/java/com/mitchellbosecke/pebble/node/ForNode.java b/src/main/java/com/mitchellbosecke/pebble/node/ForNode.java
index f1a6459b..59bd547a 100644
--- a/src/main/java/com/mitchellbosecke/pebble/node/ForNode.java
+++ b/src/main/java/com/mitchellbosecke/pebble/node/ForNode.java
@@ -18,7 +18,11 @@ import com.mitchellbosecke.pebble.template.ScopeChain;
 import java.io.IOException;
 import java.io.Writer;
 import java.lang.reflect.Array;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.Map;
 
 /**
  * Represents a "for" loop within the template.
@@ -47,41 +51,28 @@ public class ForNode extends AbstractRenderableNode {
     @Override
     public void render(PebbleTemplateImpl self, Writer writer, EvaluationContext context)
             throws PebbleException, IOException {
-        Object iterableEvaluation = iterableExpression.evaluate(self, context);
+        Object iterableEvaluation = this.iterableExpression.evaluate(self, context);
         Iterable<?> iterable = null;
 
         if (iterableEvaluation == null) {
             return;
         }
 
-        iterable = toIterable(iterableEvaluation);
+        iterable = this.toIterable(iterableEvaluation);
 
         if (iterable == null) {
             throw new PebbleException(null, "Not an iterable object. Value = [" + iterableEvaluation.toString() + "]",
-                getLineNumber(), self.getName());
+                    this.getLineNumber(), self.getName());
         }
 
         Iterator<?> iterator = iterable.iterator();
 
-        boolean newScope = false;
-
         if (iterator.hasNext()) {
 
             ScopeChain scopeChain = context.getScopeChain();
+            scopeChain.pushScope();
 
-            /*
-             * Only if there is a variable name conflict between one of the
-             * variables added by the for loop construct and an existing
-             * variable do we push another scope, otherwise we reuse the current
-             * scope for performance purposes.
-             */
-            if (scopeChain.currentScopeContainsVariable("loop") || scopeChain
-                    .currentScopeContainsVariable(variableName)) {
-                scopeChain.pushScope();
-                newScope = true;
-            }
-
-            int length = getIteratorSize(iterableEvaluation);
+            int length = this.getIteratorSize(iterableEvaluation);
             int index = 0;
 
             Map<String, Object> loop = null;
@@ -119,16 +110,14 @@ public class ForNode extends AbstractRenderableNode {
 
                 scopeChain.put("loop", loop);
 
-                scopeChain.put(variableName, iterator.next());
-                body.render(self, writer, context);
+                scopeChain.put(this.variableName, iterator.next());
+                this.body.render(self, writer, context);
             }
 
-            if (newScope) {
-                scopeChain.popScope();
-            }
+            scopeChain.popScope();
 
-        } else if (elseBody != null) {
-            elseBody.render(self, writer, context);
+        } else if (this.elseBody != null) {
+            this.elseBody.render(self, writer, context);
         }
 
     }
@@ -139,19 +128,19 @@ public class ForNode extends AbstractRenderableNode {
     }
 
     public String getIterationVariable() {
-        return variableName;
+        return this.variableName;
     }
 
     public Expression<?> getIterable() {
-        return iterableExpression;
+        return this.iterableExpression;
     }
 
     public BodyNode getBody() {
-        return body;
+        return this.body;
     }
 
     public BodyNode getElseBody() {
-        return elseBody;
+        return this.elseBody;
     }
 
     @SuppressWarnings({ "unchecked", "rawtypes" })
@@ -186,12 +175,12 @@ public class ForNode extends AbstractRenderableNode {
 
                         @Override
                         public boolean hasNext() {
-                            return index < length;
+                            return this.index < this.length;
                         }
 
                         @Override
                         public Object next() {
-                            return Array.get(obj, index++);
+                            return Array.get(obj, this.index++);
                         }
 
                         @Override


Test patch:
diff --git a/src/test/java/com/mitchellbosecke/pebble/node/ForNodeTest.java b/src/test/java/com/mitchellbosecke/pebble/node/ForNodeTest.java
new file mode 100644
index 00000000..643543bf
--- /dev/null
+++ b/src/test/java/com/mitchellbosecke/pebble/node/ForNodeTest.java
@@ -0,0 +1,83 @@
+package com.mitchellbosecke.pebble.node;
+
+import com.mitchellbosecke.pebble.PebbleEngine;
+import com.mitchellbosecke.pebble.loader.StringLoader;
+import com.mitchellbosecke.pebble.template.PebbleTemplate;
+
+import org.junit.Test;
+
+import java.io.StringWriter;
+import java.io.Writer;
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.junit.Assert.assertEquals;
+
+public class ForNodeTest {
+  @Test
+  public void testVariableScope() throws Exception {
+    PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader()).strictVariables(false).build();
+
+    StringBuilder source = new StringBuilder("{% set fooList = range(1, 1) %}");
+    source.append("{% for item in fooList %}");
+    source.append("{% set foo1 = 'fooValue' %}");
+    source.append("Foo1 value : {{ foo1 }}");
+    source.append("{% endfor %}");
+    source.append("Foo1 value : {{ foo1 }}");
+
+    source.append("{% for item in fooList %}");
+    source.append("{% set foo2 = 'fooValue2' %}");
+    source.append("Foo2 value : {{ foo2 }}");
+    source.append("{% endfor %}");
+    source.append("Foo2 value : {{ foo2 }}");
+
+    PebbleTemplate template = pebble.getTemplate(source.toString());
+
+    Map<String, Object> context = new HashMap<>();
+
+    Writer writer = new StringWriter();
+    template.evaluate(writer, context);
+    assertEquals("Foo1 value : fooValueFoo1 value : Foo2 value : fooValue2Foo2 value : ", writer.toString());
+  }
+
+  @Test
+  public void testNestedLoop() throws Exception {
+    PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader()).strictVariables(false).build();
+
+    StringBuilder source = new StringBuilder("{% for i in 0..2 %}");
+    source.append("{% for j in 0..2 %}");
+    source.append("i={{ i }} j={{ j }} ");
+    source.append("{% endfor %}");
+    source.append("{% endfor %}");
+    source.append("i={{ i }} j={{ j }} ");
+
+    PebbleTemplate template = pebble.getTemplate(source.toString());
+
+    Map<String, Object> context = new HashMap<>();
+
+    Writer writer = new StringWriter();
+    template.evaluate(writer, context);
+    assertEquals("i=0 j=0 i=0 j=1 i=0 j=2 i=1 j=0 i=1 j=1 i=1 j=2 i=2 j=0 i=2 j=1 i=2 j=2 i= j= ", writer.toString());
+  }
+
+  @Test
+  public void testLoopIndex() throws Exception {
+    PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader()).strictVariables(false).build();
+
+    StringBuilder source = new StringBuilder("{% for i in 0..2 %}");
+    source.append("{% for j in 0..2 %}");
+    source.append("inner={{ loop.index }} ");
+    source.append("{% endfor %}");
+    source.append("outer={{ loop.index }} ");
+    source.append("{% endfor %}");
+    source.append("outside loop={{ loop.index }} ");
+
+    PebbleTemplate template = pebble.getTemplate(source.toString());
+
+    Map<String, Object> context = new HashMap<>();
+
+    Writer writer = new StringWriter();
+    template.evaluate(writer, context);
+    assertEquals("inner=0 inner=1 inner=2 outer=0 inner=0 inner=1 inner=2 outer=1 inner=0 inner=1 inner=2 outer=2 outside loop= ", writer.toString());
+  }
+}
\ No newline at end of file