From 1ec0580653660cbb52a3a51863482e0a80cd168e Mon Sep 17 00:00:00 2001 From: Loz Calver Date: Thu, 7 Jun 2018 15:01:17 +0100 Subject: [PATCH] API: <% loop %> and <% with %> only ever create one new scope level (fixes #4015) --- .../01_Templates/01_Syntax.md | 16 +++-- docs/en/04_Changelogs/5.0.0.md | 4 ++ src/View/SSViewer_DataPresenter.php | 33 ++++++--- src/View/SSViewer_Scope.php | 7 +- tests/php/View/SSViewerTest.php | 71 +++++++++++-------- 5 files changed, 83 insertions(+), 48 deletions(-) diff --git a/docs/en/02_Developer_Guides/01_Templates/01_Syntax.md b/docs/en/02_Developer_Guides/01_Templates/01_Syntax.md index a360b742c54..76802268d39 100644 --- a/docs/en/02_Developer_Guides/01_Templates/01_Syntax.md +++ b/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'
-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:
```ss -

Children of '$Title'

-<% loop $Children.Sort('Title').First %> - <%-- We have two additional selectors in the loop expression so... --%> -

Page '$Title' is a child of '$Up.Up.Up.Title'

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

Children of '$Title'

diff --git a/docs/en/04_Changelogs/5.0.0.md b/docs/en/04_Changelogs/5.0.0.md index 1fbbc9aa376..651bf545293 100644 --- a/docs/en/04_Changelogs/5.0.0.md +++ b/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 diff --git a/src/View/SSViewer_DataPresenter.php b/src/View/SSViewer_DataPresenter.php index f8d120cd82e..5c990d07df1 100644 --- a/src/View/SSViewer_DataPresenter.php +++ b/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; } diff --git a/src/View/SSViewer_Scope.php b/src/View/SSViewer_Scope.php index f37ee2da9ae..12f2c43df48 100644 --- a/src/View/SSViewer_Scope.php +++ b/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)) { diff --git a/tests/php/View/SSViewerTest.php b/tests/php/View/SSViewerTest.php index 00c9a7dbe52..e050dc21c55 100644 --- a/tests/php/View/SSViewerTest.php +++ b/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,21 +1385,21 @@ 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 ) @@ -1417,9 +1407,9 @@ public function testUpInWith() // 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) ); }