[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 [4.0][Feature][WIP] Added catch-all route to all CrudControllers #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?