From 3fb7afad2869f2606dd1ebf344a00818fb7857b0 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Thu, 15 Mar 2018 22:37:38 -0400 Subject: [PATCH] Fix recursive plugin loading. Plugins can load other plugins. Support this use case by implementing the `Iterator` interface instead of `IteratorAggregate`. Using the basic interface lets us extend the collection as it is being iterated, which `IteratorAggregate` cannot do. Refs #11787 --- composer.json | 1 + src/Core/PluginCollection.php | 72 +++++++++++++++++-- tests/TestCase/Core/PluginTest.php | 15 +++- tests/TestCase/Http/BaseApplicationTest.php | 29 ++++++++ .../Plugin/ParentPlugin/src/Plugin.php | 18 +++++ 5 files changed, 126 insertions(+), 9 deletions(-) create mode 100644 tests/test_app/Plugin/ParentPlugin/src/Plugin.php diff --git a/composer.json b/composer.json index 10c3811c189..c57059a3b69 100644 --- 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/PluginCollection.php b/src/Core/PluginCollection.php index e3c6cbf7109..1fcb53e5066 100644 --- 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 index ca67fcc3de3..358231caf62 100644 --- 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 index bacbd2a82dd..cb3fc8b5499 100644 --- 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 new file mode 100644 index 00000000000..88015695070 --- /dev/null +++ b/tests/test_app/Plugin/ParentPlugin/src/Plugin.php @@ -0,0 +1,18 @@ + true]); + $app->addPlugin('TestPlugin', ['bootstrap' => true]); + } +}