Ensure cached and uncached routes share same precedence when resolving actions and names

[timacdonald]

This PR addresses a bug where routes precedence is different than expected when matching actions and names compared to matching URLs.

Also, there is an inconsistency between action matching behaviour with cached and un-cached routes. This is fun because we didn't know about the issue until it was deployed (not in production) where the routes were cached.

---

Given the following two routes:

Route::get('base/{first}/{param?}', [MyController::class, 'show']);
Route::get('base/{second}', [MyController::class, 'show']);

Although both of the routes can respond to the following URL, the first route defined takes precedence:

/base/foo

Precedence is basically: first route to match handles the request.

When it comes to generating URLs for routes, we have some inconsistencies with this behaviour that is inconsistent between cached and uncached routes.

Artisan::call('route:clear');

url()->action([MyController::class, 'show'], ['laravel', 'framework']);
> "/base/laravel?framework"

Artisan::call('route:cache');

url()->action([MyController::class, 'show'], ['laravel', 'framework']);
> "/base/laravel/framework"

Notice when the routes were not cached, the second route was used and a query string is present. When the routes are cached, the first route is used and the second parameter becomes a path segment.

When the routes are cached, I feel the expected behaviour occurs. The CompiledRouteCollection will match against the first route with the same action.

framework/src/Illuminate/Routing/CompiledRouteCollection.php

Lines 210 to 216 in 5d773d2

	$attributes = (new Collection($this->attributes))->first(function (array $attributes) use ($action) {
	if (isset($attributes['action']['controller'])) {
	return trim($attributes['action']['controller'], '\\') === $action;
	}
	return $attributes['action']['uses'] === $action;
	});

I feel that the un-compiled RouteCollection class exhibits the wrong behaviour, and will match on the last route with the same action. The cause of this is the way the RouteCollection maintains a key -> value pair of action => route.

framework/src/Illuminate/Routing/RouteCollection.php

Lines 103 to 106 in 5d773d2

	protected function addToActionList($action, $route)
	{
	$this->actionList[trim($action['controller'], '\\')] = $route;
	}

This is called whenever we add a new route:

Route::get('...', [MyController::class, 'show']); 
// $this->actionList['MyControlle@show'] = $route;

Route::get('...', [MyController::class, 'show']);
// $this->actionList['MyControlle@show'] = $route;

We also override everything again when the action lookup is refreshed:

framework/src/Illuminate/Routing/RouteCollection.php

Lines 133 to 142 in 5d773d2

	public function refreshActionLookups()
	{
	$this->actionList = [];
	foreach ($this->allRoutes as $route) {
	if (isset($route->getAction()['controller'])) {
	$this->addToActionList($route->getAction(), $route);
	}
	}
	}

The same precedence issue exists for named routes, where the last one is always matched. This doesn't have the difference between un-cached and cached routes, because you cannot serialize a routes if you have a route name conflict.

That being said, I still feel the precedence for un-cached routes should be addressed.

[github-actions]

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

[timacdonald]

This is a little weird; I know.

When routes are cached, a new application instance is resolved.

This file allows us to capture the app instance created to add routes to it.