API: <% loop %> and <% with %> only ever create one new scope level

src/View/SSViewer_DataPresenter.php

@@ -38,6 +38,14 @@ class SSViewer_DataPresenter extends SSViewer_Scope
      */
     protected $overlay;
 
+    /**
+     * Flag for whether overlay should be preserved when pushing a new scope
+     *
+     * @see SSViewer_DataPresenter::pushScope()
+     * @var bool
+     */
+    protected $preserveOverlay = false;
+
     /**
      * Underlay variables. Concede precedence to overlay variables or anything from the current scope
      *
@@ -200,13 +208,16 @@ class SSViewer_DataPresenter extends SSViewer_Scope
     public function pushScope()
     {
         $scope = parent::pushScope();
-        $upIndex = $this->getUpIndex();
+        $upIndex = $this->getUpIndex() ?: 0;
 
-        if ($upIndex !== null) {
         $itemStack = $this->getItemStack();
         $itemStack[$upIndex][SSViewer_Scope::ITEM_OVERLAY] = $this->overlay;
-
         $this->setItemStack($itemStack);
+
+        // Remove the overlay when we're changing to a new scope, as values in
+        // that scope take priority. The exceptions that set this flag are $Up
+        // and $Top as they require that the new scope inherits the overlay
+        if (!$this->preserveOverlay) {
             $this->overlay = [];
         }
 
@@ -226,7 +237,7 @@ class SSViewer_DataPresenter extends SSViewer_Scope
 
         if ($upIndex !== null) {
             $itemStack = $this->getItemStack();
-            $this->overlay = $itemStack[$this->getUpIndex()][SSViewer_Scope::ITEM_OVERLAY];
+            $this->overlay = $itemStack[$upIndex][SSViewer_Scope::ITEM_OVERLAY];
         }
 
         return parent::popScope();
@@ -252,11 +263,15 @@ class SSViewer_DataPresenter extends SSViewer_Scope
                 if ($upIndex === null) {
                     throw new \LogicException('Up called when we\'re already at the top of the scope');
                 }
-
                 $overlayIndex = $upIndex; // Parent scope
+                $this->preserveOverlay = true; // Preserve overlay
                 break;
             case 'Top':
                 $overlayIndex = 0; // Top-level scope
+                $this->preserveOverlay = true; // Preserve overlay
+                break;
+            default:
+                $this->preserveOverlay = false;
                 break;
         }
 

src/View/SSViewer_Scope.php

@@ -257,6 +257,9 @@ class SSViewer_Scope
         $this->popIndex = $this->itemStack[$newLocalIndex][SSViewer_Scope::POP_INDEX] = $this->localIndex;
         $this->localIndex = $newLocalIndex;
 
+        // $Up now becomes the parent scope - the parent of the current <% loop %> or <% with %>
+        $this->upIndex = $this->itemStack[$newLocalIndex][SSViewer_Scope::UP_INDEX] = $this->popIndex;
+
         // We normally keep any previous itemIterator around, so local $Up calls reference the right element. But
         // once we enter a new global scope, we need to make sure we use a new one
         $this->itemIterator = $this->itemStack[$newLocalIndex][SSViewer_Scope::ITEM_ITERATOR] = null;

tests/php/View/SSViewerTest.php

@@ -1510,21 +1510,21 @@ after'
 
         // Two level with block, up refers to internally referenced Bar
         $this->assertEquals(
-            'BarFoo',
+            'BarTop',
             $this->render('<% with Foo.Bar %>{$Name}{$Up.Name}<% end_with %>', $data)
         );
 
         // Stepping up & back down the scope tree
         $this->assertEquals(
-            'BazBarQux',
+            'BazFooBar',
-            $this->render('<% with Foo.Bar.Baz %>{$Name}{$Up.Name}{$Up.Qux.Name}<% end_with %>', $data)
+            $this->render('<% with Foo.Bar.Baz %>{$Name}{$Up.Foo.Name}{$Up.Foo.Bar.Name}<% end_with %>', $data)
         );
 
         // Using $Up in a with block
         $this->assertEquals(
-            'BazBarQux',
+            'BazTopBar',
             $this->render(
-                '<% with Foo.Bar.Baz %>{$Name}<% with $Up %>{$Name}{$Qux.Name}<% end_with %>'
+                '<% with Foo.Bar.Baz %>{$Name}<% with $Up %>{$Name}{$Foo.Bar.Name}<% end_with %>'
                 . '<% end_with %>',
                 $data
             )
@@ -1532,9 +1532,9 @@ after'
 
         // Stepping up & back down the scope tree with with blocks
         $this->assertEquals(
-            'BazBarQuxBarBaz',
+            'BazTopBarTopBaz',
             $this->render(
-                '<% with Foo.Bar.Baz %>{$Name}<% with $Up %>{$Name}<% with Qux %>{$Name}<% end_with %>'
+                '<% with Foo.Bar.Baz %>{$Name}<% with $Up %>{$Name}<% with Foo.Bar %>{$Name}<% end_with %>'
                 . '{$Name}<% end_with %>{$Name}<% end_with %>',
                 $data
             )
@@ -1544,16 +1544,37 @@ after'
         $this->assertEquals(
             'Foo',
             $this->render(
-                '<% with Foo.Bar.Baz %><% with Up %><% with Qux %>{$Up.Up.Name}<% end_with %><% end_with %>'
+                '<% with Foo %><% with Bar %><% with Baz %>{$Up.Up.Name}<% end_with %><% end_with %>'
                 . '<% end_with %>',
                 $data
             )
         );
 
-        // Using $Up.Up, where first $Up points to an Up used in a local scope lookup, should still skip to Foo
+        // Using $Up as part of a lookup chain in <% with %>
+        $this->assertEquals(
+            'Top',
+            $this->render('<% with Foo.Bar.Baz.Up.Qux %>{$Up.Name}<% end_with %>', $data)
+        );
+    }
+
+    /**
+     * @expectedException \LogicException
+     * @expectedExceptionMessage Up called when we're already at the top of the scope
+     */
+    public function testTooManyUps()
+    {
+        $data = new ArrayData([
+            'Foo' => new ArrayData([
+                'Name' => 'Foo',
+                'Bar' => new ArrayData([
+                    'Name' => 'Bar'
+                ])
+            ])
+        ]);
+
         $this->assertEquals(
             'Foo',
-            $this->render('<% with Foo.Bar.Baz.Up.Qux %>{$Up.Up.Name}<% end_with %>', $data)
+            $this->render('<% with Foo.Bar %>{$Up.Up.Name}<% end_with %>', $data)
         );
     }