Models
The first thing is to shake off the idea that MVC means three different class types. "M" in MVC doesn't mean data models like User
or Post
or Article
. "M" in MVC means "The Model" - aka the entire domain layer of your application. It's unfortunate that ORM entity data models are named what they are, because it leads to confusion. They are categorically NOT what "M" in "MVC" means. At all.
The "Model" in MVC can be made up of any number of different classes necessary to model the behavior and state of your domain. That Model can include specific entity/data models like a Post
class or an Article
class, services, validators, or anything else you might need, but the idea is that it's a layer, not a type of class. So in reality, almost everything should go into "The Model".
If you're asking what should go into your more specific data models that represent entities like Post
or Article
or what have you, the answer is typically whatever is most relevant to them. Those objects should tell you about themselves, validate their state, and control how their own data & state gets mutated, but nothing else. They should not manage or manipulate other entities, and should not be dumping grounds for functions/methods that have nothing to do with their own data and state.
Any operation that involves updating multiple entities/data models, should be done via another class with a well defined responsibility. You will want to avoid to creating generic service classes (like UserService
) that become dumping grounds for functions and methods. That's not what good object-oriented programming is. It's good practice to deliberately avoid using generic catch-all words like Service
and Manager
in your class names. You want classes to have specific responsibilities.
Controllers
Unlike "M" in MVC, Controllers aren't just a layer - they are indeed a specific type of class with a specific responsibility:
- Accept the request object
- Call the appropriate part(s) of the Model to do heavy lifting (fetch data, store data, manipulate data, validate data etc...)
- Return a response to the browser with the results of that model call
That is literally all they should. The challenge comes in deciding how much they should utilize the Model. A given controller action might need to perform several steps. Take registration for example: validate the request / form, create a new user, send a welcome email, and return a redirect to /welcome
or /profile
or what have you. Different parts of the Model can do each of those individual actions, but do you let the controller glue those individual pieces together, or do you just put that stuff into a dedicated class inside of the Model and just call that from the controller? Honestly, it really depends. If the whole operation happens in a lot of places and needs to be re-used, then encapsulated it somewhere in the Model. If it doesn't, then just create that procedure right there in the controller, calling various bits of the Model as needed. Controllers and controller actions are easy to test, so it's fine if you want to "glue" bits of the Model together right there in the controller. This isn't very "pure", but it's practical, and practical is what ships software so....
Your Specific Examples
I know you know your forums "model" is doing far too much, but I just want to go through a quick exercise with one of the methods:
Take your mainPage()
method for example. What you're really doing there is fetching the data that would be describe a board index - a listing of forums, grouped by category. But all you're doing is returning an unstructured array of that data. Later on, something is going to take that data, and actually assemble it into a view. So if we were following OO principles, you might want to consider that function an entire class unto itself - a BoardIndexData
, which can have various methods like ->getCategories()
, ->getForums()
, and ->getTopicData(). But you also probably want a way of assembling the individual bits of that data into a useful structure that a template can just do some dumb rendering of. So in addition, you probably want something like a
BoardIndex` class that doesn't do any data fetching, but rather, assembles forum index data into a useful nested structure. So you could do something like:
$boardIndex = new BoardIndex($categories, $forums, $topicData);
Then in a view template, just do something like:
@foreach($boardIndex->buildTree() as $category)
<h2>{{ $category->name }}</h2>
@foreach($category->forums as $forum)
<a href="...">{{ $forum->name }}</a> {{ $forum->total_topics }} {{ $forum->last_post_author->name }} // etc....
@endforeach
@endforeach
Why do something like this? Because now BoardIndex
contains the logic necessary for assembling formerly unrelated bits of data that can be fetched anywhere. You can create N-number of different board indexes just by passing in different categories/forums/topicData. But maybe this split isn't necessarily the right way to go - it really depends a lot on your application. However, the general idea is that you don't want to treat that Forums class as a dumping ground. Concepts like board index, forums, topics, threads, pings, posts, marking topics as read etc.. all of those are distinct things that should each have their own classes or sets of classes modeling them.
Your Specific Questions
processThread question
So the first issue is the name. What does "processThread" mean? From looking at the function, you're actually creating a new thread. Along the way, you're doing authorization checks, validation of input, the actual database insertion, sending out a notification, flashing session errors, and creating a redirect which you then return. But generally, the domain action you're performing is the creation of a new thread. So I would rename the method to be more clear and explicit: createNewThread(...)
You probably do want this to encapsulate data fetching that it needs, as well as do appropriate validation. So yes, it should be grabbing the topic, and the controller should only pass in the data it gets from the request object. However, notions like sessions and redirects are typically NOT part of the Model's responsibility, so that should not be here. What you should be doing is returning a kind of ServiceResponse
object that includes the newly created object (or at least it's ID), a status code, and any error messages. You'd then let the controller decide what to do with this response object. So it might be something like this in the controller
$serviceResponse = $this->forums->createNewThread(...data from request...)
if ($serviceResponse->ok()) {
return redirect('/forums/thread/'.$serviceResponse->data->id);
} else {
Session::flash('errors', $serviceResponse->errors);
Request::flash();
return redirect('/forums/create/'.$topicId);
}
If you find yourself doing that pattern a lot, you can wrap all of the session/flashing stuff in its own utility class or even just stick it in a method in the parent controller and call this->redirectWithErrors('/url/to/go/to', $serviceResponse->errors)
validation question
I think the validation should live closest to where it is most relevant, which is in the method itself. You can delegate out to a dedicated validation class if there is a lot of complex validation to be done, but you definitely want to at least call that validation from within that method. The method itself should protect itself from bad data, which means validation should either be done in that method, or called from that method. Otherwise, you could accidentally call that method without doing validation beforehand, and now your Model is in a bad state.
redirect
Definitely do redirect in the controller. Controllers manage user flow, and redirection is user flow.
bonus question
Why not pass that data into the view directly? You shouldn't have to do static calls from a view ever, because you can always just pass that data into the view itself.