API: <% loop %> and <% with %> only ever create one new scope level (fixes #4015)

docs/en/02_Developer_Guides/01_Templates/01_Syntax.md

@@ -478,21 +478,23 @@ Given the following structure, it will output the text.
 	Page 'Child 2' is a child of 'MyPage'
 
 <div class="notice" markdown="1">
-Additional selectors implicitely change the scope so you need to put additional `$Up` to get what you expect.
+Each `<% loop %>` or `<% with %>` block results in a change of scope, regardless of how the objects are traversed in the opening statement.
+See the example below:
 </div>
 
 ```ss
-<h1>Children of '$Title'</h1>
-<% loop $Children.Sort('Title').First %>
-    <%-- We have two additional selectors in the loop expression so... --%> 
-    <p>Page '$Title' is a child of '$Up.Up.Up.Title'</p>
-<% end_loop %>
+{$Title} <%-- Page title --%>
+<% with $Members.First.Organisation %>
+    {$Title} <%-- Organisation title --%>
+    {$Up.Title} <%-- Page title --%>
+    {$Up.Members.First.Name} <%-- Member name --%>
+<% end_with %>
 ```
 
 #### Top
 
 While `$Up` provides us a way to go up one level of scope, `$Top` is a shortcut to jump to the top most scope of the 
-page. The  previous example could be rewritten to use the following syntax.
+page. For example:
 
 ```ss
 <h1>Children of '$Title'</h1>

docs/en/04_Changelogs/5.0.0.md

@@ -13,6 +13,10 @@ guide developers in preparing existing 4.x code for compatibility with 5.0.
 
 ## API Changes {#api-changes}
 
+* `<% loop %>` and `<% with %>` now only ever result in one new scope level. For example
+  `<% loop $Pages.Limit(5) %>{$Up.Up.Title}<% end_loop %>` must be rewritten to use just one `$Up` statement to reach
+  the parent scope.
+
 ### General {#overview-general}
 
 * Minimum PHP version raised to 7.1

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
      *
@@ -199,14 +207,17 @@ public function getInjectedValue($property, array $params, $cast = true)
     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;
+        $itemStack = $this->getItemStack();
+        $itemStack[$upIndex][SSViewer_Scope::ITEM_OVERLAY] = $this->overlay;
+        $this->setItemStack($itemStack);
 
-            $this->setItemStack($itemStack);
-            $this->overlay = array();
+        // 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 = [];
         }
 
         return $scope;
@@ -225,7 +236,7 @@ public function popScope()
 
         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();
@@ -249,13 +260,17 @@ public function obj($name, $arguments = [], $cache = false, $cacheName = null)
             case 'Up':
                 $upIndex = $this->getUpIndex();
                 if ($upIndex === null) {
-                    user_error('Up called when we\'re already at the top of the scope', E_USER_ERROR);
+                    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

@@ -192,7 +192,7 @@ public function obj($name, $arguments = [], $cache = false, $cacheName = null)
         switch ($name) {
             case 'Up':
                 if ($this->upIndex === null) {
-                    user_error('Up called when we\'re already at the top of the scope', E_USER_ERROR);
+                    throw new \LogicException('Up called when we\'re already at the top of the scope');
                 }
 
                 list(
@@ -258,6 +258,9 @@ public function pushScope()
         $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;
@@ -291,7 +294,7 @@ public function next()
 
         if (!$this->itemIterator) {
             // Note: it is important that getIterator() is called before count() as implemenations may rely on
-            // this to efficiency get both the number of records and an iterator (e.g. DataList does this)
+            // this to efficiently get both the number of records and an iterator (e.g. DataList does this)
 
             // Item may be an array or a regular IteratorAggregate
             if (is_array($this->item)) {

tests/php/View/SSViewerTest.php

@@ -1361,31 +1361,21 @@ public function testUpInWith()
     {
 
         // Data to run the loop tests on - three levels deep
-        $data = new ArrayData(
-            array(
+        $data = new ArrayData([
             'Name' => 'Top',
-            'Foo' => new ArrayData(
-                array(
+            'Foo' => new ArrayData([
                 'Name' => 'Foo',
-                'Bar' => new ArrayData(
-                    array(
+                'Bar' => new ArrayData([
                     'Name' => 'Bar',
-                    'Baz' => new ArrayData(
-                        array(
+                    'Baz' => new ArrayData([
                         'Name' => 'Baz'
-                        )
-                    ),
-                    'Qux' => new ArrayData(
-                        array(
+                    ]),
+                    'Qux' => new ArrayData([
                         'Name' => 'Qux'
-                        )
-                    )
-                    )
-                )
-                )
-            )
-            )
-        );
+                    ])
+                ])
+            ])
+        ]);
 
         // Basic functionality
         $this->assertEquals(
@@ -1395,31 +1385,31 @@ public function testUpInWith()
 
         // 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',
-            $this->render('<% with Foo.Bar.Baz %>{$Name}{$Up.Name}{$Up.Qux.Name}<% end_with %>', $data)
+            'BazFooBar',
+            $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
             )
         );
 
         // 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
             )
@@ -1429,16 +1419,37 @@ public function testUpInWith()
         $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)
         );
     }