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

[8.x] Don't name unnamed routes inside a group with a prefix #39525

Closed
wants to merge 1 commit into from

Conversation

tobyzerner
Copy link
Contributor

@tobyzerner tobyzerner commented Nov 8, 2021

Currently if you register an unnamed route within a group with a name prefix, the route's name is set to the prefix.

$router->name('api.')->group(function ($router) {
    $router->get('users', 'UsersController@index');
});

$routes = $router->getRoutes()->get();
echo last($routes)->getName(); // "api."

While the consequences of this are somewhat obscure (but definitely exist), it is indeed odd behavior and I would expect getName() to return null in the above example since I never explicitly set a name on the route.

This PR attempts to fix that within the constraints of the current architecture. I'm not sure if this would be considered a breaking change vs a bug fix, but I can PR this instead to master if desired.

Basically, instead of passing the group's $action['as'] value into each new route created within it, it passes it as $action['as_prefix']. Then when a name is set on a route, it prepends $action['as_prefix'].

This also changes the behavior of calling name more than once on a route. Previously each time you called it, the value passed would be continually appended:

$router->name('api.')->group(function ($router) {
    $router->get('users', 'UsersController@index')->name('foo')->name('bar') // "api.foobar"
});

Now, the value will be appended onto the initial group prefix each time:

$router->name('api.')->group(function ($router) {
    $router->get('users', 'UsersController@index')->name('foo')->name('bar') // "api.bar"
});

@devcircus
Copy link
Contributor

Seems like a common sense change but worried it would be a breaking change for a lot of apps. I've see a number of apps with route "groups" for individual routes and using the group name for the route. I know I've ended up with one route in a group after moving other routes out of the group and never refactoring. IMO this should go into the next version with a note in the upgrade guide.

@driesvints
Copy link
Member

This is a huge breaking change.

@tobyzerner
Copy link
Contributor Author

@taylorotwell would you accept this for master/9.x? Or not interested?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants