BUG Remove usage of deprecated each() and use a helper method instead

src/Forms/GridField/GridField.php

@@ -105,6 +105,11 @@ class GridField extends FormField
      */
     protected $name = '';
 
+    /**
+     * Pattern used for looking up
+     */
+    const FRAGMENT_REGEX = '/\$DefineFragment\(([a-z0-9\-_]+)\)/i';
+
     /**
      * @param string $name
      * @param string $title
@@ -362,11 +367,19 @@ class GridField extends FormField
             'before' => true,
             'after' => true,
         );
+        $fragmentDeferred = [];
 
-        reset($content);
+        // TODO: Break the below into separate reducer methods
 
-        while (list($contentKey, $contentValue) = each($content)) {
-            if (preg_match_all('/\$DefineFragment\(([a-z0-9\-_]+)\)/i', $contentValue, $matches)) {
+        // Continue looping if any placeholders exist
+        while (array_filter($content, function ($value) {
+            return preg_match(self::FRAGMENT_REGEX, $value);
+        })) {
+            foreach ($content as $contentKey => $contentValue) {
+                // Skip if this specific content has no placeholders
+                if (!preg_match_all(self::FRAGMENT_REGEX, $contentValue, $matches)) {
+                    continue;
+                }
                 foreach ($matches[1] as $match) {
                     $fragmentName = strtolower($match);
                     $fragmentDefined[$fragmentName] = true;
@@ -380,7 +393,7 @@ class GridField extends FormField
                     // If the fragment still has a fragment definition in it, when we should defer
                     // this item until later.
 
-                    if (preg_match('/\$DefineFragment\(([a-z0-9\-_]+)\)/i', $fragment, $matches)) {
+                    if (preg_match(self::FRAGMENT_REGEX, $fragment, $matches)) {
                         if (isset($fragmentDeferred[$contentKey]) && $fragmentDeferred[$contentKey] > 5) {
                             throw new LogicException(sprintf(
                                 'GridField HTML fragment "%s" and "%s" appear to have a circular dependency.',

src/ORM/ArrayLib.php

@@ -2,6 +2,8 @@
 
 namespace SilverStripe\ORM;
 
+use Generator;
+
 /**
  * Library of static methods for manipulating arrays.
  */
@@ -263,4 +265,31 @@ class ArrayLib
 
         return $out;
     }
+
+    /**
+     * Iterate list, but allowing for modifications to the underlying list.
+     * Items in $list will only be iterated exactly once for each key, and supports
+     * items being removed or deleted.
+     * List must be associative.
+     *
+     * @param array $list
+     * @return Generator
+     */
+    public static function iterateVolatile(array &$list)
+    {
+        // Keyed by already-iterated items
+        $iterated = [];
+        // Get all items not yet iterated
+        while ($items = array_diff_key($list, $iterated)) {
+            // Yield all results
+            foreach ($items as $key => $value) {
+                // Skip items removed by a prior step
+                if (array_key_exists($key, $list)) {
+                    // Ensure we yield from the source list
+                    $iterated[$key] = true;
+                    yield $key => $list[$key];
+                }
+            }
+        }
+    }
 }

src/ORM/Hierarchy/MarkedSet.php

@@ -5,6 +5,7 @@ namespace SilverStripe\ORM\Hierarchy;
 use InvalidArgumentException;
 use LogicException;
 use SilverStripe\Core\Injector\Injectable;
+use SilverStripe\ORM\ArrayLib;
 use SilverStripe\ORM\ArrayList;
 use SilverStripe\ORM\DataObject;
 use SilverStripe\ORM\FieldType\DBField;
@@ -459,7 +460,7 @@ class MarkedSet
 
         // Build markedNodes for this subtree until we reach the threshold
         // foreach can't handle an ever-growing $nodes list
