PHP MVC - One view class for all controllers vs a view class for each controller

Something like…

In your controller should be a kind of…

$this->getResponse()->setToExcel($excelFile);

And response object makes the rest - required headers, readfile() and so on…

Attention! Self promotion!:grinning: I am senor PHP developer and that’s why have a lot of answers on questions you got.

That is my framework… https://github.com/igor1999/gi And I hope, there you find something interesting for you.

I`ll take a lot at your framework, thanks!

And i think i understand now why the view class shouldn’t output the view directly, and instead, it should return the view to the index.php file.

I still don’t understand about the headers completely, but at least i understand that index.php should be echoing out the view string. At least for now.

Thanks for the help with this!

One thing I would change for now is not making the Router the entry point for your application. I would create an Application class that instantiates the router and passes it the request. Initially that might be all it’s doing, but you’ll find other uses for the Application class as well.

Router just doesn’t sound like a top level concept, it should be part of your application, not the entry point.

Regarding headers, you could start with a very simple Response object for now, that just holds the view content, so you at least have something to extend later, without having to refactor everything from strings to a Response object. Once you get to the point you need to use headers, you’ll recognise it :wink:

class Response
{
    private $body;

    public function __construct(string $body)
    {
        $this->body = $body;
    }

    public function getBody(): string
    {
        return $this->body;
    }
}
1 Like

Nicked from the CodeIgniter Manual:

Yea, that`s a good point, i will do that. Even though i may not use headers right now, in the future it will at least be set up to allow the implementation of headers and other functionalities.

So in index.php, instead of doing:

$app = new Router();
echo $app->output;

I would do:

$app = new Application();

And the application class would instantiate the router, and the router would return a response to the application class. At this point the Application class will have the output string, should it be echoing it, or should index.php call something like:

$app = new Application();
echo $app->output;

Also, where exactly would the Response class come in?
Does the Application class instantiate the Response class and send the view string to the Response class, or does the Router instantiate the Response class and send the view string to it?

Thanks, that’s actually something that’s nice to keep in mind

2 Likes

well, i tried to implement what i understood from this topic, and from reading as many other sources as i could, and this is what i did:

index.php:

//Instantiate the application
$app = new Application();
$app->run();

application.php


class Application{
    private $request;
    private $router;
    private $response;
    private $dispatcher;


    public function __construct(){

        /*  Create a new request
        *   Handles the url and saves $controller, $action,
        *   and $params as properties. */
        $this->request = new Request();


        /*  Instantiate a new router class
        *   Uses the Request object, accesses its properties,
        *   and returns the correct controller, action, and params. */
        $this->router = new Router();
        $this->router->route($this->request);


        //Instantiate a new response
        $this->response = new Response();


        //Instantiate the dispatcher
        $this->dispatcher = new Dispatcher();
    }



    /* Run the application
    =================================================== */
    public function run(){

        /*  Call the run method from the dispatcher
        *   Takes the Router and Response objects for use in the method. */
        $this->dispatcher->run($this->router, $this->response);


        /*  Send the reponse
        *   Outputs the view. */
        $this->response->send();
    }
}

Im skipping the Request and Router at this point as i want to focus on the Response, Dispatcher, and Controller.

dispatcher.php

class Dispatcher{


    public function __construct(){}



    /*  Run
    *   Instantiates the controller, calls the required method, and sends the params.
    *   Passes on the Response object as a property to the controller.
    *   The core controller has functions to implement/update the Response object.
    =================================================== */
    public function run($router, $response){

        //Save the controller, action, and params to variables
        $controller = $router->controller;
        $action     = $router->action;
        $params     = $router->params;


        //If there is a method and it exists in the controller
        if($action && method_exists($controller, $action)){

            //Instantiate the controller
            $run = new $controller;


            /*  Save the Response object as a property in the controller
            *   Saves the object in the core controllers property. */
            $run->response = $response;


            //Call the method and send the params
            call_user_func_array([$run, $action], $params);


        /*  If no method is set, instantiate the controller and send the params
        *   Similar rules as above about the Response object. */
        }else{
            $run = new $controller($params);
            $run->response = $response;
        }
    }
}

