Merge pull request #11832 from cakephp/issue-11787

Fix recursive plugin loading
cakephp · Mar 16, 2018 · a9a0199 · a9a0199
2 parents b24bf2e + 3fb7afa
commit a9a0199
Show file tree

Hide file tree

Showing 6 changed files with 127 additions and 9 deletions.
diff --git a/composer.json b/composer.json
@@ -65,6 +65,7 @@
             "TestPluginTwo\\": "tests/test_app/Plugin/TestPluginTwo/src/",
             "Company\\TestPluginThree\\": "tests/test_app/Plugin/Company/TestPluginThree/src/",
             "Company\\TestPluginThree\\Test\\": "tests/test_app/Plugin/Company/TestPluginThree/tests/",
+            "ParentPlugin\\": "tests/test_app/Plugin/ParentPlugin/src/",
             "PluginJs\\": "tests/test_app/Plugin/PluginJs/src/"
         }
     },

diff --git a/src/Core/Plugin.php b/src/Core/Plugin.php
@@ -409,6 +409,7 @@ protected static function _includeFile($file, $ignoreMissing = false)
     /**
      * Get the shared plugin collection.
      *
+     * @internal
      * @return \Cake\Core\PluginCollection
      */
     public static function getCollection()

diff --git a/src/Core/PluginCollection.php b/src/Core/PluginCollection.php
@@ -17,7 +17,7 @@
 use Cake\Core\Exception\MissingPluginException;
 use Countable;
 use InvalidArgumentException;
-use IteratorAggregate;
+use Iterator;
 use RuntimeException;
 
 /**
@@ -26,8 +26,12 @@
  * Holds onto plugin objects loaded into an application, and
  * provides methods for iterating, and finding plugins based
  * on criteria.
+ *
+ * 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.
  */
