diff --git a/src/Forms/GridField/GridField.php b/src/Forms/GridField/GridField.php index 94e335629..ebaeb6be7 100644 --- a/src/Forms/GridField/GridField.php +++ b/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.', diff --git a/src/ORM/ArrayLib.php b/src/ORM/ArrayLib.php index 8df958d59..d47bc073b 100644 --- a/src/ORM/ArrayLib.php +++ b/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]; + } + } + } + } } diff --git a/src/ORM/Hierarchy/MarkedSet.php b/src/ORM/Hierarchy/MarkedSet.php index 353b11cf8..96fd23602 100644 --- a/src/ORM/Hierarchy/MarkedSet.php +++ b/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 diff --git a/src/View/Parsers/Diff.php b/src/View/Parsers/Diff.php index c32606fc3..2072e854e 100644 --- a/src/View/Parsers/Diff.php +++ b/src/View/Parsers/Diff.php @@ -181,16 +181,15 @@ class Diff extends \Diff $content = str_replace(array(" ", "<", ">"), 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; diff --git a/tests/php/ORM/ArrayLibTest.php b/tests/php/ORM/ArrayLibTest.php index b4bc00280..91460b747 100644 --- a/tests/php/ORM/ArrayLibTest.php +++ b/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' + ); + } }