Controller.php
This is one part that im really not sure about, but basically whats going on in this file is that im saving the Response object as a property, and in the renderView method, i get the view from the View Class, and save it to the Responses responseBody. Then i echo that out in the actual Response->send() method.

class Controller{
    public $reponse;

    /*  Render method
    *   Renders the view, accepts the file name to load ($viewName)
    *   and the data that should be accessible in the view.
    *   Saves the returned view to the Response objects responseBody property.
    *   This is echoed in the Response->send() method in the Application object.
    ===============================================*/
    protected function renderView($view, $viewData = []){

        //Instantiate a new view
        $view = new View($view, $viewData);


        //Save the view to the responseBody property
        $this->response->responseBody = $view->render();
    }
}

and finally, Response.php ( it only has two methods for now because that’s all i’m using at this point)

class Response{
    public $responseBody;


    public function __construct(){}



    /*  Save body
    *   Saves the view output string to the responseBody property.
    =================================================== */
    public function saveBody($responseBody){
        $this->responseBody = $responseBody;
    }



    /*  Send
    *   Outputs the view.
    =================================================== */
    public function send(){
        echo $this->responseBody;
    }
}

I actually just noticed that i’m not even using the saveBody() method in the Response class, because i’m saving the views output string to the responseBody property directly. I also left out other parts of code that don’t have to do with this topic.

I just finished this and everything is actually working surprisingly well, but i’m still not sure i got all of this correct. There are a few areas where i don’t know if what i did actually makes sense and is an efficient way of doing it, basically, all of it :sweat_smile:

What do you think about this?

The general setup looks awesome :+1:
There’s some nice separation of concerns, and the flow looks good.

I don’t really like the communication through public properties though as that couples classes together and can lead to hard to debug errors, I would suggest changing that to returning response objects/arrays.

Basically instead of keeping state in services and then passing those services for other services to read that state, keep state completely out of the services and pass state around as method arguments instead, using the output of one service as an input for the next.

Let’s go through all classes one by one.

Application

This looks fine, however I would not call $this->router->route() in the constructor, as it’s generally considered bad practice to actually do stuff in constructors, as you can never instantiate the object without stuff happening - which is fine in this case, but take for example a database class, if it connects to the database when you instantiate it, but then you never use it, you’ve made that connection for nothing.

Anothing thing is that I wouldn’t create a response in the Application yet, as a Response is an excellent candidate for an Immutable object. You create it once and then you never have to change it, makes it a lot easier to debug stuff.

So then the constructor would just be:

public function __construct() {
    $this->request = new Request();
    $this->router = new Router();
    $this->dispatcher = new Dispatcher();
}

For more flexibility I would create interfaces for the Router and the Dispatcher and optionally accept those in the constructor:

public function __construct(RouterInterface $router, DispatcherInterface $dispatcher) {
    $this->request = new Request();
    $this->router = $router ?: new Router();
    $this->dispatcher = $dispatcher ?: new Dispatcher();
}

That way you get the default classes when you do new Application() but you could override the Router or Dispatcher if you wanted to - or both.

These interfaces may look something like this:

interface Router
{
    public function route(Request $request): array;
}

interface Dispatcher
{
    public function dispatch(array $route): Response;
}

I would change the run method of the Application as well, but for that we’d need to discuss the Router and the Dispatcher first.

Router

You haven’t posted the code for your Router, but from the looks of it it takes Request as argument and then sets some public properties on the Router. I would recommend against that. What if you want to support sub-request at some point, how are you going to separate the two? It’s going to be a real mess.

Instead I would do what I suggested in my RouterInterface above and return an array with routing information from the Router.

Such an array may look like:

[
    'controller' => [FooBar::class, 'someAction'],
    'params' => [
        'foo' => 'bar',
    ]
]

And then instead of passing the entire Router to the dispatcher, you would only need to pass it this array, decoupling the implementation of the Router from the Dispatcher

Dispatcher

Since we’ve changed the router, we need to change the Dispatcher as well to take these changes into account:

