Some quick context for others: Over the years, @TomB and I have debated the merits of various approaches to MVC in server-side web applications. To continue that discussion, we’re going to start with a bare bones Web MVC sample, discuss any potential deficienies, and, if any, then possible ways to refactor.
Tom, below is what I think is the barest of bones Web MVC. We can make adjustments as needed to consider a variety of scenarios.
##.htaccess
RewriteEngine On
RewriteCond %{REQUEST_FILENAME} !-f
RewriteRule ^ index.php
##index.php
<?php
require_once 'controllers.php';
// Route
if ($_SERVER['REQUEST_URI'] === '/new' && $_SERVER['REQUEST_METHOD'] === 'GET') {
$response = newAction();
} else if ($_SERVER['REQUEST_URI'] === '/new' && $_SERVER['REQUEST_METHOD'] === 'POST') {
$response = createAction($_POST);
} else {
// 404; not implemented
exit;
}
// Send response
// Assumes response in the form of ['headers' => [...], 'body' => ...].
foreach ($response['headers'] as $header) {
header($header);
}
echo $response['body'];
##controllers.php
<?php
require_once 'model.php';
// Shows a form to create a "thing"
function newAction()
{
return ['headers' => [], 'body' => render('newThing.html.php')];
}
function createAction($post)
{
$violations = validateThing($post);
if (empty($violations)) { // if valid
insertThing($post);
return ['headers' => ['Location: /somewhere-else'], 'body' => ''];
} // <-- discourse indentation bug
// Otherwise, not valid
return ['headers' => [], 'body' => render('newThing.html.php', ['entity' => $post, 'violations' => $violations])];
}
// Utility function: Render a PHP template with params and returns the result
function render($template, $params = [])
{
extract($params);
ob_start();
require $template; // <-- another discourse indentation bug
$content = ob_get_clean();
return $content;
}
##model.php
<?php
function validateThing($entity)
{
$violations = [];
// Pretend both are required
if (!isset($entity['field1']) || $entity['field1'] === '') {
$violations[] = 'Field 1 is required.';
}
if (!isset($entity['field2']) || $entity['field2'] === '') {
$violations[] = 'Field 2 is required.';
}
// Pretend field 2 requires at least 3 chars
if (strlen($entity['field2']) < 3) {
$violations[] = 'Field 2 must have at least 3 characters.';
}
return $violations;
}
function insertThing($entity)
{
// Pretend insert SQL
}
Barest bones. Trying to keep everything boiled down to their very simplest to help zero in on the most crucial details. Certainly the templating, the validator, the repository, among others, could be made as injectable objects, but those aren’t the crucial details to Web MVC.
Excellent, I’m going to turn this into OOP for the simple reason that without OOP object state it’s impossible to have a proper model. In MVC, the controller updates the model and the view reads from it, without OOP this is particularly difficult and I’d rather avoid global variables even for an example. Not only that, it’s difficult to highlight issues of flexibility/bad practice when you don’t have information hiding/LoD violations and polymorphism.
controllers.php:
<?php
require_once 'model.php';
class Controller {
private $model;
public function __construct(Model $model) {
$this->model = $model;
}
public function newAction()
{
return ['headers' => [], 'body' => $this->render('newThing.html.php')];
}
public function createAction($post)
{
$violations = $this->model->validateThing($post);
if (empty($violations)) { // if valid
$this->model->insertThing($post);
return ['headers' => ['Location: /somewhere-else'], 'body' => ''];
} // <-- discourse indentation bug
// Otherwise, not valid
return ['headers' => [], 'body' => $this->render('newThing.html.php', ['entity' => $post, 'violations' => $violations])];
}
public function render($template, $params = [])
{
extract($params);
ob_start();
require $template; // <-- another discourse indentation bug
$content = ob_get_clean();
return $content;
}
}
model.php
<?php
class Model {
public function validateThing($entity)
{
$violations = [];
// Pretend both are required
if (!isset($entity['field1']) || $entity['field1'] === '') {
$violations[] = 'Field 1 is required.';
}
if (!isset($entity['field2']) || $entity['field2'] === '') {
$violations[] = 'Field 2 is required.';
}
// Pretend field 2 requires at least 3 chars
if (strlen($entity['field2']) < 3) {
$violations[] = 'Field 2 must have at least 3 characters.';
}
return $violations;
}
public function insertThing($entity)
{
// Pretend insert SQL
}
}
newThing.html.php remains unchanged
index.php
<?php
require_once 'controllers.php';
// Route
if ($_SERVER['REQUEST_URI'] === '/new' && $_SERVER['REQUEST_METHOD'] === 'GET') {
$controller = new Controller(new Model);
$response = $controller->newAction();
} else if ($_SERVER['REQUEST_URI'] === '/new' && $_SERVER['REQUEST_METHOD'] === 'POST') {
$controller = new Controller(new Model);
$response = $controller->createAction($_POST);
} else {
// 404; not implemented
exit;
}
// Send response
// Assumes response in the form of ['headers' => [...], 'body' => ...].
foreach ($response['headers'] as $header) {
header($header);
}
echo $response['body'];
Before continuing can you confirm you’re happy with this OOP version?
(Unlike certain other recent discussions here ahem, It’s refreshing to be debating someone who’s opinion I respect and has an excellent understanding of the topic in hand. This should be a very interesting discussion )
Almost. Let’s put the actions in their own classes so they can have their own dependencies, and let’s put render in its own object that also gets injected. I was slightly tempted to break the model into a validator object and a repository object, but it might not matter, so I left that alone for now.
##controllers.php
<?php
require_once 'model.php';
class NewController
{
private $templating;
public function __construct(Template $templating)
{
$this->templating = $templating;
}
public function doAction()
{
return ['headers' => [], 'body' => $this->templating->render('newThing.html.php')];
}
}
class CreateController
{
private $templating;
private $model;
public function __construct(Template $templating, Model $model)
{
$this->templating = $templating;
$this->model = $model;
}
public function doAction($post)
{
$violations = $this->model->validateThing($post);
if (empty($violations)) { // if valid
$this->model->insertThing($post);
return ['headers' => ['Location: /somewhere-else'], 'body' => ''];
}
// Otherwise, not valid
return ['headers' => [], 'body' => $this->templating->render('newThing.html.php', ['entity' => $post, 'violations' => $violations])];
}
}
class Template
{
public function render($template, $params = [])
{
extract($params);
ob_start();
require $template;
$content = ob_get_clean();
}
}
##index.php
<?php
require_once 'controllers.php';
// Route
if ($_SERVER['REQUEST_URI'] === '/new' && $_SERVER['REQUEST_METHOD'] === 'GET') {
$controller = new NewController(new Templating);
$response = $controller->doAction();
} else if ($_SERVER['REQUEST_URI'] === '/new' && $_SERVER['REQUEST_METHOD'] === 'POST') {
$controller = new CreateController(new Templating, new Model);
$response = $controller->doAction($_POST);
} else {
// 404; not implemented
exit;
}
// Send response
// Assumes response in the form of ['headers' => [...], 'body' => ...].
foreach ($response['headers'] as $header) {
header($header);
}
echo $response['body'];
I didn’t run these before posting, so crossing my fingers I didn’t leave a typo.
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:
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.
“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.
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 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:
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?
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 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.
I have various stages of completeness. Unfortunately most of my work involves updates to existing sites and when new sites are needed recently the deadlines been tight so I haven’t had a chance to play around making a new framework
I’m not sure if this is the case or not, I don’t really see why you’d get duplicated code in the model in web mvc to start with. What this does allow is removing duplicated code from controllers by putting more in the model.
You answered my line of thinking, which was, one of your arguments against the page controller type of architecture is, there can be a lot of code duplication in the controllers. With this different MVC architecture, concerns are split properly and classes are more responsible for what they should be doing, and thus, as I was thinking, there is less code duplication and more modularity.
One thing I think Symfony does well is the index.php (or app.php). It is so bare minimum.
//....omitted some calls for loading files and classes.
// Initialize the application
$kernel = new AppKernel($environment, $environment != 'prod');
$kernel->loadClassCache();
//$kernel = new AppCache($kernel);
$request = Request::createFromGlobals();
$response = $kernel->handle($request);
$response->send();
$kernel->terminate($request, $response);
And it is funny, it is procedural code in the end, isn’t it?
Should we look at index.php to get the routing out of it? How could the routing be made with little to no configuration? Would that be wise for a web application? I ask, because controlling the “slug” is always important for SEO. (at least a lot of people think it is).
This is a whole different topic with its own set of nuances and not really related to the MVC architecture insofar as a router should be fairly easily amended to work with either of the MVC implementations above
That said, getting it to work with zero configuration is the next step… which is pretty easy but requires a bit of renaming of things
class Model → class Create class CreateController → class FormController
If we want to automate the controller, we need to create a better contract between the model and controller by way of an interface. This way the controller can only work with objects that have the right API:
interface FormModel {
public function validateThing($data);
public function insertThing($data);
public function success();
}
The names here aren’t great but I’m building on what we have with as few changes as possible. Then have both the controller and model reference the interface:
class CreateModel implements FormModel {
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;
}
}
class CreateController {
private $model;
public function __construct(FormModel $model) {
$this->model = $model;
}
public function doAction($post)
{
if ($this->model->insertThing($post)) {
$this->model->success();
}
}
}
Then we can build a router that takes a route, e.g. /create and works out that it needs the model Create, FormController, and create.html.php
<?php
$name = trim($_SERVER['REQUEST_URI'], '/');
if (file_exists('models/' . $name . '.php')) {
require_once 'models/' . $name . '.php';
$model = new $name;
//Work out controller type by model type:
$controllerTypes = ['FormModel' => 'FormController'];
foreach ($controllerTypes as $interface => $controllerName) {
//Check the model's interface to work out the controller type
if ($model instanceof $interface) {
require_once 'controllers/' . $controllerName . '.php';
$controller = new $controllerName($model);
break;
}
}
if ($controller && $_SERVER['REQUEST_METHOD'] === 'POST') {
$controller->doAction($_POST);
}
$view = new View('templates/' . $name . '.html.php', $model);
}
else {
//404 not implemented
}
Obviously this is incredibly restrictive but should demonstrate how the basis of an automated router could work.
Once you have your automation set up and want some more control over URLs it’s simple enough to just add a little configuration at the top of the router:
$seoUrls = ['my-fancy-seo-url-for-new-item-creation', 'create'];
if (isset($seoUrls($name))) $name = $seoUrls[$name];
else {
//And then maybe block access to the original url to stop duplicate content issues:
if (isset(array_flip($seoUrls)[$name])) $name = null;
}
$name = trim($_SERVER['REQUEST_URI'], '/');
//as before
if (file_exists('models/' . $name . '.php')) {
Taking that a step further, the router could easily allow for requesting different content types:
<?php
list ($name, $type) = explode('.', trim($_SERVER['REQUEST_URI'], '/'));
if (file_exists('models/' . $name . '.php')) {
require_once 'models/' . $name . '.php';
$model = new $name;
//Work out controller type by model type:
$controllerTypes = ['FormModel' => 'FormController'];
foreach ($controllerTypes as $interface => $controllerName) {
//Check the model's interface to work out the controller type
if ($model instanceof $interface) {
require_once 'controllers/' . $controllerName . '.php';
$controller = new $controllerName($model);
break;
}
}
if ($controller && $_SERVER['REQUEST_METHOD'] === 'POST') {
$controller->doAction($_POST);
}
if ($type == 'html') $view = new View('templates/' . $name . '.html.php', $model);
else if ($type == 'pdf') $view = new PdfView('templates/' . $name . '.html.php', $model);
else if ($type == 'json') $view = new Json($model);
}
else {
//404 not implemented
}
Going to /create.html will give you html, going to create.json will give you json and create.pdf will give you a pdf representation of the model.
Again, it’s not perfect and open to abuse, not built in an extensible way but it should be food for thought
Indeed. I think this is worth a real-code comparison. I’m going to change the /new URL to be /new(.json | .xml)?, allowing for ajax, and the code must respond appropriately.
class CreateController
{
// ...
public function doAction($post, $format = 'html')
{
$violations = $this->model->validateThing($post);
if (empty($violations)) { // if valid
$this->model->insertThing($post);
if ($_SERVER['HTTP_X_REQUESTED_WITH'] !== 'XmlHttpRequest') { // if not ajax, redirect
return ['headers' => ['Location: /somewhere-else'], 'body' => ''];
} else { // if ajax, response according to format
return ['headers' => ['Content-Type: text/'.$format], 'body' => $this->templating->render('newThing.'.$format.'.php', ['success' => true])]
}
}
// Otherwise, not valid
return [
'headers' => ['Content-Type: text/'.format],
'body' => $this->templating->render('newThing.'.$format.'.php', ['entity' => $post, 'violations' => $violations])
];
}
}
For your refactored version, I suspect it would go like this (though please change it however you feel necessary; I think these side-by-side code comparisons will be much more useful than hand-wavy claims of better or worse).
I actually have a couple possible versions for this, and I’m not sure either is clearly best. This first has a separate route for each kind of format, because they each need to instantiate a different view class.
##index.php (separate routes)
} else if ($_SERVER['REQUEST_URI'] === '/new' && $_SERVER['REQUEST_METHOD'] === 'POST') {
$model = new Model;
$controller = new CreateController($model);
$controller->doAction($_POST);
$view = new HtmlView($model);
} else if ($_SERVER['REQUEST_URI'] === '/new.json' && $_SERVER['REQUEST_METHOD'] === 'POST') {
$model = new Model;
$controller = new CreateController($model);
$controller->doAction($_POST);
$view = new JsonView($model);
} else if ($_SERVER['REQUEST_URI'] === '/new.xml' && $_SERVER['REQUEST_METHOD'] === 'POST') {
$model = new Model;
$controller = new CreateController($model);
$controller->doAction($_POST);
$view = new XmlView($model);
} else {
But this can get tedious for this specific scenario where we want to support several formats. This second version, instead, has one route and an if-else to select the appropriate view class.
##index.php (combined route)
} else if (preg_match('#^/new(?:\.(json|xml))?$#', $_SERVER['REQUEST_URI'], $matches) && $_SERVER['REQUEST_METHOD'] === 'POST') {
$format = $matches[1];
$model = new Model;
$controller = new CreateController($model);
$controller->doAction($_POST);
if (!$format) {
$view = new HtmlView($model);
} else if ($format === 'json') {
$view = new JsonView($model);
} else if ($format === 'xml') {
$view = new XmlView($model);
}
} else {
But the downside with this version is that route definitions are supposed to be dumb, such that they can be expressed with a config file such as yaml or xml. But now this route contains actual logic and probably can’t be easily expressed through a config file anymore.
On the plus side, I genuinely did not need to touch the controller to support these formats, so when you say the view and controller are decoupled, you’re not wrong. On the downside, I had to touch the router instead. Ultimately the logic has to live somewhere. :-/
I think this is starting to exaggerate the degree to which your code is decoupled. Different form templates may require different arguments. The one we made here needed “entity” and “violations”. Some other form might also need, for example, “parentEntity” and “category”, values which may come from the URL or a cookie. Each of these form templates can’t work with any random model class. They need a model class that provides their particular properties. Likewise, a controller also can’t work with any random model class. Each model class will almost certainly have different methods to initialize its different properties.
The only information the controller needs to know are the arguments it must provide. Just like it needs to know the arguments the repository requires. Just like it needs to know the arguments the validator requires. Yes, it must also know the arguments the template requires.
Even though we’ve factored out the HTML formatting, you seem to be treating the mere invocation of render as repeated code. I think that’s taking things too far. That’s like… imagine you have three functions and some repeated code between them, so you factor out the repeated code into a function that the other three each invoke. But wait! Now that invocation itself is repeated code!.. Or so this argument seems to go.
Sorta yes, sorta no, I think. In Web MVC, a controller’s job is to produce a response from a request. For some pages that task may be complex, and for others it will be super simple.
In your refactored code, you were able to remove the controller class but you had to add a view class, so… six of one.
A lot of these later sections started making broad assumptions about how things would be implemented in Web MVC, assumptions which I don’t think are accurate, so if we want to dive into those specific scenarios, we’ll probably need to do more side-by-side code comparisons.
I have a couple responses to this one.
There’s a decent chance we’re trying to follow this principle a bit too zealously. Remember, the term “single responsibility principle” was coined by Robert Martin, same guy who also described the architecture we’re now calling Web MVC. So either Martin violated his own principle, or we’re going a bit overboard with it.
Nonetheless, let’s double check our possible reasons to change. The validation rules, the database insertion logic, the HTML formatting can all change without the controller having to change. But even though those things have been factored out, it seems as though you’re treating the mere invocation of that factored out code as a reason for change (e.g., updating the model, calling render). That seems to me to be going a bit overboard.
But I think this reinforces the need to settle this through code comparisons. The point of SRP is that “changes to one responsibility may impair or inhibit the class’ ability to meet the others.” So let’s find a feature or change that we think might expose such fragileness.
I’ve gone back and forth on this one myself. I used to also think that the view should be the whole of what we send to the browser – the whole HTTP message, headers and all. But one day it occurred to me that even the HTTP message still isn’t all of what the browser receives. What the browser actually receives are TCP packets. And I wondered that, if they were hypothetically under the control of our code, should they too be considered part of the view? But I don’t think so. TCP has nothing to do with what we think of as the view, and the same is true for most (but admittedly not all) of the HTTP headers. Headers such as cache control, or encoding (whether the response is gzipped), or cookies, or expires times, and many more, are not things the view cares about. So I’ve gone back to the conventional school of thought where the view is the HTTP body content.
####Some concluding thoughts
Supporting ajax and formats in Web MVC was admittedly a little awkward. But supporting them in your refactored version seems a little awkward too. Maybe all we did was move the awkwardness to a different place.
You think my NewController is redundant; I think your CreateController is redundant. :-p All the work of interpreting user input has already been farmed out to validators, forms, and repositories, so it doesn’t seem like this controller saved us from much, if any, repeated code.
I consider myself a pragmatist rather than an academic. I’m not judging these comparisons by how closely we follow a principle or adhere to a terminology, but by how useful it is in real situations. In the specific scenario we’ve compared so far, the refactored version seems (to me) a little harder to follow without adding substantial benefits. If we want to continue, I think it’s time to expand the scenario. I’ll come back and add an edit/update and delete actions.
I’ve always thought that the controller acts like a middle man. The index calls for the core controller. Then that controller checks to see what other controllers need to be required within the requests. The other controllers will then call forth the model if it needs to. Within the controller, it will also call the views. The model aka data will then populate the views which is required by the controller. The model will never touch the views at all. If you think about it, the data reaches the views, but the views will never be called in the model files.
That’s how I always thought it works. Am I wrong? Is this MVP?
Trying to keep everything boiled down to their very simplest to help zero in on the most crucial details. Certainly the templating, the validator, the repository, among others, could be made as injectable objects, but those aren’t the crucial details to Web MVC.
On the downside, I had to touch the router instead. Ultimately the logic has to live somewhere. :-/