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

@@ -602,28 +602,45 @@ class RouteBuilder
      * 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');
     }
 
     /**

tests/TestCase/Routing/RouteBuilderTest.php

@@ -1206,11 +1206,13 @@ class RouteBuilderTest extends TestCase
      */
     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 @@ class RouteBuilderTest extends TestCase
         $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');
     }
 
     /**

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 @@ class IntegrationTestCaseTest extends IntegrationTestCase
         Router::$initialized = true;
 
         $this->useHttpServer(true);
+        $this->configApplication(Configure::read('App.namespace') . '\Application', null);
         DispatcherFactory::clear();
     }
 
@@ -281,6 +283,23 @@ class IntegrationTestCaseTest extends IntegrationTestCase
      *
      * @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`.
+     *
+     * @return void
+     */
     public function testGetUsingApplicationWithDefaultRoutes()
     {
         // first clean routes to have Router::$initailized === false

tests/test_app/Plugin/TestPlugin/config/routes.php

@@ -4,5 +4,9 @@ use Cake\Core\Configure;
 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'
+    );
 }

tests/test_app/TestApp/ApplicationWithPluginRoutes.php

@@ -0,0 +1,48 @@
+<?php
+/**
+ * CakePHP(tm) : Rapid Development Framework (https://cakephp.org)
+ * Copyright (c) Cake Software Foundation, Inc. (https://cakefoundation.org)
+ *
+ * Licensed under The MIT License
+ * For full copyright and license information, please see the LICENSE.txt
+ * Redistributions of files must retain the above copyright notice
+ *
+ * @copyright     Copyright (c) Cake Software Foundation, Inc. (https://cakefoundation.org)
+ * @link          https://cakephp.org CakePHP(tm) Project
+ * @since         3.6.6
+ * @license       https://opensource.org/licenses/mit-license.php MIT License
+ */
+namespace TestApp;
+
+use Cake\Http\BaseApplication;
+use Cake\Routing\Middleware\RoutingMiddleware;
+
+class ApplicationWithPluginRoutes extends BaseApplication
+{
+    public function bootstrap()
+    {
+        parent::bootstrap();
+        $this->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');
+    }
+}