Small PHP-MVC system (for personal learning / use)

Question

I've made an "simple" / "small" MVC-like PHP system. I've used some code of an project I used before and ask questions about it back then to.

The thing is, it works (the admin part to begin with), but I'm not sure about the quality of the code. I know that there are mainly no comments, but they will be added when I'm sure that I stick with the current version.

Here's an link to an Github page containing all the source code

This is /admin/index.php

<?php

session_start();

require_once 'autoload.php';

// Initialize the log class which sets the error logger
$log = new Log();

$router = new Router();
$app = $router->startRouting();

echo $app;

/admin/autoload.php

<?php

require_once 'config.php';
require_once 'filters.php';

/*****************
 *               *
 *   Libraries   *
 *               *
 *****************/
require_once DIR_LIB . 'BaseController.php';
require_once DIR_LIB . 'BaseModel.php';
require_once DIR_LIB . 'db.php';
require_once DIR_LIB . 'language.php';
require_once DIR_LIB . 'log.php';
require_once DIR_LIB . 'minify.php';
require_once DIR_LIB . 'router.php';
require_once DIR_LIB . 'view.php';

/***************
 *             *
 *   Helpers   *
 *             *
 ***************/
require_once DIR_HELPER . 'func.php';
require_once DIR_HELPER . 'url.php';

/**************
 *            *
 *   Vendor   *
 *            *
 **************/
require_once '../vendor/autoload.php';

/DIR_LIB/router.php

<?php

class Router
{
    private $get, $post, $request, $cookie, $server, $routeExp;

    public function startRouting()
    {
        $this->routeExp = explode("/", rtrim((isset($this->get['_route_']) ? $this->get['_route_'] : "home/index"), "/"));

        if (!isset($this->routeExp[1])) {
            $this->routeExp[1] = "index";
        }

        // Let's start the routing filter
        Filter::route($this->routeExp);

        if (!file_exists(DIR_CTRL . $this->routeExp[0] . ".php")) {
            $view = new View();
            echo $view->generate404();
            die();
        }

        $app = $this->loadController($this->routeExp[0]);

        if (!method_exists($app, $this->routeExp[1])) {
            trigger_error("Error: Could not load method " . $this->routeExp[1] . " inside class " . ucfirst($this->routeExp[0] . 'Controller'));
        }
        $method = $this->routeExp[1];

        return $app->$method($this->getParams());
    }

    private function loadController($name)
    {
        require_once DIR_CTRL . $name . '.php';
        $controllerName = ucfirst($name) . "Controller";
        return new $controllerName();
    }

    private function getParams()
    {
        $params = $this->routeExp;
        unset($params[0], $params[1]);
        return array_values($params);
    }

    /**
     * This construct will make all incoming data "clean"
     */
    public function __construct()
    {
        $_GET = $this->clean($_GET);
        $_POST = $this->clean($_POST);
        $_REQUEST = $this->clean($_REQUEST);
        $_COOKIE = $this->clean($_COOKIE);
        $_SERVER = $this->clean($_SERVER);

        $this->get = $_GET;
        $this->post = $_POST;
        $this->request = $_REQUEST;
        $this->cookie = $_COOKIE;
        $this->server = $_SERVER;
    }

    /**
     * This recursive function will make the input clean through htmlspecialchars()
     *
     * @param $data array|string
     * @return array|string
     */
    private function clean($data)
    {
        if (is_array($data)) {
            foreach ($data as $key => $value) {
                unset($data[$key]);
                $data[$this->clean($key)] = $this->clean($value);
            }
        } else {
            $data = htmlspecialchars($data, ENT_COMPAT, 'UTF-8');
        }

        return $data;
    }
}

/filters.php

<?php

class Filter{

    private static $route;

    // Filter at route level
    public static function route($routeExp){
        self::$route = $routeExp;

        // Make sure that the admin is logged
        if(!isset($_SESSION['isAdmin']) || $_SESSION['isAdmin'] != true){
            if(self::$route[0] != 'login'){
                Url::navigate('login');
            }
        }
    }

    public static function beforeViewing(){

    }
}

These are some important files, but if you want me to show some more files, let me know.

Anything that could help / guide me to improve the code would help me a lot. I'm still rather new to this whole code separation trough OOP / MVC-like thing and want to learn it in the best way possible.

Answer 1

I don't have all that many complaints (other people didn't seem to have many either), so just a couple of small points.

I generally like your code. Your styling is clean and (mostly) consistent, and your variable/function naming is mostly good, which both really helps readability. There also do not seem to be any major errors or antipatterns in your code.

Security

• When calling require/include, I would always check if the normalized given path/name is inside an expected root directory (in this case inside the controller directory). Even if only PHP files can be included - in this case probably only under Windows - directory traversal should still be forbidden.
• clean doesn't really make the data clean, but dirty. It does provide quite a bit of security, but can lead to usability issues later on, which - when you try to solve them - can easily lead to security issues. Applying htmlspecialchars to every input may cause problems with passwords, with displaying data outside of HTML, with allowing some HTML code (by certain users), etc. This may seem ok now, but you may later want to remove this function - in which case you would have to go back and apply it everywhere where echoing things (which makes it very likely that you will overlook it somewhere).

Naming

• Filter doesn't actually filter anything (I would have expected it to filter out .. or similar). It's actually responsible for authorization, so the name should reflect that.
• routeExp: I don't like shortening variable names, so I would write routeExpression. But is the Exp part really relevant? I don't think so, just route should be fine.

Structure

Your Router does too much. It routes, but it also parses input, cleans it, and stores it. This is not something I would expect from a router.

Routing

I'm a bit confused as to your routing actually works. You seem to retrieve the route from a GET parameter called _route_, and then you get the parameters from that value as well (minus the first two)?

So I would use it like this:

/index.php?_route_=foo/bar/1/

Which would call the function bar inside the controller foo with argument 1?

The more standard way of doing this would probably be to parse the URL, so it would look like this:

/index.php/foo/bar/1/

or just

/foo/bar/1/