Yeah was going to suggest splitting out the render function as it did seem a bit out of place but didn’t want to mess with your example too much.
I agree, for our purposes here, I don’t think breaking the model apart will make any difference at all so let’s keep it simple.
The basic problems with the suggested approach are.
- The controllers are doing too much. It has more than one responsibility:
- Updating the model based on user input
- Selecting which view to render
- Generating the response
This of course, is a symptom rather than a problem in itself.
-
Repeated code in controller actions. Every controller action must call $this->render even though they’re often rendering the same template
-
Unnecessary and outright inflexible binding logic, any piece of information required by the view must be fed to it by the controller
return ['headers' => [], 'body' => $this->templating->render('newThing.html.php', ['entity' => $post, 'violations' => $violations])];
The biggest hint that this is a bad idea is that the controller must know a lot about the template in order to work, it has to provide the relevant array that contains both ‘entity’ and ‘violations’. Serious question: Why should the controller be aware of what the view is doing? And worse, the data structures that the view requires in order to represent the data. The result of this broken encapsulation is that the controller can only work with views that need only those two parameters. If another template requires a third piece of information, perhaps a logged in username or something the controller has to be amended. Some flexibility can be added like this by removing the hardcoded template name:
$this->templating->render($this->template, ['entity' => $post, 'violations' => $violations])];
which at first seems to solve the problem, is actually still very restrictive. If there are two or more templates that could be used here they must require the exact same set of information, if they require any different information then the controller action must be amended to provide it, and it will be provided to any template that might be used as $this->template.
- Display logic in controller actions.
return ['headers' => ['Location: /somewhere-else'], 'body' => ''];
“Redirect to a different page” or “This is the html which should be displayed” is display logic and belongs in the view.
- The NewController is 100% redundant (I’ll refactor this out), you’re constructing a controller object and calling the controller action just to render a view. Skip the middle man and just render the view.
I’ll elaborate on these as I refactor the code. Unfortunately “incremental” doesn’t really work because it’s an architectural change which obviously has knock on effects to all collaborators, however I’ll make as few changes as possible keeping all existing method/variable names
and as much code as I can.
Ok on with the refactoring:
controllers.php
<?php
require_once 'model.php';
class CreateController {
private $model;
public function __construct(Model $model) {
$this->model = $model;
}
public function doAction($post)
{
if ($this->model->insertThing($post)) {
$this->model->success();
}
}
}
You’ll notice this is sparse in comparison but there are immediate advantages here:
- The controller is coupled only to the model, not the view
- Doesn’t break SRP: It now only has the responsibility of updating the model with user input, not the mixed responsibility of also selecting and rendering the view
- Where there is no user action (just viewing a template) there is no need for a controller
model.php:
<?php
class Model {
public $violations = [];
public $entity;
public $success = false;
public function validateThing($entity)
{
// Pretend both are required
if (!isset($entity['field1']) || $entity['field1'] === '') {
$this->violations[] = 'Field 1 is required.';
}
if (!isset($entity['field2']) || $entity['field2'] === '') {
$this->violations[] = 'Field 2 is required.';
}
// Pretend field 2 requires at least 3 chars
if (strlen($entity['field2']) < 3) {
$this->violations[] = 'Field 2 must have at least 3 characters.';
}
return $this->violations;
}
public function insertThing($entity)
{
// Pretend insert SQL
$this->entity = $entity;
}
public function success() {
$this->success = true;
}
}
A couple of minor changes here. The model now has $violations and $entity as class variables that is accessible to the view and the addition of a success method that is called when the form has been submitted. Why not just call success in insertThing()? Because another part of the system might want to reuse the insert logic without triggering the success method (which perhaps might send an email or log some information)
view.php
I’ve taken your template class and changed it a little. This is now a view that has access to a model and loads a template given as a constructor argument.
<?php
class View {
private $template;
private $model;
public function __construct($template, $model) {
$this->template = $template;
$this->model = $model;
}
public function render()
{
$headers = [];
$model = $this->model;
ob_start();
require $this->template; // <-- another discourse indentation bug
$content = ob_get_clean();
return ['headers' => $headers, 'body' => $content];
}
}
The view is responsible for all output, headers and body, removing this responsibility from the controller. I’ll explain why this is better shortly.
newThing.html.php
<?php if ($model->success) {
$headers[] = 'Location: /somewhere-else';
return;
} ?>
<?php if (!empty($model->violations)): ?>
<ul>
<?php foreach ($model->violations as $violation): ?>
<li><?= htmlspecialchars($violation) ?></li>
<?php endforeach ?>
</ul>
<?php endif ?>
<form action="/new" method="post">
<label>Field 1 <input name="field1" value="<?= isset($model->entity) ? htmlspecialchars($model->entity['field1']) : '' ?>"></label>
<label>Field 2 <input name="field2" value="<?= isset($model->entity) ? htmlspecialchars($model->entity['field2']) : '' ?>"></label>
<input type="submit">
</form>
Redirect aka “Display a different page” is display logic so clearly belongs in the view, here if the insert was successful the redirect happens. It’s now the view’s job to return headers and content rather than having the controller tell the view what its content-type is. I’ll elaborate on this in a second, but for now:
index.php
<?php
require_once 'controllers.php';
require_once 'view.php';
// Route
if ($_SERVER['REQUEST_URI'] === '/new' && $_SERVER['REQUEST_METHOD'] === 'GET') {
$model = new Model();
$view = new View('newThing.html.php', $model);
} else if ($_SERVER['REQUEST_URI'] === '/new' && $_SERVER['REQUEST_METHOD'] === 'POST') {
$model = new Model();
$controller = new CreateController($model);
$controller->createAction($_POST);
$view = new View('newThing.html.php', $model);
} else {
// 404; not implemented
exit;
}
$response = $view->render();
// Send response
// Assumes response in the form of ['headers' => [...], 'body' => ...].
foreach ($response['headers'] as $header) {
header($header);
}
echo $response['body'];
The router previously selected a model, selected a controller, selected a controller action and called it. Now it selects the model, view and controller being used and calls the controller action. The view is configured with a specific template.
You’ll notice that $response comes from $view->render() rather than being returned by the controller.
Advantages of this approach
N.b. I realise some of these things can be incorporated into Web MVC but it doesn’t make sense to compare against a non-existent example so these are in comparison to the example provided.
1) The view and controller are decoupled
The problem with putting the view logic (render, redirect, etc) the controller is that it makes the controller difficult to reuse with other view types. It’s not unreasonable to want the form to be submitted via AJAX and return a JSON response in the format {"success": false, "errors": ["Field 1"]}. Here you most certainly do not want a redirect header on a successful insert. Using your approach, the controller action always returns a redirect header on success.
If the controller knows that the view is responding with HTML (rather than PDF, JSON, etc) then the controller knows too much about the state of the view, breaking encapsulation.
What does this mean in real terms? Well it’s easy to make a view that returns the state of the model in JSON rather than HTML:
<?php
class JsonView {
private $model;
public function __construct($model) {
$this->model = $model;
}
public function render()
{
$headers = ['Content-type: application/json'];
return ['headers' => $headers, 'body' => json_encode($this->model)];
}
}
Using the same controller and model it’s simple enough to generate a JSON response, that doesn’t redirect and contains the model’s state.
index.php
<?php
require_once 'controllers.php';
require_once 'view.php';
// Route
if ($_SERVER['REQUEST_URI'] === '/new' && $_SERVER['REQUEST_METHOD'] === 'GET') {
$model = new Model();
$view = new View('newThing.html.php', $model);
} else if ($_SERVER['REQUEST_URI'] === '/new' && $_SERVER['REQUEST_METHOD'] === 'POST' && $_SERVER['HTTP_X_REQUESTED_WITH'] == 'XmlHttpRequest') {
$model = new Model;
$controller = new CreateController($model);;
$controller->doAction($_POST);
$view = new JsonView($model);
} else if ($_SERVER['REQUEST_URI'] === '/new' && $_SERVER['REQUEST_METHOD'] === 'POST') {
$model = new Model;
$controller = new CreateController($model);
$controller->doAction($_POST);
$view = new View('newThing.html.php', $model);
} else {
// 404; not implemented
exit;
}
$response = $view->render();
// Send response
// Assumes response in the form of ['headers' => [...], 'body' => ...].
foreach ($response['headers'] as $header) {
header($header);
}
echo $response['body'];
And now we have ajax and non-ajax form submissions using most of the same code unchanged. I don’t need to alter the controller action (or add a new controller action) specifically to handle this request, only reconfigure the router and it will all just work for both ajax and non-ajax form submissions returning either the HTML or JSON representation of the model. Changes made to the controller logic will affect both ajax and non-ajax requests as there is no repeated code.
Want a PDF representation of an HTML page? Sure… assuming use of the mPDF library
<?php
class PdfView {
private $template;
private $model;
public function __construct($template, $model) {
$this->template = $template;
$this->model = $model;
}
public function render()
{
$model = $this->model;
ob_start();
require $this->template; // <-- another discourse indentation bug
$content = ob_get_clean();
$headers = ['Content-type: application/pdf'];
$mpdf = new Mpdf();
$mpdf->WriteHtml($content);
return ['headers' => $headers, 'body' => $mpdf>Output()];
}
}
And just add a route for it, selecting the existing model, controller and template while changing the view.
This is difficult with your example because the controller has to set the content-type header. This means it’s impossible to do without either adding a controller action or adding an if/else to the doAction method. Creating a PDF version of a HTML page in your example forfeits reuse somewhere, the simplest approach being the need to add a specific controller action to generate the PDF.
2) The model stores the application state
Because the model has a state and stores Success/failure and errors, this state can be represented in different ways. What this means is that all the awful repeated binding logic between controller and view can be removed entirely:
$this->render('newThing.html.php', ['entity' => $post, 'violations' => $violations])];
This is 100% redundant. In your example, this is a requirement in each and every controller action. By rendering the view in the front controller and giving the view access to the model, this binding becomes irrelevant $violations and $entity are stored in one place and one place only avoiding this game of pass the parcel (and lowering memory usage).
The model provides an API for the view to use which is shared by any view that uses the model. There’s no extra binding logic required to grab data from the model and pass it to the view.
3) Business logic belongs in the model
Want to send an email when the form is submitted? Put it in the success() method. Want to track hack attempts by analysing the content of the submission? Put it in the success method in the model. The business rules for what happens when the form is submitted are handled by the model leaving the controller free to be reusable and deal only with user input. Padraic Brady summed this up perfectly here: http://blog.astrumfutura.com/2008/12/the-m-in-mvc-why-models-are-misunderstood-and-unappreciated/
In my mind, the best Controller is a Controller which doesn’t need to exist :). I was immediately treated to a mass of resistance. Few people seemed to understand that an empty Controller, with all Model interaction extracted into simple reusable View Helpers, would reduce code duplication in Controllers (lots of code duplication!) and avoid the necessity of chaining Controller Actions.
By letting the model, rather than the controller, have control over what happens when the form is sumitted, it allows easily reusing the controller with different models.
4) By setting headers in the view and giving it access to the model life is simpler. For example (sorry the form one can’t really demonstrate this, consider a template that displays a product on an ecommerce site by loading a product based on an ID e.g. /product/1245 )
$product = $model->getProduct();
if ($product) {
?>
<h1><?= $product->title; ?></h1>
<p><?= $product->description; ?></p>
<?php
}
else {
$headers[] = 'HTTP/1.0 404 Not Found';
?>
<p>Sorry, the product you're looking for could not be found.</p>
<?php
}
The view here decides what to do when the product doesn’t exist. Redirect to home page? Give a 404 and display a message? By setting headers in the controller this check needs to happen twice, once in the controller to determine whether or not to give the 404 header and once in the view to display the error message.
The controller in this example is not concerned with domain concepts (Does the product exist or not?) all it does it handle the user input. It’s up to the model to decide what to do when someone tries to load an invalid product and the view to decide what to do when the model can’t provide it a product.
And the reasons for the model not providing the view a product could be numerous: It’s been unpublished, it’s been deleted, it exists only being published after a specific date which is in the future. By setting the header in the controller the controller either needs to know these business rules or inquire a display status for the current product from the model, putting more work in the controller.
5) There is no need for controllers or controller actions when there is no user input
Your implementation has a router which selects a controller, selects a controller action then the controller action then selects a view. When all the controller action is doing is selecting a view this chain of events surely is redundant. The callstack in each route is:
controller::__construct($model);
controller::action();
controller::render();
In this alterantive approach, the callstack in the route is:
view::__construct($model)
(view::render() is then called automatically for all routes, this call is never repeated!). You’ll need to make a pretty strong case to argue for this added complexity. What is the benefit of constructing a controller just so that that can render a template and do nothing else? Why not just render the template?
6) Remeber the dangerous binding logic?
$this->render($this->template, ['entity' => $post, 'violations' => $violations])];
Lets say I alter your controller to accept a template name to use in the action:
$controller = new CreateController(new Model, 'newThing.html.php');
$response = $controller->createAction($_POST);
And let’s say I want to have a version of the view which displays some information about the logged in user… First I have to change the view being assigned:
$controller = new Controller(new Model, 'newThingWithUser.html.php');
$response = $controller->createAction($_POST);
(And I’m not even mentioning the elephant in the room that most Web MVC implementations have actions for more than one view here, you have avoided this by splitting each action into its own class)
There is no way to configure the view to have access to different information. For example, what if the view should require the logged in user? The only way to do this is to change the controller. For simplicity lets assume the user can be retrieved with the static method User::getCurrent();
$this->render($this->template, ['entity' => $post, 'violations' => $violations, 'user' => User::getCurrent()])];
This inadvertently makes $user available to every instance of the view. Not really a problem but it’s rather messy, all the variables that could ever possibly be needed by any template must be provided by the controller. With 100 variants that all need different information this is going to be messy isn’t it?
Instead, using a separate view class, this becomes incredibly simple:
$model = new Model;
$controller = new CreateController($model);
$controller->doAction($_POST);
$view = new View('newThing.html.php', $model);
and
$model = new Model;
$controller = new CreateController($model);
$controller->doAction($_POST);
$view = new View('newThingWithUser.html.php', ['form' => $model, 'user' => User::getCurrent()]);
Ok an array isn’t probably isn’t ideal but you could make a new model that extended or encapsulated $model and provided $user as well:
$model = new ModelWithUser;
$controller = new CreateController($model);
$controller->doAction($_POST);
$view = new View('newThingWithUser.html.php', $model);
7) Which adds up to increased flexility.
-
I can use the same controller code with any form in my application. (In the event that in an edge case this isn’t sufficient and needs additional logic I can easily enough add a new controller that is)
-
Without changing the controller, I can change the model to send an email when the form is submitted, this allows the use of the controller with different models and views. I can have completely different forms being displayed but use the same controller. Alternatively, I can create a new model using the same view and controller.
-
I can very easily use a different template that formats the HTML in a different way and use it with the same model/controller (and view class). With your example, the template name is hardcoded into the controller making it impossible to substitute and would require either: a new route and a new controller action or some hacky if/else branching in the controller action.
I realise there are obvious fixes for a few of these problems in Web MVC, however, properly applying inversion-of-control and separating out the responsibilities so that the controller doesn’t break SRP is unquestionably a stronger foundation to work from.
Conclusion
Hopefully this example demonstrates that “Web MVC” blurs the responsiblities of the components by putting all the decision making and application state into controller actions. Instead, by putting the application state in the model, and decoupling the controller from the view, everything becomes more flexible because you can easily pick and choose different components.