From d8bba8b49cec59291d47d9f73a8a634de26a8416 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Fri, 15 Jun 2018 23:36:29 -0400 Subject: [PATCH] Fix duplicate route errors when using loadPlugin() Use the plugin object to load plugin routes. This allows us to disable the hook method so that it is not triggered again. There is still a scenario where a plugin loads routes from another plugin after that plugin's routes() hook has already been called. I'm going to count that as an unlikely scenario. Refs #12130 --- src/Routing/RouteBuilder.php | 39 ++++++++++----- tests/TestCase/Routing/RouteBuilderTest.php | 15 ++++-- .../TestSuite/IntegrationTestCaseTest.php | 19 ++++++++ .../Plugin/TestPlugin/config/routes.php | 6 ++- .../TestApp/ApplicationWithPluginRoutes.php | 48 +++++++++++++++++++ 5 files changed, 110 insertions(+), 17 deletions(-) create mode 100644 tests/test_app/TestApp/ApplicationWithPluginRoutes.php diff --git a/src/Routing/RouteBuilder.php b/src/Routing/RouteBuilder.php index 447ce6dd14c..652a435c0ec 100644 --- a/src/Routing/RouteBuilder.php +++ b/src/Routing/RouteBuilder.php @@ -602,28 +602,45 @@ protected function _methodRoute($method, $template, $target, $name) * the current RouteBuilder instance. * * @param string $name The plugin name - * @param string $file The routes file to load. Defaults to `routes.php` + * @param string $file The routes file to load. Defaults to `routes.php`. This parameter + * is deprecated and will be removed in 4.0 * @return void * @throws \Cake\Core\Exception\MissingPluginException When the plugin has not been loaded. * @throws \InvalidArgumentException When the plugin does not have a routes file. */ public function loadPlugin($name, $file = 'routes.php') { - if (!Plugin::loaded($name)) { + $plugins = Plugin::getCollection(); + if (!$plugins->has($name)) { throw new MissingPluginException(['plugin' => $name]); } + $plugin = $plugins->get($name); - $path = Plugin::configPath($name) . DIRECTORY_SEPARATOR . $file; - if (!file_exists($path)) { - throw new InvalidArgumentException(sprintf( - 'Cannot load routes for the plugin named %s. The %s file does not exist.', - $name, - $path - )); + // @deprecated This block should be removed in 4.0 + if ($file !== 'routes.php') { + deprecationWarning( + 'Loading plugin routes now uses the routes() hook method on the plugin class. ' . + 'Loading non-standard files will be removed in 4.0' + ); + + $path = $plugin->getConfigPath() . DIRECTORY_SEPARATOR . $file; + if (!file_exists($path)) { + throw new InvalidArgumentException(sprintf( + 'Cannot load routes for the plugin named %s. The %s file does not exist.', + $name, + $path + )); + } + + $routes = $this; + include $path; + + return; } + $plugin->routes($this); - $routes = $this; - include $path; + // Disable the routes hook to prevent duplicate route issues. + $plugin->disable('routes'); } /** diff --git a/tests/TestCase/Routing/RouteBuilderTest.php b/tests/TestCase/Routing/RouteBuilderTest.php index 6c931a8f6ed..217897fbe7a 100644 --- a/tests/TestCase/Routing/RouteBuilderTest.php +++ b/tests/TestCase/Routing/RouteBuilderTest.php @@ -1206,11 +1206,13 @@ public function testLoadPluginBadPlugin() */ public function testLoadPluginBadFile() { - $this->expectException(\InvalidArgumentException::class); - $this->expectExceptionMessage('Cannot load routes for the plugin named TestPlugin.'); - Plugin::load('TestPlugin'); - $routes = new RouteBuilder($this->collection, '/'); - $routes->loadPlugin('TestPlugin', 'nope.php'); + $this->deprecated(function () { + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('Cannot load routes for the plugin named TestPlugin.'); + Plugin::load('TestPlugin'); + $routes = new RouteBuilder($this->collection, '/'); + $routes->loadPlugin('TestPlugin', 'nope.php'); + }); } /** @@ -1225,6 +1227,9 @@ public function testLoadPlugin() $routes->loadPlugin('TestPlugin'); $this->assertCount(1, $this->collection->routes()); $this->assertNotEmpty($this->collection->parse('/test_plugin', 'GET')); + + $plugin = Plugin::getCollection()->get('TestPlugin'); + $this->assertFalse($plugin->isEnabled('routes'), 'Hook should be disabled preventing duplicate routes'); } /** diff --git a/tests/TestCase/TestSuite/IntegrationTestCaseTest.php b/tests/TestCase/TestSuite/IntegrationTestCaseTest.php index 7dfc458e1b6..f2f2381ab55 100644 --- a/tests/TestCase/TestSuite/IntegrationTestCaseTest.php +++ b/tests/TestCase/TestSuite/IntegrationTestCaseTest.php @@ -15,6 +15,7 @@ namespace Cake\Test\TestCase\TestSuite; use Cake\Core\Configure; +use Cake\Core\Plugin; use Cake\Event\EventManager; use Cake\Http\Response; use Cake\Routing\DispatcherFactory; @@ -52,6 +53,7 @@ public function setUp() Router::$initialized = true; $this->useHttpServer(true); + $this->configApplication(Configure::read('App.namespace') . '\Application', null); DispatcherFactory::clear(); } @@ -276,6 +278,23 @@ public function testGetLegacy() }); } + /** + * Test sending get request and using default `test_app/config/routes.php`. + * + * @return void + */ + public function testGetUsingApplicationWithPluginRoutes() + { + // first clean routes to have Router::$initailized === false + Router::reload(); + Plugin::unload(); + + $this->configApplication(Configure::read('App.namespace') . '\ApplicationWithPluginRoutes', null); + + $this->get('/test_plugin'); + $this->assertResponseOk(); + } + /** * Test sending get request and using default `test_app/config/routes.php`. * diff --git a/tests/test_app/Plugin/TestPlugin/config/routes.php b/tests/test_app/Plugin/TestPlugin/config/routes.php index afaad41b94f..65f8df45a46 100644 --- a/tests/test_app/Plugin/TestPlugin/config/routes.php +++ b/tests/test_app/Plugin/TestPlugin/config/routes.php @@ -4,5 +4,9 @@ Configure::write('PluginTest.test_plugin.routes', 'loaded plugin routes'); if (isset($routes)) { - $routes->get('/test_plugin', ['controller' => 'TestPlugin', 'plugin' => 'TestPlugin', 'action' => 'index']); + $routes->get( + '/test_plugin', + ['controller' => 'TestPlugin', 'plugin' => 'TestPlugin', 'action' => 'index'], + 'test_plugin:index' + ); } diff --git a/tests/test_app/TestApp/ApplicationWithPluginRoutes.php b/tests/test_app/TestApp/ApplicationWithPluginRoutes.php new file mode 100644 index 00000000000..0ecf1e4047b --- /dev/null +++ b/tests/test_app/TestApp/ApplicationWithPluginRoutes.php @@ -0,0 +1,48 @@ +addPlugin('TestPlugin'); + } + + public function middleware($middleware) + { + $middleware->add(new RoutingMiddleware($this)); + + return $middleware; + } + + /** + * Routes hook, used for testing with RoutingMiddleware. + * + * @param \Cake\Routing\RouteBuilder $routes + * @return void + */ + public function routes($routes) + { + $routes->scope('/app', function ($routes) { + $routes->connect('/articles', ['controller' => 'Articles']); + }); + $routes->loadPlugin('TestPlugin'); + } +}