Merge pull request #13264 from cakephp/issue-13246

Fix nested plugin enumeration corrupting iterator state.
cakephp · May 23, 2019 · 0decd75 · 0decd75
2 parents e67bb6c + 7745d0c
commit 0decd75
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 18 deletions.
diff --git a/src/Core/PluginCollection.php b/src/Core/PluginCollection.php
@@ -28,6 +28,9 @@
  * This class implements the Iterator interface to allow plugins
  * to be iterated, handling the situation where a plugin's hook
  * method (usually bootstrap) loads another plugin during iteration.
+ *
+ * While its implementation supported nested iteration it does not
+ * support using `continue` or `break` inside loops.
  */
 class PluginCollection implements Iterator, Countable
 {
@@ -46,11 +49,18 @@ class PluginCollection implements Iterator, Countable
     protected $names = [];
 
     /**
-     * Iterator position.
+     * Iterator position stack.
+     *
+     * @var int[]
+     */
+    protected $positions = [];
+
+    /**
+     * Loop depth
      *
      * @var int
      */
-    protected $position = 0;
+    protected $loopDepth = -1;
 
     /**
      * Constructor
@@ -166,6 +176,9 @@ public function remove($name)
     public function clear()
     {
         $this->plugins = [];
+        $this->names = [];
+        $this->positions = [];
+        $this->loopDepth = -1;
 
         return $this;
     }
@@ -197,14 +210,26 @@ public function get($name)
         return $this->plugins[$name];
     }
 
+    /**
+     * Implementation of Countable.
+     *
+     * Get the number of plugins in the collection.
+     *
+     * @return int
+     */
+    public function count()
+    {
+        return count($this->plugins);
+    }
+
     /**
      * Part of Iterator Interface
      *
      * @return void
      */
     public function next()
     {
-        $this->position++;
+        $this->positions[$this->loopDepth]++;
     }
 
     /**
@@ -214,7 +239,7 @@ public function next()
      */
     public function key()
     {
-        return $this->names[$this->position];
+        return $this->names[$this->positions[$this->loopDepth]];
     }
 
     /**
@@ -224,7 +249,8 @@ public function key()
      */
     public function current()
     {
-        $name = $this->names[$this->position];
+        $position = $this->positions[$this->loopDepth];
+        $name = $this->names[$position];
 
         return $this->plugins[$name];
     }
@@ -236,7 +262,8 @@ public function current()
      */
     public function rewind()
     {
-        $this->position = 0;
+        $this->positions[] = 0;
+        $this->loopDepth += 1;
     }
 
     /**
@@ -246,19 +273,13 @@ public function rewind()
      */
     public function valid()
     {
-        return $this->position < count($this->plugins);
-    }
+        $valid = isset($this->names[$this->positions[$this->loopDepth]]);
+        if (!$valid) {
+            array_pop($this->positions);
+            $this->loopDepth -= 1;
+        }
 
-    /**
-     * Implementation of Countable.
-     *
-     * Get the number of plugins in the collection.
-     *
-     * @return int
-     */
-    public function count()
-    {
-        return count($this->plugins);
+        return $valid;
     }
 
     /**

diff --git a/tests/TestCase/Core/PluginCollectionTest.php b/tests/TestCase/Core/PluginCollectionTest.php
@@ -130,6 +130,36 @@ public function testWith()
         $this->assertSame($pluginThree, $out[0]);
     }
 
+    /**
+     * Test that looping over the plugin collection during
+     * a with loop doesn't lose iteration state.
+     *
+     * This situation can happen when a plugin like bake
+     * needs to discover things inside other plugins.
+     *
+     * @return
+     */
+    public function testWithInnerIteration()
+    {
+        $plugins = new PluginCollection();
+        $plugin = new TestPlugin();
+        $pluginThree = new TestPluginThree();
+
+        $plugins->add($plugin);
+        $plugins->add($pluginThree);
+
+        $out = [];
+        foreach ($plugins->with('routes') as $p) {
+            foreach ($plugins as $i) {
+                // Do nothing, we just need to enumerate the collection
+            }
+            $out[] = $p;
+        }
+        $this->assertCount(2, $out);
+        $this->assertSame($plugin, $out[0]);
+        $this->assertSame($pluginThree, $out[1]);
+    }
+
     public function testWithInvalidHook()
     {
         $this->expectException(InvalidArgumentException::class);