public function run(array $route, Response $response)
{
    [$controller, $action] = $route['controller'];
    
    if (!method_exists($controller, $action)) {
         throw new RuntimeException('Unable to route '.$controller.'::'.$action.' - no such method exists');
    }

    // may want to think about constructor arguments for the controller here
    // maybe use a Dependency Injection Container
    $instance = new $controller();
    $instance->response = $response;

    call_user_func_array([$instance, $action], $params);
}

But, remember the bit about using an immutable response? That would the change this bit to look like this:

public function run(array $route): Response
{
    [$controller, $action] = $route['controller'];
    
    if (!method_exists($controller, $action)) {
         throw new RuntimeException('Unable to route '.$controller.'::'.$action.' - no such method exists');
    }

    // may want to think about constructor arguments for the controller here
    // maybe use a Dependency Injection Container
    $instance = new $controller();

    $response = call_user_func_array([$instance, $action], $params);
    return $response;
}

So now instead of passing the controller an existing Response we expect the Controller to create a Response and return it to us. Again, when you start doing sub-requests this will make your life a lot easier than trying to use one global Response object.

Response

To make the Response actually immutable, change it like so:

class Response{

    private $body;

    public function __construct(string $body)
    {
        $this->body = $body;
    }

    /*  Send
    *   Outputs the view.
    =================================================== */
    public function send() {
        echo $this->body;
    }
}

Controller

Now the controller changes to this:

protected function renderView(string $view, $viewData = []) {

    //Instantiate a new view
    $view = new View($view, $viewData);

    //Create and return a Response
    return new Response($view->render());
}

Application (Continued)

And lastly we can change the run method of the Dispatcher:

public function run() {
    /*  Call the run method from the dispatcher
     *   Takes the Router and Response objects for use in the method. */
    $response = $this->dispatcher->run($this->router->route($this->request), $this->response);

    /*  Send the reponse
     *   Outputs the view. */
    $response->send();
}

Additionally you could instantiate the Request outside of the Application and then pass it to the Application::run method instead.

And then that’s it :slight_smile:

Basically all these changes boil down to: instead of setting properties on objects and then use those properties in other objects, pass around result objects/arrays instead. That lowers coupling between classes and improves encapsulation. And as a result the code will be easier to understand, because the result of calling a method on a service is no longer dependent on the state of the service, so less information to keep track off. And also it will be much easier to test using automated testing.

1 Like

Note that with the setup above it’s easy to create several subclasses of Response for different kinds of responses.

Want to return JSON for an API? Create a JsonResponse:

public function JsonResponse extends Response
{
    private $data;
    public function __construct(array $data)
    {
        $this->data = $data;
    }
    public function send()
    {
        header('Content-Type: application/json');
        echo json_encode($this->data);
    }
}

and then in a Controller:

return new JsonResponse(['foo' => 'bar']);

Want to send a file to the browser? Sure, no problem:

// Warning: this class is not production ready as
// mime type guessing is notorious for being abused in all kind of ways
// and also there is no kind of virus checking etc
// this class is for illustration purposes only
class FileResponse extends Response
{
    private $filename;
    private $forceDownload;
    public function __construct(string $filename, bool $forceDownload = false)
    {
        $this->filename = $filename;
        $this->forceDownload = $forceDownload;
    }
    public function send()
    {
        header('Content-Type: '.mime_content_type($this->filename));
        if ($this->forceDownload) {
            header('Content-Disposition: attachment; filename="'.basename($this->filename).'"');
        }
        header('Content-Length: ' . filesize($file));
        readfile($this->filename);
    }
}

And the in the controller:

return new FileResponse('/path/to/some/excelfile.xls');

So now instead of one Response object that knows all kind of things about different response types, there are specialized classes for the different response types. That makes it a lot easier to see what will be returned, plus it’s impossible to mix response types.

1 Like

Ok, so i was sort of on the right track, that’s good to know!

I think i implemented most of what you said, and it does look less messy and more practical, i didn’t however implement the interfaces yet as i don’t really understand a lot about interfaces, i’m pretty new to OOP and classes and all of this and didn’t really get in to reading about interfaces yet.

