Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Use validation for routes with id #106

Open
3 tasks
JN-Jones opened this issue Apr 23, 2015 · 7 comments
Open
3 tasks

Use validation for routes with id #106

JN-Jones opened this issue Apr 23, 2015 · 7 comments

Comments

@JN-Jones
Copy link
Contributor

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
@wpillar
Copy link
Contributor

wpillar commented Apr 23, 2015

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.

@JN-Jones
Copy link
Contributor Author

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)

@wpillar
Copy link
Contributor

wpillar commented Apr 23, 2015

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

@euantorano
Copy link
Member

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).

@JN-Jones
Copy link
Contributor Author

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
Copy link
Contributor Author

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).

@euantorano
Copy link
Member

True.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants