Yet another PHP MVC form validation

Question

I'm working on my own "MVC" framework for learning purposes and for personal use. I use the "MVC" expression as a concept not a concrete design pattern. So I refer to MVC as a "separation concept" not a concrete thing since (mainly in the world of web) the implementation can be different in every framework.

Main

The htaccess rewrite rule says that everything (which is not a file or a directory) is passed to the index.php (bootstrap). A Router instance is created and asked to process the URL and render the page.

Controller

The specific Controller instance is instantiated by and its method is called by the Router. The Router extract the controller and action name from the URL.

Example:

localhost/user/viewall

is "translated to":

$controller = new UserController();
$controller->viewall();

So the Controller is something like an entry point.

View

I have a concrete View class which is pretty simple, I can add, remove and get custom data which is used when the page is rendered, something like this:

public function setData($key, $value = null)
{
    if (!is_string($key)) {
        throw new Exceptions\TypeException("The \"key\" parameter must be a string");
    }

    $this->data[$key] = $value;
}

public function setFile($file)
{
    if (!is_string($file)) {
        throw new Exceptions\TypeException("The \"file\" parameter must be a string");
    }

    $this->file = $file;
}

public function render()
{
    if (empty($this->file)) {
        return "";
    }

    if (!empty($this->data)) {
        extract($this->data);
    }

    // dirty-hack: use output buffering so we can easily check if a file can be included
    // if not, simply reject the output and throw an exception (let the caller handle it)
    // if the inclusion was successfull then return with the buffered content 
    ob_start();
    if (!include($this->file)) {
        ob_end_clean();
        throw new Exceptions\FatalException("View file \"{$this->file}\" not found");
    }

    return ob_get_clean();
}

The "file" field is a "template file" which is currently a simple php file. This way it's possible to implement a custom template parser later on.

Model