I do have a few questions though about the rest which i implemented:

  1. $response = $this->dispatcher->run($this->router->route($this->request));
    Since now i am only passing the route() function instead of the whole object, would it not be better to also pass a request() function instead of the object Request to the route() function?
    Like this:
    $response = $this->dispatcher->run($this->router->route($this->request->handle()));
    And handle() would also return just an array.

  2. I set up the renderView() function to create a new response object and return it like you mentioned, but in my set up, i have the CoreController which is part of the framework, and then i have a BaseController, this controller extends the CoreController so that i can add more functions that are used by all of the apps controllers, then i have the actual app controllers like PostController.
    So the way it goes is PostsController extends BaseController, and BaseController extends CoreController. This way PostsController has everything in the CoreController and the “custom core controller” which is BaseController. The response object is created in the CoreController, which means that to return it back to the dispatcher, i have to return it from the CoreController, to the BaseController, to the PostsController, and then to the dispatcher. So my question is, is there a different way of doing this? I just seems like there will be a lot of returns throughout the whole process and may be more complex than it could be.
    For example, in my PostsController, i have a default method that gets called if there is no action in the url, and that default method sends to a method called list(). So, /posts and /posts/list end up going to the list() method. Since im sending the default method to the list method, this means ill have another extra return, so its CoreController->BaseController->PostsContsoller-list()->PostsController-defaultMethod()->dispatcher. Is this efficient?

  3. Since i am returning the response object to the PostsController for example, i could add another function in the Response class called saveHeader(), and in PostsController i would use this function to set headers, and then return the response object to the dispatcher. Is that correct or should there be separate functions or another way?

Good idea, but I think that would be taking it too far. Request is already one of the objects that we pass around that has state. No need to extract that even further. Also, Request should be immutable as well, so none of the method that gets it passed can change it, so you’re free to pass it around as you like.

Yes, that’s because of the entire inheritance tree. I wouldn’t use CoreController and a/or a BaseController at all. It works fine in the beginning, but you’ll inevitable run into a problem where you want to use something from either the CoreController or the BaseController but just slightly different. So you hack around it. Then the second things comes along, you hack around it. Before you know it it’s become an entangled mess that it very hard to untangle.

Instead I would use just the PostController, not extending anything, and then inject the services it needs through constructor injection. This does mean you’d need to introduce a dependency injection container. As a start I’d take a look at Aura.DI. I actually like Symfony’s Dependency Injection Container (DIC) better, but Aura is easier to get started with if you’ve never used Dependency Injection (DI).

Take a look at their docs here: http://auraphp.com/packages/2.x/Di.html

Or, since you’re rolling your own framework, you could of course roll your own :slight_smile:

Just these few layers won’t cause any noticable overhead. But as said above I’d do away with the entire inheritance tree.

The idea is correct, but since Response is supposed to be immutable, it should be a wither:

class Response {

    private $body;
    private $headers = [];

    public function __construct(string $body)
    {
        $this->body = $body;
    }

    public function withHeader(string $header, string $value): Response
    {
        $copy = new self($this->body);
        $copy->headers[$header] = $value;

        return $copy;
    }

    /*  Send
    *   Outputs the view.
    =================================================== */
    public function send() {
        foreach ($this->headers as $header => $value) {
            header(sprintf('%s: %s', $header, $value));
        }
        echo $this->body;
    }
}

And then in the PostController you could do (assuming you’re keeping the CoreController and BaseController here for the example’s sake):

public function someAction(): Response
{
    return parent::render($template, $values)->withHeader('Pragma', 'no-cache');
}

So basically you create a new Response and return that, so anyone who referenced the “old” Response doesn’t see anything changed - and doesn’t need to.

I can’t help with the details of PHP and MVC and since you have this solved or nearly solved there is not much for me to say. And the following might be totally irrelevant.

Based on my experience with OOP for desktop applications I would say that you would want a base class for the view and then derive classes for each actual view.

The specific template would be specified in the derived classes.

I am not familiar with PHP classes but the base class could have a virtual renderView method and then PostController could call that method on the instance of the derived classes.

If you can do things that way then it might be more OO.

PHP doesn’t have virtual methods, all non-final methods can be overridden.

Although in this case I would say an abstract method may be appropriate.