-class PluginCollection implements IteratorAggregate, Countable
+class PluginCollection implements Iterator, Countable
 {
     /**
      * Plugin list
@@ -36,6 +40,18 @@ class PluginCollection implements IteratorAggregate, Countable
      */
     protected $plugins = [];
 
+    /**
+     * Names of plugins
+     *
+     * @var array
+     */
+    protected $names = [];
+
+    /**
+     * @var null|string
+     */
+    protected $position = 0;
+
     /**
      * Constructor
      *
@@ -60,6 +76,7 @@ public function add(PluginInterface $plugin)
     {
         $name = $plugin->getName();
         $this->plugins[$name] = $plugin;
+        $this->names = array_keys($this->plugins);
 
         return $this;
     }
@@ -73,6 +90,7 @@ public function add(PluginInterface $plugin)
     public function remove($name)
     {
         unset($this->plugins[$name]);
+        $this->names = array_keys($this->plugins);
 
         return $this;
     }
@@ -105,13 +123,55 @@ public function get($name)
     }
 
     /**
-     * Implementation of IteratorAggregate.
+     * Part of Iterator Interface
+     *
+     * @return void
+     */
+    public function next()
+    {
+        $this->position += 1;
+    }
+
+    /**
+     * Part of Iterator Interface
+     *
+     * @return string
+     */
+    public function key()
+    {
+        return $this->names[$this->position];
+    }
+
+    /**
+     * Part of Iterator Interface
+     *
+     * @return \Cake\Core\PluginInterface
+     */
+    public function current()
+    {
+        $name = $this->names[$this->position];
+
+        return $this->plugins[$name];
+    }
+
+    /**
+     * Part of Iterator Interface
      *
-     * @return \ArrayIterator
+     * @return void
+     */
+    public function rewind()
+    {
+        $this->position = 0;
+    }
+
+    /**
+     * Part of Iterator Interface
+     *
+     * @return bool
      */
-    public function getIterator()
+    public function valid()
     {
-        return new ArrayIterator($this->plugins);
+        return $this->position < count($this->plugins);
     }
 
     /**

diff --git a/tests/TestCase/Core/PluginTest.php b/tests/TestCase/Core/PluginTest.php
@@ -301,7 +301,10 @@ public function testClassPathNotFound()
     public function testLoadAll()
     {
         Plugin::loadAll();
-        $expected = ['Company', 'PluginJs', 'TestPlugin', 'TestPluginFour', 'TestPluginTwo', 'TestTheme'];
+        $expected = [
+            'Company', 'ParentPlugin', 'PluginJs', 'TestPlugin',
+            'TestPluginFour', 'TestPluginTwo', 'TestTheme'
+        ];
         $this->assertEquals($expected, Plugin::loaded());
     }
 
@@ -338,7 +341,10 @@ public function testLoadAllWithDefaults()
     {
         $defaults = ['bootstrap' => true, 'ignoreMissing' => true];
         Plugin::loadAll([$defaults]);
-        $expected = ['Company', 'PluginJs', 'TestPlugin', 'TestPluginFour', 'TestPluginTwo', 'TestTheme'];
+        $expected = [
+            'Company', 'ParentPlugin', 'PluginJs', 'TestPlugin',
+            'TestPluginFour', 'TestPluginTwo', 'TestTheme'
+        ];
         $this->assertEquals($expected, Plugin::loaded());
         $this->assertEquals('loaded js plugin bootstrap', Configure::read('PluginTest.js_plugin.bootstrap'));
         $this->assertEquals('loaded plugin bootstrap', Configure::read('PluginTest.test_plugin.bootstrap'));
@@ -360,7 +366,10 @@ public function testLoadAllWithDefaultsAndOverride()
         ]);
         Plugin::routes();
 
-        $expected = ['Company', 'PluginJs', 'TestPlugin', 'TestPluginFour', 'TestPluginTwo', 'TestTheme'];
+        $expected = [
+            'Company', 'ParentPlugin', 'PluginJs', 'TestPlugin',
+            'TestPluginFour', 'TestPluginTwo', 'TestTheme'
+        ];
         $this->assertEquals($expected, Plugin::loaded());
         $this->assertEquals('loaded js plugin bootstrap', Configure::read('PluginTest.js_plugin.bootstrap'));
         $this->assertEquals('loaded plugin routes', Configure::read('PluginTest.test_plugin.routes'));

diff --git a/tests/TestCase/Http/BaseApplicationTest.php b/tests/TestCase/Http/BaseApplicationTest.php
@@ -182,4 +182,33 @@ public function testPluginBootstrapInteractWithPluginLoad()
             'Key should not be set, as plugin has already had bootstrap run'
         );
     }
+
+    /**
+     * Test that plugins loaded with addPlugin() can load additional
+     * plugins.
+     *
+     * @return void
+     */
+    public function testPluginBootstrapRecursivePlugins()
+    {
+        $app = $this->getMockForAbstractClass(
+            BaseApplication::class,
+            [$this->path]
+        );
+        $app->addPlugin('ParentPlugin');
+        $app->pluginBootstrap();
+
+        $this->assertTrue(
+            Configure::check('ParentPlugin.bootstrap'),
+            'Plugin bootstrap should be run'
+        );
+        $this->assertTrue(
+            Configure::check('PluginTest.test_plugin.bootstrap'),
+            'Nested plugin should have bootstrap run'
+        );
+        $this->assertTrue(
+            Configure::check('PluginTest.test_plugin_two.bootstrap'),
+            'Nested plugin should have bootstrap run'
+        );
+    }
 }
diff --git a/tests/test_app/Plugin/ParentPlugin/src/Plugin.php b/tests/test_app/Plugin/ParentPlugin/src/Plugin.php
@@ -0,0 +1,18 @@
+<?php
+namespace ParentPlugin;
+
+use Cake\Core\BasePlugin;
+use Cake\Core\Configure;
+use Cake\Core\PluginApplicationInterface;
+use Cake\Core\Plugin as CorePlugin;
+
+class Plugin extends BasePlugin
+{
+    public function bootstrap(PluginApplicationInterface $app)
+    {
+        Configure::write('ParentPlugin.bootstrap', true);
+
+        CorePlugin::load('TestPluginTwo', ['bootstrap' => true]);
+        $app->addPlugin('TestPlugin', ['bootstrap' => true]);
+    }
+}