Repository: PebbleTemplates/pebble
Pull request #211 (2016-07-21 20:38:19)

Allow the set tag to update existing values in parent scopes
This fixes a problem when using the set tag in a nested for tag. The for
tag pushes a new scope if there is already a "loop" variable in scope. If
a set tag executes in that nested scope, the changes are undone at the end
of the for tag, which is not what users expect.

Example:

{% set x = 1 %}
{% for i in 1..1 %}
  {% for j in 1..1 %}
    {% set x = 2 %}
  {% endfor %}
{% endfor %}
{{ x }}

Previously this would yield "1"; now it will yield "2".

toby-allsopp: This is a fix for issue #201, in which set tags inside nested for loops are undone at the end of the for loop.
mbosecke: This is a clever solution, thanks. I'll have to think a bit more about scenarios where a user may have been relying on the old behavior but I'm optimistic that this is the way to go.
toby-allsopp: Cool, thanks for taking a look.

FYI, we're no longer relying on this change. We were using it so we could implement a global counter that increments throughout the template, but we discovered that that approach can't work through macros so I changed to a static variable in a custom tag, which is totally thread-unsafe but works fine for our use case.

Patch:
diff --git a/src/main/java/com/mitchellbosecke/pebble/node/SetNode.java b/src/main/java/com/mitchellbosecke/pebble/node/SetNode.java
index a17a05cb..22ab351c 100644
--- a/src/main/java/com/mitchellbosecke/pebble/node/SetNode.java
+++ b/src/main/java/com/mitchellbosecke/pebble/node/SetNode.java
@@ -30,7 +30,7 @@ public class SetNode extends AbstractRenderableNode {
 
     @Override
     public void render(PebbleTemplateImpl self, Writer writer, EvaluationContext context) throws PebbleException {
-        context.getScopeChain().put(name, value.evaluate(self, context));
+        context.getScopeChain().set(name, value.evaluate(self, context));
     }
 
     @Override
diff --git a/src/main/java/com/mitchellbosecke/pebble/template/ScopeChain.java b/src/main/java/com/mitchellbosecke/pebble/template/ScopeChain.java
index 109b685c..3e84ecee 100644
--- a/src/main/java/com/mitchellbosecke/pebble/template/ScopeChain.java
+++ b/src/main/java/com/mitchellbosecke/pebble/template/ScopeChain.java
@@ -193,4 +193,41 @@ public class ScopeChain {
         return stack.getFirst().containsKey(variableName);
     }
 
+    /**
+     * Sets the value of a variable in the first scope in the chain that
+     * already contains the variable; adds a variable to the current scope
+     * if an existing variable is not found.
+     *
+     * @param key   The name of the variable
+     * @param value The value of the variable
+     */
+    public void set(String key, Object value) {
+        /*
+         * The majority of time, the requested variable will be in the first
+         * scope so we do a quick lookup in that scope before attempting to
+         * create an iterator, etc. This is solely for performance.
+         */
+        Scope scope = stack.getFirst();
+        if (scope.isLocal() || scope.containsKey(key)) {
+            scope.put(key, value);
+            return;
+        }
+
+        Iterator<Scope> iterator = stack.iterator();
+
+        // account for the first lookup we did
+        iterator.next();
+
+        while (iterator.hasNext()) {
+            scope = iterator.next();
+
+            if (scope.isLocal() || scope.containsKey(key)) {
+                scope.put(key, value);
+                return;
+            }
+        }
+
+        // no existing variable, create a new one
+        put(key, value);
+    }
 }


Test patch:
diff --git a/src/test/java/com/mitchellbosecke/pebble/CoreTagsTest.java b/src/test/java/com/mitchellbosecke/pebble/CoreTagsTest.java
index 71dd9289..bef0c28b 100755
--- a/src/test/java/com/mitchellbosecke/pebble/CoreTagsTest.java
+++ b/src/test/java/com/mitchellbosecke/pebble/CoreTagsTest.java
@@ -856,6 +856,26 @@ public class CoreTagsTest extends AbstractTest {
         assertEquals("SUCCESS", writer.toString());
     }
 
+    @Test
+    public void testReSetInForLoop() throws PebbleException, IOException {
+        PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader()).strictVariables(false).build();
+
+        String source = "{% set total = 0 %}{% for i in 1..1 %}{% for item in items %}{% set total = total + item.balance %}{% endfor %}{% endfor %}{{ total }}";
+        PebbleTemplate template = pebble.getTemplate(source);
+        Map<String, Object> context = new HashMap<>();
+        List<Map<String,Object>> items = new ArrayList<>();
+        for (int i = 1; i < 4 ; ++i) {
+            Map<String, Object> item = new HashMap<>();
+            item.put("balance", i);
+            items.add(item);
+        }
+        context.put("items", items);
+
+        Writer writer = new StringWriter();
+        template.evaluate(writer, context);
+        assertEquals("6", writer.toString());
+    }
+
     @Test
     public void testVerbatim() throws PebbleException, IOException {
         PebbleEngine pebble = new PebbleEngine.Builder().loader(new StringLoader()).strictVariables(false).build();
diff --git a/src/test/java/com/mitchellbosecke/pebble/ScopeChainTest.java b/src/test/java/com/mitchellbosecke/pebble/ScopeChainTest.java
new file mode 100644
index 00000000..9883cf25
--- /dev/null
+++ b/src/test/java/com/mitchellbosecke/pebble/ScopeChainTest.java
@@ -0,0 +1,40 @@
+/*******************************************************************************
+ * This file is part of Pebble.
+ * <p>
+ * Copyright (c) 2014 by Mitchell Bösecke
+ * <p>
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ ******************************************************************************/
+package com.mitchellbosecke.pebble;
+
+import com.mitchellbosecke.pebble.error.PebbleException;
+import com.mitchellbosecke.pebble.template.ScopeChain;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.junit.Assert.assertEquals;
+
+public class ScopeChainTest extends AbstractTest {
+
+    @Test
+    public void testSet() throws PebbleException, IOException {
+        ScopeChain scopeChain = new ScopeChain();
+        scopeChain.pushScope();
+        scopeChain.set("key", "value");
+        assertEquals("value", scopeChain.get("key"));
+        scopeChain.pushScope();
+        scopeChain.set("key", "value2");
+        assertEquals("value2", scopeChain.get("key"));
+        scopeChain.popScope();
+        assertEquals("value2", scopeChain.get("key"));
+        scopeChain.pushLocalScope();
+        scopeChain.set("key", "value3");
+        assertEquals("value3", scopeChain.get("key"));
+        scopeChain.popScope();
+        assertEquals("value2", scopeChain.get("key"));
+    }
+}