Ensure cached and uncached routes share same precedence when resolving actions and names (laravel#56920)

* Ensure cached and uncached routes share same precedence

* formatting

* Formatting

* formatting

* Fix tests

Author: timacdonald

src/Illuminate/Routing/RouteCollection.php

@@ -91,7 +91,7 @@ protected function addLookups($route)
         // If the route has a name, we will add it to the name look-up table, so that we
         // will quickly be able to find the route associated with a name and not have
         // to iterate through every route every time we need to find a named route.
-        if ($name = $route->getName()) {
+        if (($name = $route->getName()) && ! $this->inNameLookup($name)) {
             $this->nameList[$name] = $route;
         }
 
@@ -100,7 +100,7 @@ protected function addLookups($route)
         // processing a request and easily generate URLs to the given controllers.
         $action = $route->getAction();
 
-        if (isset($action['controller'])) {
+        if (($controller = $action['controller'] ?? null) && ! $this->inActionLookup($controller)) {
             $this->addToActionList($action, $route);
         }
     }
@@ -117,6 +117,28 @@ protected function addToActionList($action, $route)
         $this->actionList[trim($action['controller'], '\\')] = $route;
     }
 
+    /**
+     * Determine if the given controller is in the action lookup table.
+     *
+     * @param  string  $controller
+     * @return bool
+     */
+    protected function inActionLookup($controller)
+    {
+        return array_key_exists($controller, $this->actionList);
+    }
+
+    /**
+     * Determine if the given name is in the name lookup table.
+     *
+     * @param  string  $name
+     * @return bool
+     */
+    protected function inNameLookup($name)
+    {
+        return array_key_exists($name, $this->nameList);
+    }
+
     /**
      * Refresh the name look-up table.
      *
@@ -129,8 +151,8 @@ public function refreshNameLookups()
         $this->nameList = [];
 
         foreach ($this->allRoutes as $route) {
-            if ($route->getName()) {
-                $this->nameList[$route->getName()] = $route;
+            if (($name = $route->getName()) && ! $this->inNameLookup($name)) {
+                $this->nameList[$name] = $route;
             }
         }
     }
@@ -147,7 +169,7 @@ public function refreshActionLookups()
         $this->actionList = [];
 
         foreach ($this->allRoutes as $route) {
-            if (isset($route->getAction()['controller'])) {
+            if (($controller = $route->getAction()['controller'] ?? null) && ! $this->inActionLookup($controller)) {
                 $this->addToActionList($route->getAction(), $route);
             }
         }

tests/Integration/Routing/CompiledRouteCollectionTest.php

@@ -94,6 +94,38 @@ public function testRouteCollectionCanRetrieveByAction()
         $this->assertSame($action, Arr::except($route->getAction(), 'as'));
     }
 
+    public function testCompiledAndNonCompiledUrlResolutionHasSamePrecedenceForActions()
+    {
+        @unlink(__DIR__.'/Fixtures/cache/routes-v7.php');
+        $this->app->useBootstrapPath(__DIR__.'/Fixtures');
+        $app = (static function () {
+            $refresh = true;
+
+            return require __DIR__.'/Fixtures/app.php';
+        })();
+        $app['router']->get('/foo/{bar}', ['FooController', 'show']);
+        $app['router']->get('/foo/{bar}/{baz}', ['FooController', 'show']);
+        $app['router']->getRoutes()->refreshActionLookups();
+
+        $this->assertSame('foo/{bar}', $app['router']->getRoutes()->getByAction('FooController@show')->uri);
+
+        $this->artisan('route:cache')->assertExitCode(0);
+        require __DIR__.'/Fixtures/cache/routes-v7.php';
+
+        $this->assertSame('foo/{bar}', $app['router']->getRoutes()->getByAction('FooController@show')->uri);
+
+        unlink(__DIR__.'/Fixtures/cache/routes-v7.php');
+    }
+
+    public function testCompiledAndNonCompiledUrlResolutionHasSamePrecedenceForNames()
+    {
+        $this->router->get('/foo/{bar}', ['FooController', 'show'])->name('foo.show');
+        $this->router->get('/foo/{bar}/{baz}', ['FooController', 'show'])->name('foo.show');
+        $this->router->getRoutes()->refreshNameLookups();
+
+        $this->assertSame('foo/{bar}', $this->router->getRoutes()->getByName('foo.show')->uri);
+    }
+
     public function testRouteCollectionCanGetIterator()
     {
         $this->routeCollection->add($this->newRoute('GET', 'foo/index', [

tests/Integration/Routing/Fixtures/app.php

@@ -0,0 +1,18 @@
+<?php
+
+namespace Illuminate\Tests\Integration\Routing\Fixtures;
+
+use Illuminate\Foundation\Application;
+
+if (! class_exists(AppCache::class)) {
+    class AppCache
+    {
+        public static $app;
+    }
+}
+
+if (isset($refresh)) {
+    return AppCache::$app = Application::configure(basePath: __DIR__)->create();
+} else {
+    return AppCache::$app ??= Application::configure(basePath: __DIR__)->create();
+}

tests/Integration/Routing/Fixtures/bootstrap/cache/.gitignore

@@ -0,0 +1,2 @@
+*
+!.gitignore

tests/Integration/Routing/Fixtures/cache/.gitignore

@@ -0,0 +1,2 @@
+*
+!.gitignore

tests/Routing/RouteRegistrarTest.php

@@ -719,6 +719,8 @@ public function testCanSetShallowOptionOnRegisteredResource()
     public function testCanSetScopedOptionOnRegisteredResource()
     {
         $this->router->resource('users.tasks', RouteRegistrarControllerStub::class)->scoped();
+        $this->router->getRoutes()->refreshNameLookups();
+
         $this->assertSame(
             ['user' => null],
             $this->router->getRoutes()->getByName('users.tasks.index')->bindingFields()
@@ -731,6 +733,7 @@ public function testCanSetScopedOptionOnRegisteredResource()
         $this->router->resource('users.tasks', RouteRegistrarControllerStub::class)->scoped([
             'task' => 'slug',
         ]);
+        $this->router->getRoutes()->refreshNameLookups();
         $this->assertSame(
             ['user' => null],
             $this->router->getRoutes()->getByName('users.tasks.index')->bindingFields()
@@ -904,6 +907,7 @@ public function testCanSetMiddlewareForSpecifiedMethodsOnRegisteredResource()
             ->middlewareFor('index', RouteRegistrarMiddlewareStub::class)
             ->middlewareFor(['create', 'store'], 'one')
             ->middlewareFor(['edit'], ['one', 'two']);
+        $this->router->getRoutes()->refreshNameLookups();
 
         $this->assertEquals($this->router->getRoutes()->getByName('users.index')->gatherMiddleware(), ['default', RouteRegistrarMiddlewareStub::class]);
         $this->assertEquals($this->router->getRoutes()->getByName('users.create')->gatherMiddleware(), ['default', 'one']);
@@ -918,6 +922,7 @@ public function testCanSetMiddlewareForSpecifiedMethodsOnRegisteredResource()
             ->middlewareFor(['create', 'store'], 'one')
             ->middlewareFor(['edit'], ['one', 'two'])
             ->middleware('default');
+        $this->router->getRoutes()->refreshNameLookups();
 
         $this->assertEquals($this->router->getRoutes()->getByName('users.index')->gatherMiddleware(), [RouteRegistrarMiddlewareStub::class, 'default']);
         $this->assertEquals($this->router->getRoutes()->getByName('users.create')->gatherMiddleware(), ['one', 'default']);
@@ -1448,6 +1453,7 @@ public function testCanSetMiddlewareForSpecifiedMethodsOnRegisteredSingletonReso
             ->middlewareFor('show', RouteRegistrarMiddlewareStub::class)
             ->middlewareFor(['create', 'store'], 'one')
             ->middlewareFor(['edit'], ['one', 'two']);
+        $this->router->getRoutes()->refreshNameLookups();
 
         $this->assertEquals($this->router->getRoutes()->getByName('users.create')->gatherMiddleware(), ['default', 'one']);
         $this->assertEquals($this->router->getRoutes()->getByName('users.store')->gatherMiddleware(), ['default', 'one']);
@@ -1463,6 +1469,7 @@ public function testCanSetMiddlewareForSpecifiedMethodsOnRegisteredSingletonReso
             ->middlewareFor(['create', 'store'], 'one')
             ->middlewareFor(['edit'], ['one', 'two'])
             ->middleware('default');
+        $this->router->getRoutes()->refreshNameLookups();
 
         $this->assertEquals($this->router->getRoutes()->getByName('users.create')->gatherMiddleware(), ['one', 'default']);
         $this->assertEquals($this->router->getRoutes()->getByName('users.store')->gatherMiddleware(), ['one', 'default']);