Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.x] Intelligently drop unnamed prefix name routes when caching #31917

Merged
merged 1 commit into from Mar 12, 2020

Conversation

@taylorotwell
Copy link
Member

taylorotwell commented Mar 11, 2020

In Laravel 6.x, when using route name groups, any routes not given a more an explicit name would be given the group name. For example:

Route::name('admin.')->group(function () {
    Route::get('/dashboard');
    Route::get('/users');
});

Both of the routes above would be receive the route name admin.. This, of course, is odd behavior but was not particularly problematic until Laravel 7.x route caching was introduced.

Laravel 7.x uses Symfony's very fast route matcher, but that route matcher requires every route to have a unique name. This has introduced a significant barrier to upgrade to Laravel 7.x because people are now forced to remove the route name prefix from the group or literally assign a route name to every single route within the group explicitly.

In my opinion, when caching, the framework should assign a generated name to a route if the route's name ends in a . and it will create a duplicate route name error. When we encounter routes like these, I think it is safe to make the assumption that the developer did not actually intend for them to have a name at all and likely have been unaware they were receiving a name in previous Laravel releases. In fact, this caused a problem with Laravel Nova itself that even I did not expect.

This will significantly ease the burden of upgrading to Laravel 7.x when using route name groups with no breaking changes since we are only changing behavior in a situation where the code was about to throw an exception anyways.

@GrahamCampbell GrahamCampbell changed the title Intelligently drop unnamed prefix name routes when caching [7.x] Intelligently drop unnamed prefix name routes when caching Mar 11, 2020
@browner12

This comment has been minimized.

Copy link
Contributor

browner12 commented Mar 11, 2020

I know it's been a bit of a convention to use a . to separate names, but do we need to account for any other separators?

@taylorotwell

This comment has been minimized.

Copy link
Member Author

taylorotwell commented Mar 11, 2020

We can evaluate that as needed but this is the way route group name prefixes are documented so this feels like the safest place to start.

@georgeboot

This comment has been minimized.

Copy link

georgeboot commented Mar 11, 2020

I think it is safe to make the assumption that the developer did not actually intend for them to have a name at all and likely have been unaware they were receiving a name in previous Laravel releases.

I’m not too certain about that. I’ve used similarly named routes for both get and post routes (eg, for a form show and save method) in some projects I recently upgraded. This was fully intentional.

@taylorotwell

This comment has been minimized.

Copy link
Member Author

taylorotwell commented Mar 11, 2020

@georgeboot you have intentionally named routes with a trailing dot and duplicate names? And actually referred to them using the route and action helpers with trailing dots in the names? If what you say is true, you would still have not even had a way to access them using the route and action helpers because the names would have been duplicated and it would have just returned the first matching route every time.

@georgeboot

This comment has been minimized.

Copy link

georgeboot commented Mar 11, 2020

Sorry no, I meant, I have 2 routes both called profile. One is GET, one is POST.

I generate urls to when with route() helper.

I know this isn’t directly linked to nested routes, but this behaviour worked in Laravel 6. I noticed it when upgrading and running route:cache.

What I’m trying to say is that non-fully unique route names aren’t always developer intended

@taylorotwell

This comment has been minimized.

Copy link
Member Author

taylorotwell commented Mar 11, 2020

That situation is not the situation that this PR is referring to. This PR is ONLY dealing with duplicate route names that end with a trailing ..

@georgeboot

This comment has been minimized.

Copy link

georgeboot commented Mar 11, 2020

Oké sorry, ignore my comments in that case.

@LionsAd

This comment has been minimized.

Copy link

LionsAd commented Mar 11, 2020

Wouldn’t it be safer to just check for duplicate route names and auto-increase them instead, eg.

admin.auto_generated_1, admin.auto_generated_2

etc?

@rodrigopedra

This comment has been minimized.

Copy link
Contributor

rodrigopedra commented Mar 11, 2020

I never relied on routes ending with a ., but I can imagine someone checking for a route prefix with request()->routeIs(...) (sorry if it is not the correct method, I'm on mobile). Checking for routes prefix is something I already saw/did in some projects.

Maybe @LionsAd suggestion on appending a suffix instead of replacing the route's name can prevent some hiccups when upgrading an app which can be using that feature to check for routes' prefix.

@taylorotwell

This comment has been minimized.

Copy link
Member Author

taylorotwell commented Mar 12, 2020

@rodrigopedra that would still work. We just put a generated random name after the prefix.

@taylorotwell taylorotwell merged commit d58f9e2 into 7.x Mar 12, 2020
14 checks passed
14 checks passed
P7.2 - Sprefer-lowest
Details
P7.2 - Sprefer-lowest
Details
P7.2 - Sprefer-stable
Details
P7.2 - Sprefer-stable
Details
P7.3 - Sprefer-lowest
Details
P7.3 - Sprefer-lowest
Details
P7.3 - Sprefer-stable
Details
P7.3 - Sprefer-stable
Details
P7.4 - Sprefer-lowest
Details
P7.4 - Sprefer-lowest
Details
P7.4 - Sprefer-stable
Details
P7.4 - Sprefer-stable
Details
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/styleci/push The analysis has passed
Details
@rodrigopedra

This comment has been minimized.

Copy link
Contributor

rodrigopedra commented Mar 12, 2020

Ok, thanks for the heads up!

@GrahamCampbell GrahamCampbell deleted the route-name-cache branch Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.