Crud: [4.0][Feature][Proposal] Automatic routes for CrudControllers

[tabacitu]

Right now, if you want to add a new Operation (say... moderate), you also have to add the new route for it. For example, in backpack instant fields @tswonke instructs you to do:

CRUD::resource('entity', 'EntityCrudController')->with(function () {
   Route::any('entity/ajax/{mode?}', 'EntityCrudController@handleAjaxRequest');
});

Which is a simple way to do it. But I'd rather not go to a different file to enable a CrudController operation. I think use InstantFields; should be the _only_ step.

This small inconvenience is also present when using select2_from_ajax fields. You have to create a method, but you also have to create a route for it, in a different file.

Solution 1.

We could expose all public methods in your EntityCrudController as routes.

PROs:

• It would also make it dead-simple to create new operations - just create a new public method;
• It would make it super simple to install operations from other packages; just use TraitName;
• It's similar to how CodeIgniter did it, so it should be familiar to those coming from there;

CONs:

• We have to be very VERY clear that public methods have routes generated for them, so that people don't create public methods in EntityCrudControllers willy-nilly; This would probably get solved with a comment inside generated EntityCrudControllers (just split the file into private methods and public methods);
• We will need some conventions, to settle the action and parameters for the route; probably:
  
  
  
  • the method has to be public for it to have a route;
  
  
  • if the method name does not begin with HTTP verb (get, post, put, patch, delete, options), then it'll assume it's get;
  
  
  • if the method has parameters (other than a Request), then the route should have the same parameters;
  
  
  • if the parameters have a default value, they are optional; otherwise they're required;
  
  
  • public function index() defaults to the main route;
  
  
  • the route name will be the method name;
  
  

The rules above would mean you'd only need Route::allPublicMethodsFrom('ProductCrudController'); in the route file. Then inside the ProductCrudController:

| Method definition | Instead of defining route like so |
| ------------- |:-------------:|
| public function moderate() | Route::get('moderate', 'ProductCrudController@moderate') |
| public function postModerate() | Route::post('moderate', 'ProductCrudController@postModerate') |

Existing methods would become:

| Method definition | Instead of defining route like so |
| ------------- |:-------------:|
| public function index() | Route::get('', 'CrudController@index') |
| public function create() | Route::get('create', 'CrudController@create') |
| public function postCreate(StoreRequest $request) | Route::post('create', 'ProductCrudController@store') |
| public function update() | Route::get('update', 'CrudController@update') |
| public function postUpdate() | Route::post('create', 'ProductCrudController@store') |

---

Solution 2.

We could have a getRoutes() method on the CrudController that gets the routes; Developers would be able to customize it:

    private function routes() {
       $defaultRoutes = parent::routes();

       $customRoutes = [
             'get' => [
                // route definition => 'method name'
                'moderate/{id}' => 'moderateee',
              ],
              'post' => [
                'moderate/{id}' => 'postModerate',
                'moderate/{id}/comment/{comment_id}/edit' => 'editComment',
              ],
              'put' => [
                'moderate/{id}/comment' => 'addComment',
              ],
              'patch' => [
              ],
              'delete' => [
                'moderate/{id}/comment/{comment_id}/delete' => 'deleteComment',
              ],
       ];

       return array_merge_recursive($defaultRoutes, $customRoutes);
    }

Solution 3.

Instead of making all routes as proposed in Solution 1, we could just add the _ability_ to expose extra routes just by making the method public and including the verb in the name. So edit() would not have an auto-route. But getEdit would.

Thoughts, anyone?

[tswonke]

Thanx @tabacitu for bringing this up.

IMO Solution 1 would be too much "magic". I highly prefer Solution 2, but one question:
Would it be possible to use the standard syntax? Can't test it right now.

Solution 4.

private function routes() {
       $defaultRoutes = parent::routes();

       $customRoutes = [
             Route::get('moderate/{id}', 'EntityCrudController@moderateee'),
             Route::post('moderate/{id}', 'EntityCrudController@postModerate'),
             //Route::group()...

       ];

       return array_merge_recursive($defaultRoutes, $customRoutes);
    }

Then we have all flexibility, I think.

If this is not possible wie should at least consider to enable the @-Syntax (optionally) for the method, so we are able to use another (Crud)Controller (of the package).

[tabacitu]

Hmm... you're right @tswonke - using the regular syntax would be better. It _should_ be possible, I see no reason why not. Thanks again! I think Solution 4 is the way to go right now.

[tswonke]

We probably have to introduce a helper like addRoutes() or something like that. Imagine multiple packages to be used in one EntityCrudController - each with a routes() method. That won't work.

[tabacitu]

Right. So maybe a property would be better. Something like this:

private $routes = [
        Route::get('moderate/{id}', 'EntityCrudController@moderateee'),
        Route::post('moderate/{id}', 'EntityCrudController@postModerate'),
        //Route::group()...
];

private function getRoutes() {
       return array_merge_recursive(parent::routes(), $this->routes);
    }

private function addRoute() {}
private function addRoutes() {}

Then again, this requires the user to do an addRoute() in their setup() for each Operation they want to add to the controller. So... you don't add the route _in a route file_, but you still need to manually add the route _in the controller_. Kind of defeats the purpose...

I guess the real problem here is that Operations are PHP Traits. And Traits can't have a separate initialization event, or even a __construct() method. Food for thought...

[tswonke]

Maybe something like this will work:

if a setup{TraitName}Routes() method (or similar) is present in the trait it could be triggered in the __consruct() of the original backpack CrudController?

[tabacitu]

Indeed, sounds like the only thing possible inside traits - a convention-based system. Which again is a bit of "magic"... Hm...

Solution 5.

