FIX Don't use ArrayIterator for templates

silverstripe · May 22, 2024 · edc33c4 · edc33c4
1 parent 6ce976a
commit edc33c4
Show file tree

Hide file tree

Showing 8 changed files with 16 additions and 60 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
@@ -308,29 +308,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 +318,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,17 @@ public function getItem()
         if (is_scalar($item)) {
             $item = $this->convertScalarToDBField($item);
         }
-        // Wrap arrays
+
+        // Wrap arrays in ViewableData so templates can handle them
         if (is_array($item)) {
             if (array_is_list($item)) {
-                // Wrap in ArrayIterator to respect method signature
-                $item = new ArrayIterator($item);
+                $item = ArrayList::create($item);
             } else {
-                // Wrap in ArrayData so values can be accessed by key in templates
+                // Allow values to be accessed by key in templates
                 $item = ArrayData::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,13 +559,12 @@ public function obj($fieldName, $arguments = [], $cache = false, $cacheName = nu
             $value = $this->$fieldName;
         }
 
-        // Wrap arrays
+        // Wrap arrays in ViewableData so templates can handle them
         if (is_array($value)) {
             if (array_is_list($value)) {
-                // Wrap in ArrayIterator to respect method signature
-                $value = new ArrayIterator($value);
+                $value = ArrayList::create($value);
             } else {
-                // Wrap in ArrayData so values can be accessed by key in templates
+                // Allow values to be accessed by key in templates
                 $value = ArrayData::create($value);
             }
         }
@@ -615,9 +615,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;
     }
 

diff --git a/tests/php/View/SSViewerTest.php b/tests/php/View/SSViewerTest.php
@@ -1429,10 +1429,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,

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,14 +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,
+                'expectedClass' => ArrayList::class,
             ],
             'fully associative array' => [
                 'arr' => [