I’m too lazy to read whole code but…

$copy = new self($this->body);
$copy->headers[$header] = $value;

return $copy;

Wat!?

public function someAction(): Response
{
    return parent::render($template, $values)->withHeader('Pragma', 'no-cache');
}

Wat!?

foreach ($this->headers as $header => $value) {
         header(sprintf('%s: %s', $header, $value));
 }
  1. Some headers have no key, just text.
  2. header() has one more important param - replacement flag.

The relevant feature of a virtual function in C++ at least is that if a virtual function is called using an object of the base class but the object is an instance of the derived class then the the function of the derived class is called.

I did a little research and found What's the equivalent of virtual functions of c++ in PHP? - Stack Overflow. It says that in PHP all non-private functions are virtual. Maybe later I will confirm that PHP can be used to do what I am trying to suggest.

Sorry. All my comments about virtual functions are irrelevant since PHP is a Loosely Typed Language.

I ended implementing i think most of what you said, but i mostly learned a lot from all you wrote and this topic in general, so thanks for that, and to others who helped here as well!

I’m still learning all of this as i go, i guess by trial and error, so there is a lot here that i don’t understand at the moment but i’m sure will be useful for when i do understand it.

I’m think i’m pretty much settled on this topic at the moment and understand much better how the View part of this system is actually supposed to work and its connection in the triad, but you’ll probably see new questions here from me soon enough!

Since PHP 7.0, PHP is now a backwards compatible “loosely typed language”.

To prevent variable juggling add this statement at the start of every PHP file - because it does not apply to included files:

<?php
declare (strict_types=1);
error_reporting(-1); // maximum error reporting
ini_set('display_errors',  1); // produces error
ini_set('display_errors', true);// produces error
ini_set('display_errors', '1'); // valid and no errors
ini_set('display_errors', 'true'); // valid and no errors

Errors and warnings will be displayed immediately and require fixing before proceeding. Without type juggling the script will run just a little bit faster.

As stated before it is an immutable class. The point is that I can pass an immutable object anywhere without having to worry that the object I pass it to will modify it, since that’s impossible.

It’s very powerful paradigm that makes reasoning about code a lot easier and prevents loads bugs.

For example, consider:

<?php

function inRange(DateTime $first, DateTime $second, DateInterval $interval): bool
{
    return $first->add($interval) < $second;
}

$first = new DateTime();
$second = new DateTime('+2 day');
$interval = new DateInterval('P3D');

echo $first->format('Y-m-d H:i:s'), PHP_EOL; // 2019-06-10 09:59:52

inRange($first, $second, $interval);

echo $first->format('Y-m-d H:i:s'), PHP_EOL; // 2019-06-13 09:59:52

Here the DateTime I pass as $first is modified within the method, and since it’s passed by reference it’s modified outside the method as well!

Now, if I modify the function to use DateTimeImmutable instead of DateTime the problem goes away:

<?php

function inRange(DateTimeImmutable $first, DateTimeImmutable $second, DateInterval $interval): bool
{
    return $first->add($interval) < $second;
}

$first = new DateTimeImmutable();
$second = new DateTimeImmutable('+2 day');
$interval = new DateInterval('P3D');

echo $first->format('Y-m-d H:i:s'), PHP_EOL; // 2019-06-10 09:59:52

inRange($first, $second, $interval);

echo $first->format('Y-m-d H:i:s'), PHP_EOL; // 2019-06-10 09:59:52

$first is not modified inside the function anymore, since DateTimeImmutable::add() returns a new DateTimeImmutable instance, it doesn’t modify the passed one.

1 Like

My question is: why use you immutable class exactly in this situation? Immutable class used in case you need a lot instances of some class, as dates in your example. Do you need a lot of responses in pattern Middleware?

Says who?

I use an immutable class for the Response here because that way I can be sure classes aren’t messing with it. Whatever goes in is what comes out, and I don’t have to track through the entire application which classes are modifying it - they can’t.

And it’s a lot of test cases you don’t have to write, because if you don’t support mutability, you don’t have to check whether it works or not :slight_smile:

Lastly, any immutable class is referentially transparent by design, which makes it so much easier to reason about.