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

[5.3] (Revert) Resource Route Names No Longer Affected By Prefixes #15058

Closed
yajra opened this issue Aug 26, 2016 · 22 comments
Closed

[5.3] (Revert) Resource Route Names No Longer Affected By Prefixes #15058

yajra opened this issue Aug 26, 2016 · 22 comments

Comments

@yajra
Copy link
Contributor

yajra commented Aug 26, 2016

Given the changes above, how would you group your resource routes on Laravel 5.3?

For instance, I have an application that have frontend & backend panel with same route resource?
In 5.2:

// Backend post resource
$router->group([
    'middleware' => 'administrator',
    'prefix'     => 'administrator',
], function ($router) {
    $router->resource('posts', PostController::class);
});

// Frontend post resource 
$router->resource('posts', PostController::class);

This will yield a better grouping on resource route name.

administrator.posts.index  => /administrator/posts
administrator.posts.create => /administrator/posts/create
...
posts.index => /posts
posts.create => /posts/create
...

But on 5.3, same route will produce duplicate names.

posts.index  => /administrator/posts
posts.create => /administrator/posts/create
...
posts.index => /posts
posts.create => /posts/create
...

Hence if you use route() helper then your upgrade path to 5.3 is doomed!

I know that we can fix it by manually naming the routes on administrator side (which is what I did on my project) but considering a big project, IMHO, too much code duplications! Imagine that we have to manually write all our administrator resource routes?

$router->get('posts', PostsController::class . '@index')->name('administrator.posts.index');
$router->get('posts/create', PostsController::class . '@create')->name('administrator.posts.create');
$router->post('posts', PostsController::class . '@store')->name('administrator.posts.store');
$router->get('posts/{posts}/edit', PostsController::class . '@edit')->name('administrator.posts.edit');
$router->put('posts/{posts}', PostsController::class . '@update')->name('administrator.posts.update');
$router->delete('posts/{posts}', PostsController::class . '@destroy')->name('administrator.posts.destroy');

I hope this can be reconsidered. Or if there is better way that I am missing, please enlighten me. Thanks!

@GrahamCampbell
Copy link
Member

I think this was intended. Feel free to ask around on internals, or search for other issues.

@yajra
Copy link
Contributor Author

yajra commented Aug 26, 2016

@GrahamCampbell yes it was intended as documented. Submitting on internals to get what other thinks about it. Thanks!

@lchogan
Copy link

lchogan commented Aug 28, 2016

Same issue here. I have a superadmin prefix for many of my resources and the same resources without the superadmin. Now that I upgraded to 5.3 I have to write out each superadmin route. This completely negates the advantages of the route resources IMO. Thanks.

@yajra
Copy link
Contributor Author

yajra commented Aug 30, 2016

@GrahamCampbell maybe we can re-open this issue so that others may see and get a feedback from them like the issue thread on #15072?

Got 3 thumbs up on Internal issue + @lchogan so maybe this is worth for reconsideration? Thanks!

@crynobone
Copy link
Member

@yajra why should resource route name be affected by prefix?

$router->group([
    'middleware' => 'administrator',
    'prefix'     => 'admin',
], function ($router) {
    $router->resource('posts', PostController::class);
});

What happen when you need to change the prefix admin to domain admin.laravel.com as example? You're route name would be doom as well. With what we doing it in Laravel 5.3, such changes doesn't break your route name.

@yajra
Copy link
Contributor Author

yajra commented Aug 30, 2016

@crynobone changing from prefix to domain use case makes perfect sense. Didn't think of that.

@RDelorier suggested a workaround to use as laravel/ideas#181 and would give it a shot.

Route::group([
    'as' => 'admin.', // the trailing dot makes me think this is unintended
    'prefix' => 'admin'
], function () {
    Route::resource('users', 'UserController');
});

Route::resource('users', 'UserController');

Thanks for the feedbacks!

@lorvent
Copy link

lorvent commented Sep 9, 2016

Definitely a breaking change (many breaking ones like this makes 5.3 eligible for 6.0)
but moving forward, if this is the only way....we have no choice than adopt to it.

@fernandobandeira
Copy link
Contributor

This changed because before when using as and prefix in the same route would append both of them to your route name, so the prefix appending dropped in place of the as.

@lorvent
Copy link

lorvent commented Sep 9, 2016

but adding .(dot) i.e. 'as' => 'admin.' looks weired

and i guess people were using only prefix before as it will do both things.

@fernandobandeira
Copy link
Contributor

fernandobandeira commented Sep 9, 2016

People were using both actually, there's a lot of issues related to this previous behavior, you can check some of them:

#12216 , #13903

#14547 , #14833

So basically prefix will affect the generation of your url meanwhile as will affect the name.

@Skintillion
Copy link

but adding .(dot) i.e. 'as' => 'admin.' looks weird

I agree, however it gives some the opportunity to use _ instead I guess. Personally I would prefer the decimal included automatically.

@bjvelarde
Copy link

Route::group(['prefix' => 'admin', 'as' => 'admin.'], function() { Auth::routes(); });

will give us:
POST | admin/login | admin. |
POST | admin/password/email | admin. |
GET|HEAD | admin/password/reset | admin. |
POST | admin/password/reset | admin. |
GET|HEAD | admin/password/reset/{token} | admin. |

etc.... How do we fix this?

@Skintillion
Copy link

Fix what?

@bjvelarde
Copy link

Various auth routes having the same route name

@Skintillion
Copy link

Don't do that with auth routes.

@bjvelarde
Copy link

So how do I do it if I want the following routes: admin.login, admin.logout, admin.resetpassword...etc?

@Skintillion
Copy link

Skintillion commented Dec 3, 2016

I'm confused as to why you would need that anyways? Erm... I guess maybe you are overriding an API and Web route, thereby creating the dupe route.

(api and web both changed to admin)

@bjvelarde
Copy link

Because the project I'm working on has the authentication on the admin only and I want my routes organized that way so that I know which part of the application the routes are for. I'm just concerned if having those auth related routes having the same 'admin.' name will not have unpredictable effects on the whole auth process...

@Skintillion
Copy link

Auth Routes command is here:

https://github.com/laravel/framework/blob/5.3/src/Illuminate/Support/Facades/Auth.php#L30

Which runs:

https://github.com/laravel/framework/blob/5.3/src/Illuminate/Routing/Router.php#L287

You could remake them yourself if you like and not use Auth::routes()

@bjvelarde
Copy link

thanks

@Skintillion
Copy link

You're welcome!

@sohyl87
Copy link

sohyl87 commented Dec 21, 2016

This worked for me :

// Backend post resource
$router->group(['middleware' => 'administrator', 'prefix' => 'administrator'], function ($router) {
$router->resource('posts', 'PostController', ['as' => 'administrator']);
});

// Frontend post resource
$router->resource('posts', 'PostController');

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

No branches or pull requests

9 participants