From d8c8478d85cce4cdbc3871de348ca7c42569ee11 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Wed, 22 May 2024 12:15:07 +1200 Subject: [PATCH] FIX Don't use ArrayIterator for templates --- src/ORM/ArrayList.php | 2 +- src/View/SSTemplateParser.peg | 2 -- src/View/SSTemplateParser.php | 9 +------ src/View/SSViewer_DataPresenter.php | 37 +---------------------------- src/View/SSViewer_Scope.php | 15 +++++------- src/View/ViewableData.php | 25 +++++++++---------- tests/php/View/SSViewerTest.php | 34 +------------------------- tests/php/View/ViewableDataTest.php | 20 +++------------- 8 files changed, 26 insertions(+), 118 deletions(-) diff --git a/src/ORM/ArrayList.php b/src/ORM/ArrayList.php index f953ffb6176..2d98d26f984 100644 --- a/src/ORM/ArrayList.php +++ b/src/ORM/ArrayList.php @@ -127,7 +127,7 @@ public function exists() public function getIterator(): Traversable { foreach ($this->items as $i => $item) { - if (is_array($item)) { + if (is_array($item) && !array_is_list($item)) { yield new ArrayData($item); } else { yield $item; diff --git a/src/View/SSTemplateParser.peg b/src/View/SSTemplateParser.peg index cefc8a34819..0f15460f97b 100644 --- a/src/View/SSTemplateParser.peg +++ b/src/View/SSTemplateParser.peg @@ -287,8 +287,6 @@ class SSTemplateParser extends Parser implements TemplateParser if (isset($sub['Call']['CallArguments']) && isset($sub['Call']['CallArguments']['php'])) { $arguments = $sub['Call']['CallArguments']['php']; $res['php'] .= "->$method('$property', [$arguments], true)"; - } elseif ($property === 'Count') { - $res['php'] .= "->$property()"; } else { $res['php'] .= "->$method('$property', null, true)"; } diff --git a/src/View/SSTemplateParser.php b/src/View/SSTemplateParser.php index 548fa84bad1..bafe80e4be4 100644 --- a/src/View/SSTemplateParser.php +++ b/src/View/SSTemplateParser.php @@ -778,8 +778,6 @@ function Lookup_AddLookupStep(&$res, $sub, $method) if (isset($sub['Call']['CallArguments']) && isset($sub['Call']['CallArguments']['php'])) { $arguments = $sub['Call']['CallArguments']['php']; $res['php'] .= "->$method('$property', [$arguments], true)"; - } elseif ($property === 'Count') { - $res['php'] .= "->$property()"; } else { $res['php'] .= "->$method('$property', null, true)"; } @@ -1888,8 +1886,6 @@ function PresenceCheck_Argument(&$res, $sub) $res['php'] .= '((bool)'.$sub['php'].')'; } else { $php = ($sub['ArgumentMode'] == 'default' ? $sub['lookup_php'] : $sub['php']); - // TODO: kinda hacky - maybe we need a way to pass state down the parse chain so - // Lookup_LastLookupStep and Argument_BareWord can produce hasValue instead of XML_val $res['php'] .= str_replace('$$FINAL', 'hasValue', $php ?? ''); } } @@ -5294,8 +5290,6 @@ function Text__finalise(&$res) $text = stripslashes($text ?? ''); $text = addcslashes($text ?? '', '\'\\'); - // TODO: This is pretty ugly & gets applied on all files not just html. I wonder if we can make this - // non-dynamically calculated $code = <<<'EOC' (\SilverStripe\View\SSViewer::getRewriteHashLinksDefault() ? \SilverStripe\Core\Convert::raw2att( preg_replace("/^(\\/)+/", "/", $_SERVER['REQUEST_URI'] ) ) @@ -5334,8 +5328,7 @@ public function compileString($string, $templateName = "", $includeDebuggingComm $this->includeDebuggingComments = $includeDebuggingComments; - // Ignore UTF8 BOM at beginning of string. TODO: Confirm this is needed, make sure SSViewer handles UTF - // (and other encodings) properly + // Ignore UTF8 BOM at beginning of string. if (substr($string ?? '', 0, 3) == pack("CCC", 0xef, 0xbb, 0xbf)) { $this->pos = 3; } diff --git a/src/View/SSViewer_DataPresenter.php b/src/View/SSViewer_DataPresenter.php index 3c836937d07..4735bba01a2 100644 --- a/src/View/SSViewer_DataPresenter.php +++ b/src/View/SSViewer_DataPresenter.php @@ -172,11 +172,6 @@ public function getInjectedValue($property, array $params, $cast = true) // Get source for this value $result = $this->getValueSource($property); if (!array_key_exists('source', $result)) { - $obj = $this->getItem(); - // $Me is a special property. If nothing is providing an override, return the current item. - if ($property === 'Me' && !isset($obj->$property)) { - return ['obj' => $this->getItem()]; - } return null; } @@ -308,29 +303,6 @@ public function getObj($name, $arguments = [], $cache = false, $cacheName = null */ public function __call($name, $arguments) { - // $Count should be handled specially, so we can count raw arrays and iterables - // which aren't ViewableData. - if (empty($arguments) && ($name === 'Count' || $name === 'count')) { - $item = $this->getItem(); - $result = null; - if ($item instanceof ViewableData) { - // Respect ViewableData casting - $result = $item->XML_val($name, [], true); - } elseif (is_object($item)) { - // Get the method or property from objects, if there is one - if (ClassInfo::hasMethod($item, $name)) { - $result = $item->$name(); - } elseif (isset($item->$name)) { - $result = $item->$name; - } - } elseif (is_countable($item)) { - // Count countables - $result = count($item); - } - $this->resetLocalScope(); - return $result; - } - // Extract the method name and parameters $property = $arguments[0]; // The name of the public function being called @@ -341,14 +313,7 @@ public function __call($name, $arguments) if ($val) { $obj = $val['obj']; if ($name === 'hasValue') { - // Check if a value exists, e.g. <% if $obj %> - if ($obj instanceof ViewableData) { - $result = $obj->exists(); - } elseif (is_countable($obj)) { - $result = count($obj) > 0; - } else { - $result = (bool) $obj; - } + $result = ($obj instanceof ViewableData) ? $obj->exists() : (bool)$obj; } elseif (is_null($obj) || (is_scalar($obj) && !is_string($obj))) { $result = $obj; // Nulls and non-string scalars don't need casting } else { diff --git a/src/View/SSViewer_Scope.php b/src/View/SSViewer_Scope.php index 6f4af08c308..658756b659c 100644 --- a/src/View/SSViewer_Scope.php +++ b/src/View/SSViewer_Scope.php @@ -5,6 +5,7 @@ use ArrayIterator; use Countable; use Iterator; +use SilverStripe\ORM\ArrayList; use SilverStripe\ORM\FieldType\DBBoolean; use SilverStripe\ORM\FieldType\DBText; use SilverStripe\ORM\FieldType\DBFloat; @@ -130,16 +131,12 @@ public function getItem() if (is_scalar($item)) { $item = $this->convertScalarToDBField($item); } - // Wrap arrays - if (is_array($item)) { - if (array_is_list($item)) { - // Wrap in ArrayIterator to respect method signature - $item = new ArrayIterator($item); - } else { - // Wrap in ArrayData so values can be accessed by key in templates - $item = ArrayData::create($item); - } + + // Wrap list arrays in ViewableData so templates can handle them + if (is_array($item) && array_is_list($item)) { + $item = ArrayList::create($item); } + return $item; } diff --git a/src/View/ViewableData.php b/src/View/ViewableData.php index e694bd8e5fb..e5a2c507c4f 100644 --- a/src/View/ViewableData.php +++ b/src/View/ViewableData.php @@ -20,6 +20,7 @@ use SilverStripe\Dev\Debug; use SilverStripe\Dev\Deprecation; use SilverStripe\ORM\ArrayLib; +use SilverStripe\ORM\ArrayList; use SilverStripe\ORM\FieldType\DBField; use SilverStripe\ORM\FieldType\DBHTMLText; use SilverStripe\View\SSViewer; @@ -558,15 +559,9 @@ public function obj($fieldName, $arguments = [], $cache = false, $cacheName = nu $value = $this->$fieldName; } - // Wrap arrays - if (is_array($value)) { - if (array_is_list($value)) { - // Wrap in ArrayIterator to respect method signature - $value = new ArrayIterator($value); - } else { - // Wrap in ArrayData so values can be accessed by key in templates - $value = ArrayData::create($value); - } + // Wrap list arrays in ViewableData so templates can handle them + if (is_array($value) && array_is_list($value)) { + $value = ArrayList::create($value); } // Cast object @@ -615,9 +610,6 @@ public function hasValue($field, $arguments = [], $cache = true) if ($result instanceof ViewableData) { return $result->exists(); } - if (is_countable($result)) { - return count($result) > 0; - } return (bool) $result; } @@ -685,6 +677,15 @@ public function getViewerTemplates($suffix = '') return SSViewer::get_templates_by_class(static::class, $suffix, self::class); } + /** + * When rendering some objects it is necessary to iterate over the object being rendered, to do this, you need + * access to itself. + */ + public function Me(): static + { + return $this; + } + /** * Get part of the current classes ancestry to be used as a CSS class. * diff --git a/tests/php/View/SSViewerTest.php b/tests/php/View/SSViewerTest.php index 92dc3896d8f..4a9aa0a42f8 100644 --- a/tests/php/View/SSViewerTest.php +++ b/tests/php/View/SSViewerTest.php @@ -2,7 +2,6 @@ namespace SilverStripe\View\Tests; -use ArrayIterator; use Exception; use InvalidArgumentException; use LogicException; @@ -1037,10 +1036,6 @@ public function provideIfBlockWithIterable(): array 'iterable' => [1, 2, 3], 'inScope' => false, ], - 'iterator' => [ - 'iterable' => new ArrayIterator([1, 2, 3]), - 'inScope' => false, - ], 'ArrayList' => [ 'iterable' => new ArrayList([['Val' => 1], ['Val' => 2], ['Val' => 3]]), 'inScope' => false, @@ -1392,7 +1387,7 @@ public function provideLoop(): array 'value 1', 'value 2', ], - new ArrayIterator([ + new ArrayList([ 'value 3', 'value 4', ]), @@ -1438,10 +1433,6 @@ public function provideCountIterable(): array 'iterable' => [1, 2, 3], 'inScope' => false, ], - 'iterator' => [ - 'iterable' => new ArrayIterator([1, 2, 3]), - 'inScope' => false, - ], 'ArrayList' => [ 'iterable' => new ArrayList([['Val' => 1], ['Val' => 2], ['Val' => 3]]), 'inScope' => false, @@ -2375,27 +2366,4 @@ public function testMe(): void $mockArrayData->expects($this->once())->method('forTemplate'); $this->render('$Me', $mockArrayData); } - - public function provideAccessAssociativeArrayValues(): array - { - return [ - 'in scope' => [ - true, - ], - 'not in scope' => [ - false, - ], - ]; - } - - /** - * @dataProvider provideAccessAssociativeArrayValues - */ - public function testAccessAssociativeArrayValues(bool $inScope): void - { - $data = new ViewableData(); - $data->Foo = ['Value1' => '1', 'Value2' => 'two']; - $template = $inScope ? '<% with $Foo %>$Value1 $Value2<% end_with %>' : '$Foo.Value1 $Foo.Value2'; - $this->assertEqualIgnoringWhitespace('1 two', $this->render($template, $data)); - } } diff --git a/tests/php/View/ViewableDataTest.php b/tests/php/View/ViewableDataTest.php index 3403fb2b9e2..1610fdb63ba 100644 --- a/tests/php/View/ViewableDataTest.php +++ b/tests/php/View/ViewableDataTest.php @@ -2,10 +2,10 @@ namespace SilverStripe\View\Tests; -use ArrayIterator; use ReflectionMethod; use SilverStripe\ORM\FieldType\DBField; use SilverStripe\Dev\SapphireTest; +use SilverStripe\ORM\ArrayList; use SilverStripe\View\ArrayData; use SilverStripe\View\SSViewer; use SilverStripe\View\Tests\ViewableDataTest\ViewableDataTestExtension; @@ -285,28 +285,14 @@ public function provideWrapArrayInObj(): array return [ 'empty array' => [ 'arr' => [], - 'expectedClass' => ArrayIterator::class, + 'expectedClass' => ArrayList::class, ], 'fully indexed array' => [ 'arr' => [ 'value1', 'value2', ], - 'expectedClass' => ArrayIterator::class, - ], - 'fully associative array' => [ - 'arr' => [ - 'v1' => 'value1', - 'v2' => 'value2', - ], - 'expectedClass' => ArrayData::class, - ], - 'partially associative array' => [ - 'arr' => [ - 'value1', - 'v2' => 'value2', - ], - 'expectedClass' => ArrayData::class, + 'expectedClass' => ArrayList::class, ], ]; }