Basic MVC Framework

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:

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');

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.

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

Model Base

```
db = Registry::getInstance()->get('database')->connect();
}
}

?>

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

StackExchange Code Review Q#127378, answer score: 2