FIX Don't use ArrayIterator for templates

silverstripe · May 23, 2024 · d8c8478 · d8c8478
1 parent 1f13108
commit d8c8478
Show file tree

Hide file tree

Showing 8 changed files with 26 additions and 118 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,
             ],
         ];
     }