-        while (list(, $node) = each($this->markedNodes)) {
+        foreach (ArrayLib::iterateVolatile($this->markedNodes) as $node) {
             $children = $this->markChildren($node);
             if ($nodeCountThreshold && sizeof($this->markedNodes) > $nodeCountThreshold) {
                 // Undo marking children as opened since they're lazy loaded

src/View/Parsers/Diff.php

@@ -181,16 +181,15 @@ class Diff extends \Diff
         $content = str_replace(array("&nbsp;", "<", ">"), array(" "," <", "> "), $content);
         $candidateChunks = preg_split("/[\t\r\n ]+/", $content);
         $chunks = [];
-        while ($chunk = each($candidateChunks)) {
-            $item = $chunk['value'];
+        for ($i = 0; $i < count($candidateChunks); $i++) {
+            $item = $candidateChunks[$i];
             if (isset($item[0]) && $item[0] == "<") {
                 $newChunk = $item;
                 while ($item[strlen($item)-1] != ">") {
-                    $chunk = each($candidateChunks);
-                    if ($chunk === false) {
+                    if (++$i >= count($candidateChunks)) {
                         break;
                     }
-                    $item = $chunk['value'];
+                    $item = $candidateChunks[$i];
                     $newChunk .= ' ' . $item;
                 }
                 $chunks[] = $newChunk;

tests/php/ORM/ArrayLibTest.php

@@ -246,4 +246,93 @@ class ArrayLibTest extends SapphireTest
 
         $this->assertEquals($expected, ArrayLib::flatten($options));
     }
+
+    /**
+     * Test that items can be added during iteration
+     */
+    public function testIterateVolatileAppended()
+    {
+        $initial = [
+            'one' => [ 'next' => 'two', 'prev' => null ],
+            'two' => [ 'next' => 'three', 'prev' => 'one' ],
+            'three' => [ 'next' => null, 'prev' => 'two' ],
+        ];
+
+        // Test new items are iterated
+        $items = $initial;
+        $seen = [];
+        foreach (ArrayLib::iterateVolatile($items) as $key => $value) {
+            $seen[$key] = $value;
+            // Append four
+            if ($key === 'three') {
+                $items['three']['next'] = 'four';
+                $items['four'] = [ 'next' => null, 'prev' => 'three'];
+            }
+            // Prepend zero won't force it to be iterated next, but it will be iterated
+            if ($key === 'one') {
+                $items['one']['next'] = 'zero';
+                $items = array_merge(
+                    ['zero' => [ 'next' => 'one', 'prev' => 'three']],
+                    $items
+                );
+            }
+        }
+        $expected = [
+            'one' => [ 'next' => 'two', 'prev' => null ],
+            'two' => [ 'next' => 'three', 'prev' => 'one' ],
+            'three' => [ 'next' => null, 'prev' => 'two' ],
+            'zero' => [ 'next' => 'one', 'prev' => 'three'],
+            'four' => [ 'next' => null, 'prev' => 'three']
+        ];
+        // All items are iterated (order not deterministic)
+        $this->assertEquals(
+            $expected,
+            $seen,
+            'New items are iterated over'
+        );
+    }
+
+    /**
+     * Test that items can be modified during iteration
+     */
+    public function testIterateVolatileModified()
+    {
+        $initial = [
+            'one' => [ 'next' => 'two', 'prev' => null ],
+            'two' => [ 'next' => 'three', 'prev' => 'one' ],
+            'three' => [ 'next' => 'four', 'prev' => 'two' ],
+            'four' => [ 'next' => null, 'prev' => 'three' ],
+        ];
+
+        // Test new items are iterated
+        $items = $initial;
+        $seen = [];
+        foreach (ArrayLib::iterateVolatile($items) as $key => $value) {
+            $seen[$key] = $value;
+            // One modifies two
+            if ($key === 'one') {
+                $items['two']['modifiedby'] = 'one';
+            }
+            // Two removes three, preventing it from being iterated next
+            if ($key === 'two') {
+                unset($items['three']);
+            }
+            // Four removes two, but since it's already been iterated by this point
+            // it's too late.
+            if ($key === 'four') {
+                unset($items['two']);
+            }
+        }
+        $expected = [
+            'one' => [ 'next' => 'two', 'prev' => null ],
+            'two' => [ 'next' => 'three', 'prev' => 'one', 'modifiedby' => 'one' ],
+            'four' => [ 'next' => null, 'prev' => 'three' ],
+        ];
+        // All items are iterated (order not deterministic)
+        $this->assertEquals(
+            ksort($expected),
+            ksort($seen),
+            'New items are iterated over'
+        );
+    }
 }