private $routes = [
        Route::get('moderate/{id}', 'EntityCrudController@moderateee'),
        Route::post('moderate/{id}', 'EntityCrudController@postModerate'),
        //Route::group()...
];

private function getRoutes() {
       return array_merge_recursive(parent::routes(), $this->routes);
    }

private function addRoute() {}
private function addRoutes() {}

public __construct() {
       // ...
       foreach (preg_grep('/^setupRoutesFor/', get_class_methods($object)) as $method) {
           // TODO: call that setupRoutesForOperation method
       }
       // ...
}

[tswonke]

Yes, you are right!

But this might be an arguable extend of magic similar to the "boot model traits" magic of Laravel.

[pxpm]

I agree with @tabacitu on the redundancy, but @tswonke has a point. That magic already happens in laravel in model traits. We can borrow it and apply in this situation.

The thing i see, but maybe i'm getting it wrong. We are registering routes in controllers ? The controller does not need the routes to be called ? E.g acessing backpack/user/create, how we will be able to reach to the controller, if the route is not defined yet, only on controller load ?

Sorry if i misunderstood the question.

MC

[tswonke]

Ooups!
I think @pxpm is right... 😅

The definition of routes for a package have to be made in the PackageServiceProvider.
But this won't be enough, because it's not dynamic if the routes have to point to different controllers. We could use a package config for that, but then we are at the beginning of this topic (avoid editing another file).

I'm currently not sure if there is solution at all.

Nevertheless we should consider a convention for the method naming?
Imagine two backpack field packages a developer want to use in the same EntityCrudController and both needs traits to be included - with methods of the same name...

[tabacitu]

Thanks @pxpm . Loading all Controllers before routes should be possible - they're just PHP classes after all. As long as they live inside one directory, we should be able to just load them all. I saw it done in a package a while back, so I think it's possible (they did it with a naming convention similar to Solution 1). We might need to do $this->crud = app()->make(CrudPanel::class); inside the setup() method instead of __construct() though, so that __construct() doesn't need the complete request.

CONS:

• It might bring down performance - loading all CrudControllers, on every request. It might bring down performance _a lot_. I need to make some tests before we advance the talk any further - might make no sense at all.
• It will make our EntityCrudControllers have methods that _do not provide content to the user_; which I really don't like; I usually like every method inside my controllers to return something to the user; either a view or a JSON (if ajax); if it's not for the user, it's probably better for it to be in a ServiceProvider or something; this would break that rule;

Right now I don't think this is such a great idea anymore :-)

---

Alternative: If we create a configuration class for the CrudPanel, instead of configuring things in the ProductCrudController::setup() method, then we could define routes in ProductCrudConfiguration. Loading those configuration objects should be fast, since they're super-simple. But again, this would be _a different file_ :-) So it doesn't solve the initial problem - how to handle routes _in the same file_. You wouldn't do routes in _a routes file_ (where it's actually more intuitive), you would do it in the _configuration file_. Kind of defeats the purpose, I think. And it's un-Laravelly and un-intuitive.

Maybe it's actually better the first way we thought of it - _in the routes file_ :-)
Enabling operations would still be a two-step process:

• add this route to your routes/backpack/custom.php file;
• add this trait on your EntityCrudController;

It's not _that_ bad considering the solutions above actually make it more difficult to understand. At least this way, you know exactly how it works. It's a route that points to a controller method that you just added using a trait. Dead-simple. Which is what Backpack is supposed to be, easy to understand.

Damn it. Might have gotten too deep down this rabbit, and not realise the solution we have now is simpler and better. Will let this sink in for a few days, then come back to it with a fresh perspective, see what I think about it then. You also let me know your thoughts.

Good talk though :-))

[tabacitu]

So here's a crazy idea :-)

Solution 6.

What if we add a catch-all route?

// catch-all route for custom operations
Route::any($this->name.'/do/{function}', $this->controller.'@callPublicFunction')
    ->where('function', '(.*)')
    ->name('crud.'.$this->name);

This would pipe all /do/smth requests to CrudController::callPublicFunction(), which can then check if smth() exists & is public. If everything's alright, it would call that function.

PROs:

• the syntax would stay the same: CRUD::resource('product', 'ProductCrudController');
• this would come ON TOP of the current way of doing things, NOT INSTEAD;
• developers would be able to create a new operation method just by creating a public method in their controllers;
• very little magic going on, easy to understand;

CONs:

• I don't think there's a way for developers to have pretty parameters in their method; so if they want parameters they need to parse the URL and get them; which is... not pretty, but it's the only downside I see; see #1932 for more details;
• those custom actions would NOT have a route name; you have to call them by the action name;

Thoughts? Do the PROs outweigh the CONs? Do you see any way to eliminate any CONs?

[tabacitu]

About the Solution 6 above: I slept on it, and didn't like the downsides. Messy. Not pretty. So I closed the PR - I no longer think it's a good idea to have catch-all routes.

@tswonke , @pxpm - I'm happy to say I've implemented @tswonke 's proposed Solution No 5. And it works like a charm! I have found no performance issues whatsoever. I really _really_ like Solution No 5 now. Here's the PR for it - let me know what you think :-)

But I think we finally have a winner :-)

[tswonke]

Looks promising, will have a deeper look as soon as possible.
Have you also tested route caching?

[tabacitu]

Great, thanks again @tswonke for the solution. Hat's off to you :-)

I had not tried route caching, forgot about it. But I tried it now - also works like a charm.

PS. I'll close this issue because I'm pretty sure we'll all be ok with this solution, so let's move the conversation to the PR - https://github.com/Laravel-Backpack/CRUD/pull/1937 . We'll reopen this thread if we decide to scrap that solution too 😅