-
Notifications
You must be signed in to change notification settings - Fork 916
[4.0][Feature][Ready] Routes are defined in each Operation #1937
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
Conversation
[ci skip] [skip ci]
…ckpack/CRUD into routes-in-controllers
@tabacitu I looked into it and think this is kind of awesome. Some points to mentionRoute names will be nasty if we have nested crudshere we should try to parse the What are the options for?shouldn't the provided options be used when creating the routes in the traits? Why don't we use more laravelly method namesIn Laravel we often use "magic method" naming like
In my opinion this would be more consistent with Laravel code if we use
How do we handle bulk-delete?I don't think registering the bulk delete route is placed correctly in the DeleteOperation. If we combine them in on single trait, we should think about enabling bulk actions via static variable instead of in the setup method... many thoughts, but just high level design issues I think :) |
Thanks a lot for the feedback @OliverZiegler !
Totally agree. Your syntax is indeed more laravelly, let's use
Indeed, BulkDelete should probably be a separate operation -
Hmm... Yes, I thought we would use On the other hand:
But... should we? I don't think so. Why declare a property that passes things to routes, that then call methods, when you could just overwrite the method itself? Wouldn't that be both easier to understand, and easier to do? There's also the option of declaring the controller middleware in the
Ugh - I did not think specifically about nested CRUDs. I've made this mistake in v3, not going to make it again, thank you. I've added a todo below to make sure I check nested route names too. Todo
|
Fixed most of the above. Note:
Also, @OliverZiegler & @tswonke , what do you think about removing the FormRequest validation through type-hinting, so that when we generate EntityCrudController, it has no operation methods there? Now that operations are stand-alone traits, it seems messy that some actions are in the generated EntityCrudController, and some are not. |
Since we are still able to override these methods if necessary I think cleaning up the EntityCrudController is a nice goal! I will think about it - not that we miss any downsides :-) |
@OliverZiegler - you were totally right about Nested CRUDs. There were routes with the same name. For example:
I just pushed a fix for this, so that:
To achieve what you wanted (clean route names for nested CRUDs) you just have to specify a name in the nested controller's route group. // routes/backpack/custom.php
Route::group([
'prefix' => config('backpack.base.route_prefix', 'admin'),
'middleware' => ['web', config('backpack.base.middleware_key', 'admin')],
'namespace' => 'App\Http\Controllers\Admin',
'name' => 'crud.',
], function () {
Route::crud('user', 'UserCrudController');
Route::crud('post', 'PostCrudController');
Route::group([
'prefix' => 'user/{user_id}',
'name' => 'user.',
], function() {
Route::crud('post', 'UserPostCrudController');
});
}); // this should be the absolute last line of this file This will lead to:
I don't love the fact that |
Merged into the Thanks a lot for contributing on this @OliverZiegler & @tswonke ! |
I am not sure if the page https://backpackforlaravel.com/docs/6.x/crud-how-to#how-to-add-extra-crud-routes-1 has the right documentation for the feature. Because I see that in the backpack code it is used the syntax It is not clear a way to add a custom page for crud for a model and how to register the new route in the documentation, maybe in this 5 years something is changed? |
so the page https://backpackforlaravel.com/docs/6.x/crud-operations#operation-routes-1 explain better compared to https://backpackforlaravel.com/docs/6.x/crud-how-to#how-to-add-extra-crud-routes-1 |
Thanks for the heads up @Mte90 and for providing the links. I've just pointed the "less complete docs" to the "more complete docs". 👍 Laravel-Backpack/docs#609 |
This PR fixes #1877 using Solution 5 (proposed by @tswonke ). It moves where the routes are defined for each operation. Previously (in v3) routes were registered in a separate class (CrudRouter), and called from the CrudServiceProvider using the CRUD alias. This made it difficult to add routes for new operations - they needed to be added in the route file.
Now, routes are registered inside operations (so inside the CrudController). Each operation has a
setupRoutesForXXX()
method which loads the routes for that operation. This way:In addition, CrudController now does NOT load any operations by default. The operations will be loaded in each EntityCrudController, by specifying the operation trait. Generators will of course default to the same set of operations, but developers will be able to easily remove an operation from a CRUD just by removing its
use
statement. Thanks to the above, not only will the operation be disabled, but the route will no longer be registered either. This should make for a cleaner output when usingphp artisan route:list
.How to add a new operation with routes
Of course, if you reuse this operation, you can just create a
trait ModerateOperation { /* ... */ }
with it, and use that trait in multiple EntityCrudControllers.How to remove a default operation
Breaking changes
Routes
Developers should change all their routes as shown below. All Backpack packages should also do this. Generators should also use this new (and only) syntax:
CrudControllers
Step 1. Make sure your
$crud
object is configured inside thesetup()
method, not ``__construct(), especially if you've generated your cruds using old versions of Backpack v3 (from 2016-2017). In most cases you can just rename
__construct()``` to ```setup()```, since ```setup()``` is called inside ```__construct()``` anyway.Step 2. ZERO operations and routes are loaded by default by CrudController. Every EntityCrudController needs the following changes to be able to have all operations that were previously loaded by default:
Step 3. This is a great time to think about which operations you're NOT using with each CrudController. If you're actually not using an operation, just commend or delete the
use
statement for that operation. This will also prevent the routes for that operation from being registered, so yourphp artisan route:list
will be cleaner.Step 4. Additionally in CrudControllers, the actions operations for create() and update() have been renamed. Please make the following change in your EntityCrudControllers:
Notice you're now calling
$this
notparent
. That's because there are no parent methods now. All operation are loaded as traits directly on your EntityCrudController, nothing on the CrudController it extends.Step 5. If you're calling a method using
parent::action()
anywhere inside your EntityCrudController, it will not work anymore. To be able to call the same method, but from the trait (not the parent), please rename the method from the trait, and call that name instead:This is inconvenient, but I think it's a small price to pay for having the convenience of:
You do NOT need to do this for everything the methods in your EntityCrudController that you've overwritten completely. Only for the methods that have a
parent::smth()
call inside them.Thoughts? I think we've got a winner :-)