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'].

• TMH: Could you elaborate on your last bullet point? If $this->url[1] wasn't set, wouldn't dispatch just default to "index"?
• tim: @TomHart oh, yes. I still think it would be better to handle a missing action/controller the same way as a non-existing one (eg throw exception), but you are right, it works as it is.
• TMH: @tim I was racking my brain thinking "What have I missed here!?" haha! I'm not entirely sure I'd agree, with an MVC I'd personally expect that not setting an action that it would just default to "index", but that's just me with my limited MVC experience.
• tim: @TomHart Sorry about the confusion :) And yes, it should definitely happen somewhere; If a user visits example.com, they shouldn't get an error, that's true; I just don't like the idea that it happens silently in the router. I would probably introduce an explicit routing table that does that (but that's a bit more complex than the basic approach of the OP), or just do it via url rewrites. But it's mostly a matter of taste.