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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Adds support for specifying a route group controller #40276

Merged
merged 7 commits into from
Jan 13, 2022
Merged

[8.x] Adds support for specifying a route group controller #40276

merged 7 commits into from
Jan 13, 2022

Conversation

lukeraymonddowning
Copy link
Contributor

@lukeraymonddowning lukeraymonddowning commented Jan 6, 2022

Howdy!

Motivation

Whilst recently working on an application with a lot of routes, I came across a situation I thought could be solved quite easily. The application has many routes that sit in groups. All routes in each group reference a different method on the same controller.

Route::prefix('placements')->as('placements.')->group(function () {
    Route::get('', [PlacementController::class, 'index'])->name('index');
    Route::get('/bills', [PlacementController::class, 'bills'])->name('bills');
    Route::get('/bills/{bill}/invoice/pdf', [PlacementController::class, 'invoice'])->name('pdf.invoice');
});

You can see here that not all methods fit in directly with CRUD. It is common that there is also a forceDelete and download method in these groups, along with other items.

Solution

I thought it would be much cleaner to be able to reference the controller just once for each group rather than repeating the same controller class over and over. With this PR, the following syntax is now possible.

 Route::controller(PlacementController::class)->prefix('placements')->as('placements.')->group(function () {
    Route::get('', 'index')->name('index');
    Route::get('/bills', 'bills')->name('bills');
    Route::get('/bills/{bill}/invoice/pdf', 'invoice')->name('pdf.invoice');
});

I think this removes a lot of visual load from a DX perspective. It also provides a nice location to group all controller methods together in route files when not using resource or apiResource.

Considerations

This can be thought of a fallback; if the user specifies @, array or closure or invokable syntax, the router will override the group controller attribute. None of the following examples will use the PlacementController.

 Route::controller(PlacementController::class)->prefix('placements')->as('placements.')->group(function () {
    // Overridden by @ syntax
    Route::get('', 'FooController@index')->name('index');

    // Overridden by array syntax
    Route::get('/bills', [FooController::class, 'bills'])->name('bills');

    // Overridden by closure syntax
    Route::get('/bills/{bill}/invoice/pdf', fn () => 'response')->name('pdf.invoice');

    // Overridden by invokable controller syntax
    Route::get('/bills/{bill}/invoice/pdf', FooController::class)->name('pdf.invoice');
});

For more thoughts on this, check out the comments on this tweet I put out: https://twitter.com/LukeDowning19/status/1479032102872096771?s=20

Thanks for all the hard work as always. What an awesome community to be a part of 馃コ

Kind Regards,
Luke

@driesvints
Copy link
Member

driesvints commented Jan 6, 2022

Did you test this with route caching enabled?

@lukeraymonddowning
Copy link
Contributor Author

@driesvints yup 馃憤 All worked as expected

Here's output of php artisan route:list too. The PostController here uses the new groups. All pages work as expected with and without route caching.

CleanShot 2022-01-06 at 13 09 40@2x

@mateusjunges
Copy link
Contributor

mateusjunges commented Jan 6, 2022

I think ->controller already provides enough context to group, so why not just pass the callback in the controller method instead of chaining another call to group?

Something like this:

Route::controller(PlacementController::class, function () {
    Route::get('/', 'index')->name('index');
    Route::get('/bills', 'bills')->name('bills');
    Route::get('/bills/{bill}/invoice/pdf', 'invoice')->name('pdf.invoice');
});

@driesvints
Copy link
Member

@mateusjunges I think because of the same reason why we don't do that in other methods: consistency. Otherwise any method on the router could have a second param as the callback with routes and that would mix things up a bit too much.

@lukeraymonddowning
Copy link
Contributor Author

Agreed with @driesvints. I think consistency is king here.

@koenhoeijmakers
Copy link
Contributor

Given that you've tested all other variations of route registry (@, array or closure syntax), maybe also confirm that an invokable controller also still works? (Route::get('test', Controller::class);)

@lukeraymonddowning
Copy link
Contributor Author

@koenhoeijmakers good call. I've added a supporting test. It actually wasn't supported before but is now 馃憤

@taylorotwell taylorotwell merged commit 25aa82e into laravel:8.x Jan 13, 2022
@rcerljenko
Copy link

hi @lukeraymonddowning , first of all thanks for this! one thing, could you add a method signature to a Route facade file? phpstan is complaining for unkown static method controller

@Oluwa-Pelumi
Copy link

Oluwa-Pelumi commented Jan 22, 2022

Using this breaks; if you're still using Route::group(['middleware' => ['auth:sanctum', 'session.expired', 'install'], 'prefix' => 'account'], function () {}); changing to this fixes
Route::controller(AdminDashboardController::class) ->middleware(['auth:sanctum', 'session.expired', 'verified', 'install']) ->prefix('admin') ->group(function () {});
To anyone facing the same problem.

@lukeraymonddowning
Copy link
Contributor Author

