Basic MVC Framework

Question

I want to improve my skills so i startet a very basic MVC framework. Its my first MVC based Framework.

Index

I rewrited all requests through index.php and the file looks like this:

<?php

namespace System;

use System\Core\App;
use System\Libraries\Config;
use System\Libraries\Database;
use System\Libraries\Registry;

define('START', microtime(true));
define('ROOT', realpath($_SERVER['DOCUMENT_ROOT'] . '/../'));

spl_autoload_register(function ($class) {
    require_once ROOT . '/' . $class . '.php';
});

$registry = Registry::getInstance();
$registry->set('config', new Config);
$registry->set('database', new Database);

$app = new App;
$app->start();

?>

Registry

The Registry class is a singleton. I can set a name and value and access it in the whole project by using Registry::getInstance()->get('name of variable');

<?php

namespace System\Libraries;

class Registry
{
    private $services = [];
    private static $instance;

    public static function getInstance()
    {
        if (self::$instance === null) {
            self::$instance = new Self;
        }

        return self::$instance;
    }

    public function set($name, $value)
    {
        if (!in_array($name, $this->services)) {
            $this->services[$name] = $value;
        }
    }

    public function get($name)
    {
        if (array_key_exists($name, $this->services)) {
            return $this->services[$name];
        }

        return null;
    }

    private function __construct() {}
    private function __wakeup() {}
    private function __clone() {}
}

?>

Router

Its not the best routing but it does the job.

<?php

namespace System\Libraries;

use App\Controllers\Error;

class Router
{
    private $url = [];
    private $action = 'index';
    private $controller = 'index';

    public function __construct()
    {
        if (isset($_GET['url'])) {
            $this->url = filter_var(strtolower($_GET['url']), FILTER_SANITIZE_URL);
            $this->url = explode('/', rtrim($this->url, '/'));
        }

        if (isset($this->url[0])) {
            if (file_exists(ROOT . '/app/controllers/' . $this->url[0] . '.php')) {
                $this->controller = $this->url[0];
            } else {
                $this->controller = 'error';
            }
        }

        $this->controller = '\\App\\Controllers\\' . $this->controller;
        $this->controller = new $this->controller;

        if (isset($this->url[1])) {
            if (method_exists($this->controller, $this->url[1] . 'Action')) {
                $this->action = $this->url[1];
            } else {
                $this->controller = new Error;
            }
        }
    }

    public function dispatch()
    {
        unset($this->url[0], $this->url[1]);
        call_user_func_array([$this->controller, $this->action . 'Action'], array_values($this->url));
    }
}

?>

Controller Base

<?php

namespace System\Core;

class Controller
{
    public function loadModel($name)
    {
        if (file_exists(ROOT . '/app/models/' . $name . '.php')) {
            $model = '\\App\\Models\\' . $name;
            return new $model;
        } else {
            die('Failed to load model: ' . $name);
        }
    }

    public function loadView($name)
    {
        $file = ROOT . '/app/views/' . $name . '.html';

        if (file_exists($file)) {
            return new View($file);
        } else {
            die('Failed to load view: ' . $file);
        }
    }
}

?>

Model Base

<?php

namespace System\Core;

use System\Libraries\Registry;

class Model
{
    protected $db;

    public function __construct()
    {
        $this->db = Registry::getInstance()->get('database')->connect();
    }
}

?>

View

<?php

namespace System\Core;

class View
{
    private $view;

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

    public function assign($name, $value)
    {
        $this->{$name} = $value;
    }

    public function render()
    {
        ob_start();
        require_once $this->view;

        $content = ob_get_contents();

        ob_end_clean();
        echo $content;
    }
}

?>

Example Controller

<?php

namespace App\Controllers;

use System\Core\Controller;

class Welcome extends Controller
{
    public function indexAction($first = null, $last = null)
    {
        $view = $this->loadView('welcome/index');
        $view->assign('first', $first);
        $view->assign('last', $last);
        $view->render();
    }
}

?>

/app/views/welcome/index.html

Welcome, <?= $this->first . ' ' . $this->last; ?>

Thats all. Hope you can give me some tips, improvements or anything other.

Answer 1

View: Possible Bug / Security Issue

The assign method of the View class sets arbitrary fields. The problem here is that you already have a field - $view - which is later passed to require_once. This means if you ever assign something with the name view, you will overwrite this field.

If this is done accidentally, you will get buggy code, if $name is ever user-supplied, you will have a security issue. For example:

$v = new View("legitimateTemplateFile.html");
$v->assign($_GET['name'], $_GET['value']);
$v->render();

This wouldn't be safe. But when developing a framework, you want to make it as difficult as possible to create unsafe code, because someone will eventually write code just like the example above.

Checking Filenames

I would always check filenames for directory traversal. Of course, nobody should write code like loadView($_GET['template']), but I really wouldn't rely on that (there might even be a legitimate reason for this), so I'd just check it as defense in depth.

Misc

• Naming: In your View class, $view should be $templateFile to make clear that it is a file, not an instance of the view class.
• Try not to die, as it takes all possibilities away from the calling code. What if they want to display a custom error page? What if the model is optional, and they can recover if it's not loaded? Just throw an exception instead of dying.
• Don't reuse one variable for two different purposes. Either $this->controller is a name, or an instance, but not both, it's confusing.
• The error handling in the Router seems confusing and not ideal. Is error really a controller that always exist? What if this specific app doesn't have that? And can I even see the difference between a non-existing controller, and a non-existing method? It doesn't seem that way.
• What if $this->url[1] is not set? dispatch wouldn't work correctly. So the same thing should happen as if it were set, but the method didn't exist, right? (just with a different error message). Same goes for url[0], and $_GET['url'].