mybb2 icon indicating copy to clipboard operation
mybb2 copied to clipboard

Use validation for routes with id

Open JN-Jones opened this issue 10 years ago • 7 comments

As @wpillar mentioned here: Instead of doing

$el = $this->elRepository->find($id);

if(!$el) {
    throw new ElNotFoundException;
}

in the controller we should add custom request classes to those functions which use the validor: id => exists:elTable

  • [ ] ForumController
  • [ ] TopicController
  • [ ] PostController

JN-Jones avatar Apr 23 '15 09:04 JN-Jones

We should be doing this for all new controller code as of now and make a list of existing cases that need to be addressed retrospectively.

wpillar avatar Apr 23 '15 09:04 wpillar

Well, existing cases are all controllers/routes which need an id. (forum show, all topics (either forum, topic or post), like, quote, conversations, user profile)

JN-Jones avatar Apr 23 '15 09:04 JN-Jones

@JN-Jones yup, I'll start adding a list to this issue. If you could add any I miss, that would really help :)

wpillar avatar Apr 23 '15 09:04 wpillar

So we should be doing two queries rather than one? No thanks...

A simple fix would be to change the repositories to use findOrFail(), which throws an exception if the ID is not found. Putting this kind of data retrieval logic inside a request class makes no sense to me. Repositories handle data retrieval, requests should handle validating request parameters. They should only check if the ID is an integer and isn't empty (e.g.: required|integer).

euantorano avatar Apr 23 '15 21:04 euantorano

Another thing I forgot: The find repository method also handles permissions for forums: https://github.com/mybb/mybb2/blob/master/app/Database/Repositories/Eloquent/ForumRepository.php#L59-L64

JN-Jones avatar Apr 24 '15 09:04 JN-Jones

Also required isn't needed: If the id isn't set laravel can't find the correct route anyways (at least with the current routes).

JN-Jones avatar Apr 24 '15 11:04 JN-Jones

True.

euantorano avatar Apr 24 '15 11:04 euantorano