The Model layer is the data representation and storage layer. I split up the persistence and representation. So I have a concrete class (like User) which represents a single user. I also have a UserService class which is used to add/remove/modify/query user to/from the database or memory or anything (because it's an abstract class, different implementations can be created).

Because I want to write less boilerplate code I've created a simple "class inspector" class. It can be used to get (and parse) the doc comment of a class, its functions and fields. I use this inspector in the base Model class to "generate" automatic getters and setters. This place can be used to final-validate data.

Code example:

public function __call($name, $args)
{
    // ...

    $getMatches = StringHelper::match($name, "^get([a-zA-Z0-9]+)$");
    if (!empty($getMatches)) {
        $property = lcfirst($getMatches[0][1]);

        if (!property_exists($this, $property)) {
            throw new Exceptions\FatalException("Cannot execute {$name}: property {$property} not found");
        }

        $metadata = $this->inspector->getPropertyMeta($property);
        if (!isset($metadata["@readonly"]) && !isset($metadata["@readwrite"])) {
            throw new Exceptions\FatalException("Cannot execute getter on write-only property: {$property}");
        }

        return $this->$property;
    }

    // ...
}

Putting it all together

The Router creates a new controller and call its action. The Controller creates the corresponding view instance and pass the model layer's data to it as a function parameter. The View instance then loads the required template file, set the data which is required by the template (strong coupling).

I also created a ServiceContainer class which is used by the controllers. Because the Controller is instantiated by the Router I cannot use direct dependency injection and this was my solution for this problem.

Example

Here comes a simple example of a one-way data flow. The UserController creates the UserView and call its viewUser(id) function which loads the required template, sets the "presentable" data and done.

// input url: localhost/user/view/12

// =========================
// index.php
// =========================

// create services
$services = new Framework\ServiceContainer();
{
    // TEST SERVICES
    //$services->add("user", new UserServiceMock());

    // REAL SERVICES
    $services->add("user", new UserServiceDB($db));
}

// create routes
$router = new Framework\Router($services);
{
    $router->addRoute("", "HomeController", "home");
}

// try to find a controller/action pair for this url and render the specified view
try {
    $router->dispatchAndRender(new PageNotFoundView());
} catch (Exception $ex) {
    die($ex->getMessage());
}

// =========================
// User.php
// =========================

class User extends Framework\Model
{
    /**
     * @readwrite 
     * @column
     * @primary
     * @type int
     */
    protected $id;
    /**
     * @readwrite
     * @column
     * @type text
     * @length 20
     */
    protected $name;

    // ...
}

// =========================
// UserService.php
// =========================

abstract class UserService
{
    public abstract function findAll();

    public abstract function findById($id);

    // ...
}

// =========================
// UserController.php
// =========================

class UserController extends Framework\Controller
{
    private $service;
    private $view;

    public function __construct($services)
    {
        $this->service = $services->get("user");

        $this->view = new UserView();
        $this->setView($this->view);
    }

    public function view($id)
    {
        $user = null;

        if (is_numeric($id)) {
            $user = $this->service->findById($id);
        }

        $this->view->viewUser($user);
    }
}

// =========================
// MainLayoutView.php
// =========================

class MainLayoutView extends Framework\View
{
    public function __construct()
    {
        $this->setFile("templates/MainLayout.php");
    }

    public function assignContent($contentView)
    {
        $this->setData("content", $contentView->render());
    }
}

// =========================
// UserView.php
// =========================

class UserView extends MainLayoutView
{
    public function viewUser($user)
    {
        $contentView = new Framework\View();
        $contentView->setFile("templates/User_View.php");

        if ($user) {
            $userData = array();

            $userData["id"] = $user->getId();
            $userData["name"] = $user->getName();
            // ... any other visible data

            $contentView->setData("user", $userData);
        }

        $this->assignContent($contentView);
    }
}

// =========================
// templates/User_View.php
// =========================

if (isset($user)) {
    echo "id: {$user['id']}<br />";
    echo "name: {$user['name']}<br />";
} else {
    echo "<b>User not found</b>";
}

Questions

1. As you can see I totally separated the Model from the View Template, so the actual rendering does not know about the data in the Model layer. This looks good for me because of the separation. I also sometimes want to preprocess the data before sending it to rendering (like creating the correct date format and so on) and this can be done in the View. Is this a good idea?

2. The example shows a "one-way data flow" but what's about the forms? I have a register() function in the UserController as well. It asks the UserView to read the form data (and check if it was sent at all). I've done this in the UserView because the UserController does not know about the concrete form. Where should I validate the data? Before HTML5 the input fields of a form are sent as simple strings without restrictions (except the radio buttons and these of course).

I would put the validation in the Controller. If I do that in the Controller, I can check the incoming input with a pattern (eg. allow only letters and numbers for a username), check the length of the string (but this is determined by the model, isn't it?) and the most important aspect is that I can use the Model and Service directly to validate the data (eg. do not allow duplicated username).

You can see the skeleton of the "validation" in the following code:

public function register()
{
    $formSent = $this->view->readRegisterForm($username, $password);

    if ($formSent) {
        $user = new User();

        // validate input (using the User instance?)
        // TODO
        $valid = false;

        if ($valid) {
            // hash password
            $password = password_hash($password, PASSWORD_DEFAULT);

            $user->setName($username);
            $user->setPassword($password);
            $this->service->add($user);

            $this->view->viewRegisterComplete($user);

            return;
        }
    }

    $this->view->viewRegisterForm();
}

Do you think it's a good idea to put the validation in the controller? My problem with the validation in the controller is that if the Model is (for some reason, so don't ask why) modified directly without using the Controller's function then the data wouldn't be validated.

Answer 1

General thoughts

• For a front controller-based application such as this, you should go ahead and make your redirect configuration in the server's .conf file instead of .htaccess and just turn off directory overrides. There is no reason to have the overhead of having to read .htaccess file introduced here.
• You say nothing about HTTP action verbs - GET, POST (and possibly PUT, DELETE, HEAD etc.) when talking about routing. I would think that this would be a critical part of route and controller instantiation in all but the most trivial applications.
• You may want to consider a more "RESTful" URL naming pattern. That would, for example have routes such as:

Example:

// Operate on collection
// get all users
GET /users/
// create new user
POST /users/
// queries against user collection
GET /users/?country=US

// Operate against specific user
// get specific user
GET /users/{id}
// modify specific user, could be POST if working with client that doesn't PUT
PUT /users/{id}
// get all things related to user, projects for example
GET /users/{id}/projects

• Not sure of the value of creating your own namespace full of exception types when base PHP exceptions type could probably do the trick (for example InvalidArgumentException vs. Exceptions\TypeException). If you want to keep your own exception types, should they be namespaced under the framework (i.e. Framework\Exceptions) or are these truly part of a standalone Exception library that you are wanting to introduce?
• Generally namespacing seems to be used inconsistently throughout. For example, should StringHelper be namespaced as part of the framework? Perhaps this is just my confusion as you did not include any namespace declarations as part of your code examples, so it just makes it unclear as to what your namespacing strategy is for the framework itself vs. the application built using the framework (in this case your User class).

View

public function setData($key, $value = null)
{
    if (!is_string($key)) {
        throw new Exceptions\TypeException("The \"key\" parameter must be a string");
    }

Good that you are attempting to validate this data, however just checking if parameter is string is probably not enough. You have edge cases you are not handling here for zero-length and trivially empty (i.e. sequence of spaces) string values and integer string representations (which PHP will convert to integer key). You might need to consider how you handle these if your expectation is to build an associative array here.

---

public function setFile($file)
{
    if (!is_string($file)) {
        throw new Exceptions\TypeException("The \"file\" parameter must be a string");

In your render function, you, in essence, deliver an empty output if file is not specified. I would question whether render should ever even be able to be called is $file value is not sent. As such, you might consider adding better validation here (similar to comments above for setData()), such that you can bail out of rendering process much earlier.

---

// dirty-hack: use output buffering so we can easily check if a file can be included
// if not, simply reject the output and throw an exception (let the caller handle it)
// if the inclusion was successfull then return with the buffered content 
ob_start();
if (!include($this->file)) {
    ob_end_clean();
    throw new Exceptions\FatalException("View file \"{$this->file}\" not found");
}

Agree this is a hack. Why not just use require which will raise fatal error if it is unsuccessful? If you really need exception-like behavior here, you can set your error handler to, in essence, catch these errors and convert to exception.

---

Should the controller that is instantiating this view really have to hold knowledge on where the view template is stored?

Model

I like your separation of single instance model from that of collection-level operations, though the name UserService perhaps seems inappropriate since I am assuming that class does not indeed expose a service. Perhaps UserProvider or similar is a more apt name.

---

I do not like the concept of reading doc blocks to create "getters" and "setters" in PHP. This sort of approach works well in compiled languages, where these doc block hints would actually be compiled into actual getter/setter methods, but in PHP this is going to potentially add a lot of operational overhead. PHP has it's __get() and __set() magic methods as the closest approximation to this behavior, and I would even question a framework introducing this potential operational overhead. This is not to mention that you might find this sort of approach harder to unit test (and I do hope you plan to write unit tests for your framework).

Your Example

$services->add("user", new UserServiceDB($db));

You should consider getting your service definitions from configuration that way you are not needing to change out this code to do things like add new services/domains or be able to instantiate mock dependencies. I would consider a pattern like:

$services = new \Framework\ServiceContainer($config);

or

$services = \Framework\ServiceProvider::getContainerFromConfig();

where you can instantiate your entire service container with applicable services based on config that does not live in this area of code.

---

// create routes
$router = new Framework\Router($services);
{
    $router->addRoute("", "HomeController", "home");
}

It is not really clear to me how the router relates to the services. Does each service has its own router, with a top-level router simply determining how to route to each services (i.e. it operates on the first segment of the URI) and then each service have configuration for the routing needed for the remaining part of the URI (the second and following URI segments + any parameter string)?

---

try {
    $router->dispatchAndRender(new PageNotFoundView());
} catch (Exception $ex) {
    die($ex->getMessage());
}

A couple of questions here.

• What is importance of PageNotFoundView()? It seems like an oddly named object to be passing to a method that is supposed to dispatch to a controller. What does the router have to do with passing along View objects?
• I wonder if the dispatchAndRender() method is appropriately named here. It may make it seem as if the router has control over the rendering, which it does not and likely should not. Perhaps just dispatch() is appropriate. After all, I would imagine this call could end up with any sort of outcome from 400 and 500 series errors, to an HTML page rendering, depending on how the controller interprets the request.
• Don't echo out raw error/exception messages to the caller. Log them. Then have the application do the appropriate thing in terms of generating error response (with appropriate HTTP error codes). I am guessing the logic should actually live in your controller, with there not being any try-catch here at all. That is saying that you should abdicate ALL responsibility to the controller to handle and interpret any underlying errors/exceptions and to generate the appropriate response. The dispatch() method call perhaps should not be expected to throw at all (because it handles all underlying exceptions), and could be the last line of code in the index.php file.

---

class UserController extends Framework\Controller
{
    private $service;
    private $view;

    public function __construct($services)
    {
        $this->service = $services->get("user");

        $this->view = new UserView();
        $this->setView($this->view);

• I am not quite following why the user controller needs access to the entirety of the ServiceContainer as a dependency. Why would it not be expect an object representing the user service only (or a mock of it) to be passed upon instantiation, with type hint enforcement for the class or interface required on the parameter? It makes no sense for this controller to have to go grab its own dependency from the collection of services when that collection of services is already established at the point that the dispatch to and instantiation of this controller happens.
• The last two lines in this code example are very convoluted. You set the view and the call setView() again on very next line? Why not just single call to $this->setView(new UserView());?

---

public function view($id)
{
    $user = null;

    if (is_numeric($id)) {
        $user = $this->service->findById($id);
    }

    $this->view->viewUser($user);
}

Should you REALLY be able to operate against a null value for $user? What happens if the value is not numeric? What happens if the value is numeric but a value like 3.14? You should be much stricter in validation of the parameter sent to this method. Make caller pass you an integer value. So you don't need 15-20 lines of guarding code at the beginning of this method to deal with all the edge cases you currently allow.

---

class MainLayoutView extends Framework\View
{
    public function __construct()
    {
        $this->setFile("templates/MainLayout.php");
    }

You seem to have a tendency to want to obscure the fact taht you are hardcoding configuration date into your classes. May hard-coding makes sense for your purpose, for example to enforce a 1:1 relationship between views and templates. But if that is the case, just make these a property or constant of the class. Don't hide is inside a setFile() method call in your constructor. There really is no reason at all for you to override your base class constructor at all if you just out this configuration where it belongs.

---

public function viewUser($user)
    {
        $contentView = new Framework\View();
        $contentView->setFile("templates/User_View.php");

        if ($user) {

• Again, an example of hard-coding a configuration (the user view template) in the middle of your code logic.
• If you truly require a valid user object to operate against, then enforce it with a type hint on the parameter. Right now your if ($user) conditional will operate on any truthy value passed. I could pass it a boolean and generate a slew of reference errors.

Your questions

1. I think decoupling is generally a good thing to strive for. I think the real thing you need to potentially revisit is your logical hierarchy of dependencies which seems to be router->controller->view->model which is probably not as appropriate as router->controller->model->view and may be what leads to your second question. Right now, you have your controllers directly instantiating the views, which should probably be instantiated through the model. After all, the model is where information on what properties an object has lives, with the view just being a representation of that model (perhaps in different view formats).

2. I think you are struggling to decide where input validation lies based on your current architecture. The validation should almost without a doubt live on the model, as again, this is where you encapsulate all the information on what an object is - its fields, it field formats, etc. With your current approach of having the controller instantiating the view directly, you now have no place in your code to all of sudden insert the data validation process. If your code was structured such that the controller instantiates an appropriate model, passing to it any instructions it needs from the request (i.e. what method is being called based on URI and/or HTTP action verb, what type of view is being requested based on HTTP Accept or similar, etc.), then your model does all the heavy lifting and injects data into the appropriate view to satisfy the controller's instruction.

Answer 2

(I'm sorry if posting an "answer" is worse than creating a new topic - this is of course not an answer but more like a process I've done based on the answer and some research. Please let me know if I should do this process differently, I'm fairly new here.)

I've used Mike Brant's answer, CakePHP and Symfony as references/suggestions. Please don't be confused with the namespaces, I have not included the namespace and using declarations in the codes below.

I have changed the "encapsulation" of the framework. Now I have a Framework\Application abstract class. It only has two public (final) functions: initialize and handle. I have also introduced a simple Framework\Request and Framework\Response class.

Now the goal is to create a Framework\Response object. This task is done by the controllers or by automatically if an exception was thrown.

As I said, the Framework\Application is an abstract class and the concrete webpage/application has to inherit from it, and implement its abstract functions. These functions are:

protected abstract function getRootPath();

protected abstract function getIncludeDirs();

protected abstract function loadConfigs($configuration);

protected abstract function loadComponents();

protected abstract function addServices($services);

protected abstract function addCustomRoutes($router);

protected abstract function createContentForException($statusCode, $statusText);

These functions are automatically called in the initialize() function. Both the initialize and handle functions are "wrapped" with a try-catch block. If an exception is thrown I ask the handler to handle it. (This could be refactored to a different class and also an event dispatcher could be added here.)

If an exception is thrown in the initialization step, a response is generated, sent and the script execiton is interrupted inmediately.

public final function initialize($debugMode)
{
    try {
        // ...
    } catch (\Exception $ex) {
        // something failed, create a response
        $response = $this->handleException($ex);
        // send the response then terminate the script
        $response->send();
        exit;
    }
}

The handle function is similar to this but it does not call send and exit. Instead it returns the Framework\Response object to the caller (which is the default flow when no exception was thrown).

Here is the handleException function:

private function handleException(\Exception $ex)
{
    $statusCode = Response::HTTP_INTERNAL_SERVER_ERROR;
    $response = new Response($statusCode);

    if ($ex instanceof Exceptions\HttpException) {
        $statusCode = $ex->getStatusCode();
        $response->setStatusCode($statusCode);
    }

    $statusText = $response->getStatusText();

    $content = $this->createContentForException($statusCode, $statusText);
    if (!is_string($content)) {
        // TODO: use default content instead of exception?
        throw new Exceptions\TypeException("Application::createContentForException must return with a string");
    }

    if ($this->debug) {
        // TODO: read this from config/template?
        $debugContent = "<b>Exception:</b> " . $ex->getMessage() . "<br />";
        $debugContent .= "<b>Stack trace:</b><br />";
        $debugContent .= "<pre>" . $ex->getTraceAsString() . "</pre>";

        $content = $debugContent . $content;
    }

    $response->setContent($content);
    return $response;
}

This way the application can react to an exception by drawing whatever it pleases. If the application wants to render a complex "page not found" view it can do that using the Application's createContentForException function. (Note again that this can be refactored but I'm happy with this right now.)

The controllers are now able to get the Framework\Request object, so it can use the GET method as well. And with the exception handling the controller is now able to throw custom "HTTP Exception" objects which are handled properly in the main application. If the exception is a subclass of the HttpException class, different HTTP status codes can be written to the response. Every exception which is not a subclass of the HttpException treated as a HTTP status code 500 (internal server error).

Anyway, the index.php is now only a few lines of code. Note that I have to use the require/include here because the autoloader is not yet created and initialized.

<?php
require_once("../application/Application.php");

$app = new Application();
$app->initialize(true);

$request = new Framework\Request();
$response = $app->handle($request);
$response->send();
?>

I think I've successfully improved the framework. I do not agree with everything that Mike Brant wrote in his answer but it was clearly useful. Probably some things could be done better and a lot of feature is not used at all, but whatever, this whole thing is for personal use.

As a last thing I'm following this concept of MVC. There are other ways to do it but I feel this "data flow" the best.