@Oluwa-Pelumi you don't need to use different syntax, just use controller as an array key rather than a method call. It works exactly the same as the other group methods 馃憤

@geisi
Copy link
Contributor

geisi commented Jan 22, 2022

I think I have found a bug concerning this new awesome feature. It seems the implementation has a problem with namespaced group routes.

This works

Route::controller(FooController::class)
    ->group(function () {
        Route::get('/foo', 'foo');
    });

This also works

Route::namespace('App\Http\Controllers')
    ->group(function () {
        Route::get('/with_namespace/bar', [FooController::class, 'bar']);
    });

This throws an exception Target class [App\Http\Controllers\App\Http\Controllers\FooController] does not exist.

Route::namespace('App\Http\Controllers')
    ->controller(FooController::class)
    ->group(function () {
        Route::get('/with_namespace/foo', 'foo');
    });

I have created an example Laravel repository with a test to reproduce the bug.

@igorgaming
Copy link

igorgaming commented Jan 23, 2022

I think I have found a bug concerning this new awesome feature. It seems the implementation has a problem with namespaced group routes.

This works

Route::controller(FooController::class)
    ->group(function () {
        Route::get('/foo', 'foo');
    });

This also works

Route::namespace('App\Http\Controllers')
    ->group(function () {
        Route::get('/with_namespace/bar', [FooController::class, 'bar']);
    });

This throws an exception Target class [App\Http\Controllers\App\Http\Controllers\FooController] does not exist.

Route::namespace('App\Http\Controllers')
    ->controller(FooController::class)
    ->group(function () {
        Route::get('/with_namespace/foo', 'foo');
    });

I have created an example Laravel repository with a test to reproduce the bug.

This is not a bug, because ->controller(FooController::class) already has namespace (because you use use ...\FooController; at top of the file). Try something like that:

Route::namespace('App\Http\Controllers')
    ->controller('FooController')
    ->group(function () {
        Route::get('/with_namespace/foo', 'foo');
    });

P.s: but why do you need to use ->namespace() together with ->controller()?

@geisi
Copy link
Contributor

geisi commented Jan 23, 2022

This is not a bug, because ->controller(FooController::class) already has namespace (because you use use ...\FooController; at top of the file).

This answer makes sense when you ignore the fact that the original namespace implementation without group controllers. Already only applies the namespace to routes which dont reference a full class namespace controller action.

This behaviour is missing in the route group controller implementation so indeed I think this is a bug. Please look at my tests.

And yeah I know this is a really special use case but this error came up in the Laravel Jetstream code base so it seems not that uncommon.

@taylorotwell
Copy link
Member

taylorotwell commented Jan 24, 2022

It doesn't look like that much of a bug to me. You specified a namespace that all controllers should be prefixed with. Then you provided a controller class name as the namespace you specified was prefixed.

@kkbt
Copy link

kkbt commented Feb 6, 2022

I would like to add a 'where' condition like this:

Route::controller(SomeController::class)->group(function () {
    Route::get('{id}', 'view');
    Route::get('/online/{id}', 'online');
})->where('id', '[a-z]{20}');

Is this possible?
Currently I receive the following error: Call to a member function where() on null

@lukeraymonddowning
Copy link
Contributor Author

@kkbt please see the docs on route groups in general: https://laravel.com/docs/8.x/routing#route-groups

The answer to your question however is no, because route groups don't support the where method at the group level. It would have to be placed on a specific route.

@kkbt
Copy link

kkbt commented Feb 6, 2022

route groups don't support the where method at the group level

And it is not possible to implement this? It would be very elegant feature.

@dennisprudlo
Copy link
Contributor

@kkbt Skimming over the PR it seems that the new controller function is just another attribute to the RouteRegistrar. So you should be able to append your condition there instead after the group-function (which is in fact a method of the Router and does not return anything, so appending ->where(...) to the group function was never possible.

Have you tried this (Note the difference in the where function, we are passing an array here instead of two parameters)?

Route::controller(FooController::class)->where([ 'id' => '[a-z]{20}' ])->group(function () {
    Route::get('{id}', 'view');
    Route::get('/online/{id}', 'online');
});

@kkbt
Copy link

kkbt commented Feb 6, 2022

@dennisprudlo It works! Excellent. Thank you very much!

@Tadaz
Copy link

Tadaz commented Mar 7, 2022

Superb future! Thank you!

A small observation: it does not work with invokable controllers though.

Both of the routes below should call SPAController@__invoke. Unfortunately, in this example, it does not work.

Route::controller(SPAController::class)->group(function () {
    Route::get('preview')->middleware('signed');
    Route::any('/{any}')->where('any', '.*');
});

Currently working workaround:

Route::controller(SPAController::class)->group(function () {
    Route::get('preview', '__invoke')->middleware('signed');
    Route::any('/{any}', '__invoke')->where('any', '.*');
});

IMO, it should call the __invoke method automatically - without a need to specify it.

@lukeraymonddowning lukeraymonddowning deleted the controller_group_support branch April 17, 2023 11:26
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.

None yet