Skip to content

Conversation

tabacitu
Copy link
Member

@tabacitu tabacitu commented Jul 12, 2019

Solution for #1877

This allows developers to create Operations and Actions for those Operations without having to add an extra route. One less step. This also means it's:

  • easier to add a custom Operation to multiple CrudControllers (have it as a trait and add it to all CrudControllers you want; no route modifications needed);
  • easier to install Operations from other people (ex from package developers);

How it works:

  • when registering the routes for the EntityCrudController, a new catch-all route is added entity/do/...;
  • the first parameter after do would be the name of the public method that needs to be called on EntityCrudController;

So when you add a public function moderate() method in your ProductCrudController, you don't need to add a route for it, you can use the catch-all route product/do/moderate.

Of course, it requires developers to be careful about the methods in their Controllers. Only public-facing methods should be public. Anything else should be private.

PROBLEM 1

One downside to this is that the function parameters don't work as they usually do in Laravel's controller methods - so it's not as intuitive as I wanted it. Since it's gotten to moderate() through a catch-all route, NOT something like /{id}/moderate, you can't do public function moderate($id) {}. You'd have to determine the ID from the URL string. Here's an example with how you can do stuff:

    public function moderate()
    {
        $parameters = func_get_args()[0];
        $httpVerb = \Request::method();
        $input = \Request::input();

        // manually check that the http method is the one you allow
        if ($httpVerb != 'GET') {
            abort(500, "No no no no. Don't funk with my heart.");
        }

        // manually get the URL parameters
        $firstParameter = explode('/', $parameters)[1];
        $secondParameter = explode('/', $parameters)[2];

        echo '<strong>$parameters</strong><pre>'; var_dump($parameters); echo '</pre>';
        echo '<strong>$firstParameter</strong><pre>'; var_dump($firstParameter); echo '</pre>';
        echo '<strong>$secondParameter</strong><pre>'; var_dump($secondParameter); echo '</pre>';
        echo '<strong>$input</strong><pre>'; var_dump($input); echo '</pre>';
        echo '<strong>$httpVerb</strong><pre>'; var_dump($httpVerb); echo '</pre>';

        dd('this was moderate()');
    }

And here's the output for /product/do/moderate/1/2?firstGetVariable=yes&secondGetVariable=no:
Screenshot 2019-07-12 at 19 36 09

You can also call the EXISTING operations over with the /do/ url:

  • GET /do/index works
  • GET /do/create works
  • PUT /do/create works
  • GET /do/{id}/edit does NOT work (because of id)
  • POST /do/{id}/edit does NOT work (because of id)
  • ...

You get the idea; everything that has an {id} in the route will not work when called through the catch-all /do/ - it will only work though its own route.

PROBLEM 2

You won't have named routes for these operations. If they're called through a catch-all route, you have to use route actions in URLs, instead of route names.


I would very much like to develop this further. Or merge it as-is. If anybody has any idea how to eliminate one of these problems, please let me know.

Even now, I think the PROs outweigh the CONs. I think for most users they wouldn't matter that much:

  • they can just use routes like product/do/moderate/1 instead of product/do/1/moderate;
  • they can just use backpack_url($crud->entity.'/moderate') instead of route('...');

Thoughts? Feedback?

@tabacitu
Copy link
Member Author

Note: you can still type-hint form requests. So you can still have validation on those methods.

public function store(StoreRequest $request) works like a charm.

@tabacitu
Copy link
Member Author

I slept on it. No longer like this :-) So let's scrap it in favour of the much better #1937

@tabacitu tabacitu closed this Jul 15, 2019
@tabacitu tabacitu deleted the catch-all-route branch July 15, 2019 13:42
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.

1 participant