1

My aim is to adapt the following controller methods to follow the DRY principle by moving the validation to form requests. However, I have a problem which is that there are overlaps between validation for different methods. For example, let's say that the input form has only one field and that's name, and that it's related to a Task model. Let's consider 3 controller methods now:

  • store method: validate that name isn't empty
  • update method: validate that name isn't empty and that a Task with the given $id exists
  • destroy method: validate that a Task with the given $id exists

So for the store method I am checking the first assumption, for the destroy method I am checking the second assumption, and for the update method I am checking both.

So, ideally, I would like to be able to do something like...

public function store(StoreTask $request)...
public function update(Store Task TaskExists $request, $id)...
public function destroy(TaskExists $id)...

...but I am very unclear on how to write the syntax for this, and whether there is some other approach which I'm missing to accomplish the same thing.

sveti petar
  • 3,637
  • 13
  • 67
  • 144
  • 2
    If you use dependency injection with Laravel, you do not need to check if the given id exists. For example, in `update(Request $r, Task $task)`, laravel will automatically check if the task that belongs to the sent id (or task) exists. So you can have a shared validation logic that applies `required` rule on `name`. Maybe do it through request, or have a separate function.. – user3532758 Nov 27 '19 at 09:24

1 Answers1

1

Validation:

With Route model binding Laravel will automatically inject the model instance so you don't have to query the database yourself and if you have the same validation for storing and updating a task you can use the same form request for both actions.

So in the end you would have something like this:

public function store(TaskRequest $request)...
public function update(TaskRequest $request, Task $task)...
public function destroy(Task $task)...

If you have different Validation rules for storing and updating, you can create two form requests:

public function store(StoreTaskRequest $request)...
public function update(UpdateTaskRequest $request, Task $task)...

And then either repeat your validation rules in both requests or use either a trait / separate class or method that contains the rules and merge them when needed.

This answer has some good suggestions.

Authorization:

To check if the user is allowed to edit/update the task I would use Laravel's Authorization.

In your case you could create a policy.

In your TaskPolicy:

public function update(User $user, Task $task)
{
    return $user->id === $task->user_id;
}

In your TaskController:

Constructor:

public function __construct()
{
    $this->authorizeResource(Task::class, 'task');
}

Or single action:

public function update(UpdateTaskRequest $request, Task $task)
{
    $this->authorize('update', $task);
}
Remul
  • 7,874
  • 1
  • 13
  • 30
  • Thanks, route-model binding does solve the problem for the example I posted. but what if I still have to validate different things, for example in the `update` method I may want to make some additional check, such as that the `task` hasn't been updated in the last 24 hours? – sveti petar Nov 27 '19 at 09:50
  • Or I may want to check in the `update` method that the `task` belongs to the user who is trying to make the update (column `user_id`)... – sveti petar Nov 27 '19 at 09:51
  • Awesome answer, I learned a lot about how Laravel handles this sort of thing. – sveti petar Nov 27 '19 at 10:11
  • One more thing: I'll still need to catch whatever exception the policy throws if the condition is not filled, so I suppose I'll still need a `try...catch` block or something similar in the controller method? Or there is a way for the policy to handle that on its end? – sveti petar Nov 27 '19 at 10:21
  • @jovan The policy will throw a `AuthorizationException` if the user is not authorized and Laravel's error handler will catch it and return a `403` response automatically so you don't need a try catch block. – Remul Nov 27 '19 at 10:26