Skip to content

[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

Merged
merged 15 commits into from
Aug 29, 2019

Conversation

tabacitu
Copy link
Member

@tabacitu tabacitu commented Jul 15, 2019

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:

  • operations are completely independent, even if they're traits;
  • you can easily overwrite the routes for one operation;
  • you can easily add routes for custom operations;
  • you can easily develop or use packages that introduce new operations;
  • you can easily create and AJAX fields (that also need a route and controller method) - just create a Backpack operation for it, and use it on all CrudControllers where you need that field;

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 using php artisan route:list.

How to add a new operation with routes

class ProductCrudController extends CrudController {

    protected function setupRoutesForModerate($name, $controller, $options) {    
        Route::get($name.'/{id}/moderate', [
            'as' => 'crud.'.$name.'.moderate',
            'uses' => $controller.'@moderate',
        ]);
    }

   public function moderate() { dd('moderate'); }

}

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

<?php

namespace App\Http\Controllers\Admin;

// VALIDATION: change the requests to match your own file names if you need form validation
use App\Http\Requests\MonsterRequest as StoreRequest;
use App\Http\Requests\MonsterRequest as UpdateRequest;
use Backpack\CRUD\app\Http\Controllers\CrudController;
use Backpack\CRUD\app\Http\Controllers\Operations\SaveActions;
use Backpack\CRUD\app\Http\Controllers\Operations\ListOperation;
use Backpack\CRUD\app\Http\Controllers\Operations\ShowOperation;
use Backpack\CRUD\app\Http\Controllers\Operations\CloneOperation;
- use Backpack\CRUD\app\Http\Controllers\Operations\CreateOperation;
use Backpack\CRUD\app\Http\Controllers\Operations\DeleteOperation;
use Backpack\CRUD\app\Http\Controllers\Operations\UpdateOperation;
use Backpack\CRUD\app\Http\Controllers\Operations\ReorderOperation;
use Backpack\CRUD\app\Http\Controllers\Operations\RevisionsOperation;


class MonsterCrudController extends CrudController
{
-    use CreateOperation, CloneOperation, DeleteOperation, ListOperation, ReorderOperation, RevisionsOperation, SaveActions, ShowOperation, UpdateOperation;
+    use CloneOperation, DeleteOperation, ListOperation, ReorderOperation, RevisionsOperation, SaveActions, ShowOperation, UpdateOperation;

    public function setup()
    {        
        /*
        |--------------------------------------------------------------------------
        | BASIC CRUD INFORMATION
        |--------------------------------------------------------------------------
        */
        $this->crud->setModel('App\Models\Monster');
        $this->crud->setRoute(config('backpack.base.route_prefix').'/monster');
        $this->crud->setEntityNameStrings('monster', 'monsters');

        /*
        |--------------------------------------------------------------------------
        | BASIC CRUD INFORMATION
        |--------------------------------------------------------------------------
        */

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:

Route::group([
    'prefix' => config('backpack.base.route_prefix'),
    'middleware' => ['web', 'admin'],
    'namespace' => 'Backpack\MenuCRUD\app\Http\Controllers\Admin',
], function () {
-    CRUD::resource('menu-item', 'MenuItemCrudController');
+    Route::crud( 'menu-item', 'MenuItemCrudController');
});

CrudControllers

Step 1. Make sure your $crud object is configured inside the setup() 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:

class SettingCrudController extends CrudController
{
+    use \Backpack\CRUD\app\Http\Controllers\Operations\ListOperation;
+    use \Backpack\CRUD\app\Http\Controllers\Operations\CreateOperation;
+    use \Backpack\CRUD\app\Http\Controllers\Operations\UpdateOperation;
+    use \Backpack\CRUD\app\Http\Controllers\Operations\DeleteOperation;
+    use \Backpack\CRUD\app\Http\Controllers\Operations\SaveActions;

    public function setup()
    {
        // ...

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 your php 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:

    public function store(StoreRequest $request)
    {
-        return parent::storeCrud($request);
+        return $this->storeEntry($request);
    }

    public function update(UpdateRequest $request)
    {
-        return parent::updateCrud($request);
+        return $this->updateEntry($request);
    }

Notice you're now calling $this not parent. 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:

use Backpack\CRUD\app\Http\Controllers\Operations\ShowOperation;

class MonsterCrudController extends CrudController
{
-    use ShowOperation;
+    use ShowOperation {
+          show as traitShow;
+       }

    public function show()
    {
        // do sth custom here
-        return parent::show();
+        return $this->traitShow();
    }

This is inconvenient, but I think it's a small price to pay for having the convenience of:

  • routes only being loaded for each operation
  • easily using operations with routes
  • easily using fields with operations with routes

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 :-)

@tabacitu tabacitu self-assigned this Jul 15, 2019
@tabacitu tabacitu requested a review from tswonke July 15, 2019 13:40
@tabacitu tabacitu changed the title [4.0][Feature][Ready] Routes are defined in the controller [4.0][Feature][Ready] Routes are defined in each Operation Jul 15, 2019
@tswonke
Copy link
Contributor

tswonke commented Jul 16, 2019

@OliverZiegler

@tswonke tswonke requested a review from OliverZiegler July 16, 2019 13:04
@OliverZiegler
Copy link
Contributor

@tabacitu I looked into it and think this is kind of awesome.

Some points to mention

Route names will be nasty if we have nested cruds

here we should try to parse the $name parameter
Example
Say we have a nested crud user/{id}/posts
this could lead to a route name: crud.user.posts.xxx
so we need user/{id}/posts to be parsed to user.posts as the identifying route name.

What are the options for?

shouldn't the provided options be used when creating the routes in the traits?
like the default Laravel route options, applying middleware, perhaps enabling route name (better: route-prefix) overrides?

Why don't we use more laravelly method names

In Laravel we often use "magic method" naming like

  • accessors getABCAttribute
  • mutators setABCAttribute
  • bootable trait bootABCTrait

In my opinion this would be more consistent with Laravel code if we use setupABCRoutes like

  • setupShowRoutes for method show and route `{id}/show
  • setupBulkCloneRoutes for method bulkClone and route bulk-clone (just an example)

How do we handle bulk-delete?

I don't think registering the bulk delete route is placed correctly in the DeleteOperation.
I think when we provide the flexibility not to register unused routes, we also should not combine routes in one trait that do not necessarily belong to each other.
Yes both do the delete but bulk delete and single instance delete are two different things.

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 :)

@tabacitu
Copy link
Member Author

tabacitu commented Jul 22, 2019

Thanks a lot for the feedback @OliverZiegler !

Why don't we use more laravelly method names

Totally agree. Your syntax is indeed more laravelly, let's use setupCreateRoutes() instead of setRoutesForCreate().

How do we handle bulk-delete?

Indeed, BulkDelete should probably be a separate operation - BulkDeleteOperation, not everybody uses it. Same with BulkClone.

What are the options for?

Hmm... Yes, I thought we would use $options the same way Laravel does for Route::resource(), but now that I think of it, we no longer use Route::resource() anywhere, so $options never gets passed to anything. We could totally remove it.

On the other hand:

  • we could replicate the Route::resource() functionality, by declaring a routeOptions property on the CrudController, which gets used in all route setup methods; and in that routeOptions array you could declare parameters, names and middleware for particular routes;

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 ProductCrudController::__construct(), for a certain controller action, with $this->middleware('admin')->only('store');. I don't think it makes sense to add yet another way to do something you can already do. Better to keep it simple. What do you think @OliverZiegler ?

Route names will be nasty if we have nested CRUDs

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

  • use setupOperationNameRoute() to load routes for each action individually, instead of setupRoutesForOperationName()
  • split BulkDelete into a separate operation;
  • split BulkClone into a separate operation;
  • remove $options parameter from all setupOperationNameRoutes() calls;
  • create a few nested CRUDs and see how route names get defined;

@tabacitu tabacitu changed the title [4.0][Feature][Ready] Routes are defined in each Operation [4.0][Feature][WIP] Routes are defined in each Operation Jul 22, 2019
@tabacitu
Copy link
Member Author

Fixed most of the above.

Note:

  • because we no longer have any operation traits in CrudController (each developer can add/remove operations by adding/removing traits to their EntityCrudController), you can't do parent::storeCrud() and parent::updateCrud() in the store() and update() methods; those methods are no longer in the parent, their in the current class;
  • so this should change to $this->storeCrud() and $this->updateCrud();
  • since this was a breaking change to that, I've also taken the liberty to rename the methods, so that it's $this->storeEntry() and $this->updateEntry() - I think that's close to what they're actually doing; Crud is not a thing; and if it is, it's the CrudPanel object, and those methods are NOT doing anything to the CrudPanel;
  • let me know if anybody finds a better name;

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.

@tswonke
Copy link
Contributor

tswonke commented Jul 23, 2019

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 :-)

@tabacitu
Copy link
Member Author

@OliverZiegler - you were totally right about Nested CRUDs. There were routes with the same name. For example:

user.index -> for user.index
user.index -> for user.post.index
post.index -> for post.index

I just pushed a fix for this, so that:

  • Backpack no longer prefixes route names with crud.; no route name prefix by default;
  • developers can now choose to prefix their route names by specifying the "name" attribute on the route group; it's an existing Laravel route group feature, it works for Route::get(), Route::controller() etc, we're just making it also work for our macro, Route::crud();

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:

user.index -> for user.index
user.post.index -> for user.post.index
post.index -> for post.index

I don't love the fact that name has to end with a dot. But hey - if it's the Laravel way, let's keep it that way :-)

@tabacitu tabacitu changed the title [4.0][Feature][WIP] Routes are defined in each Operation [4.0][Feature][Ready] Routes are defined in each Operation Jul 23, 2019
@tabacitu tabacitu changed the base branch from master to v4 August 29, 2019 07:16
@tabacitu tabacitu merged commit fb007a8 into v4 Aug 29, 2019
@tabacitu tabacitu deleted the routes-in-controllers branch August 29, 2019 08:26
@tabacitu
Copy link
Member Author

tabacitu commented Aug 29, 2019

Merged into the v4 branch.
Very good work here. Very happy with the result.

Thanks a lot for contributing on this @OliverZiegler & @tswonke !

@Mte90
Copy link

Mte90 commented Oct 11, 2024

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 setupRoutesFor* but the documentation refers about something else.

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?

@pxpm
Copy link
Contributor

pxpm commented Oct 15, 2024

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

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

Successfully merging this pull request may close these issues.

5 participants