Small MVC for PHP

Question

The following is a new question based on answers from here: Small PHP MVC Template

I have written a small MVC template library that I would like some critiques on.

The library is located here

If you are looking for where to start, check out the files in the lib directory.

I would really love to hear your critiques on:

• Code quality
• Code clarity
• How to improve
• Anything else that needs clarification expansion etc

I'm more interested in what I'm doing wrong than right.
The README is not complete and there is a lot more functionality than what is documented, if you have any questions, please ask me.

Any opinions on the actual usefulness of the library are welcome.

---

Code examples (all of the code is equal, just on my last question I was asked for some snippets):

Bootstrap.php:

<?php
/**
 * The main library for the framework
 */
namespace SmallFry\lib; 
class_alias('SmallFry\lib\MySQL_Interface', 'SmallFry\lib\MySQL'); // MySQL class to be deprecated
/**
 * Description of Bootstrap
 *
 * @author nlubin
 */
Class Bootstrap {

    /**
     *
     * @var SessionManager
     */
    private $_session;
    /**
     *
     * @var stdClass
     */
    private $_path;
    /**
     *
     * @var AppController
     */
    private $_controller;
    /**
     *
     * @var Template
     */
    private $_template;

    /**
     *
     * @var Config
     */
    private $CONFIG;

    /**
     * @param Config $CONFIG 
     */
    function __construct(Config $CONFIG) {
        $this->CONFIG = $CONFIG;
        $this->CONFIG->set('page_title', $this->CONFIG->get('DEFAULT_TITLE'));
        $this->CONFIG->set('template', $this->CONFIG->get('DEFAULT_TEMPLATE'));
        $this->_session = new SessionManager($this->CONFIG->get('APP_NAME'));
        $this->_path = $this->readPath();
        $this->_controller = $this->loadController();
        $this->_template = new Template($this->_path, $this->_session, $this->_controller, $this->CONFIG); //has destructor that controls it
        $this->_controller->displayPage($this->_path->args);   //run the page for the controller
        $this->_template->renderTemplate($this->_path->args); //only render template after all is said and done
    }

    /**
     * @return \stdClass
     */
    private function readPath(){
        $default_controller = $this->CONFIG->get('DEFAULT_CONTROLLER');
        $index = str_replace("/", "", \SmallFry\Config\INDEX);

        //use strtok to remove the query string from the end fo the request
        $request_uri = !isset($_SERVER["REQUEST_URI"]) ? "/" : strtok($_SERVER["REQUEST_URI"],'?');;
        $request_uri = str_replace("/" . $index , "/", $request_uri);

        $path = $request_uri ?: '/'.$default_controller;
        $path = str_replace("//", "/", $path);

        $path_info = explode("/",$path);

        $page = isset($path_info[2]) ? $path_info[2] : "index";
        list($page, $temp) = explode('.', $page) + array("index", null);
        $model = $path_info[1] ?: $default_controller;

        $obj = (object) array(
            'path_info' => $path_info,
            'page' => $page,
            'args' => array_slice($path_info, 3),
            'route_args' => array_slice($path_info, 2),   //if is a route, ignore page
            'model' => $model
        );

        return $obj;
    }

    /**
     * @return AppController
     */
    private function loadController(){
        $this->CONFIG->set('page', $this->_path->page);

        //LOAD CONTROLLER
        $modFolders = array('images', 'js', 'css');

        //load controller
        if(strlen($this->_path->model) == 0) $this->_path->model = $this->CONFIG->get('DEFAULT_CONTROLLER');

        //find if is in modFolders:
        $folderIntersect = array_intersect($this->_path->path_info, $modFolders);

        if(count($folderIntersect) == 0){ //load it only if it is not in one of those folders
            $controllerName = "{$this->_path->model}Controller";
            $app_controller = $this->create_controller($controllerName); 
            if(!$app_controller)    {
                $route = $this->CONFIG->getRoute($this->_path->model);
                if($route)  {   //try to find route
                    $this->_path->page = $route->page;
                    $this->CONFIG->set('page', $route->page);   //reset the page name
                    $this->_path->args = count($route->args) ? $route->args : $this->_path->route_args;
                    $app_controller = $this->create_controller($route->controller);
                    if(!$app_controller) {
                        //show nothing 
                        header("HTTP/1.1 404 Not Found");
                        exit;
                    }
                }
                else    {
                    //show nothing 
                    header("HTTP/1.1 404 Not Found");
                    exit;
                }
            }
            return $app_controller;
        }
        else {  //fake mod-rewrite
            $this->rewrite($this->_path->path_info, $folderIntersect);
        }
        //END LOAD CONTROLLER
    }

    /**
     * @param string $controllerName
     * @return AppController 
     */
    private function create_controller($controllerName) {

        $useOldVersion = !$this->CONFIG->get('DB_NEW');
        $mySQLClass = "SmallFry\lib\MySQL_PDO";
        if($useOldVersion)  {
            $mySQLClass = "SmallFry\lib\MySQL_Improved";
        }
        //DB CONN
        $firstHandle = null;
        $secondHandle = null;
        //Primary db connection
        $database_info = $this->CONFIG->get('DB_INFO');
        if($database_info)  {
        $firstHandle = new $mySQLClass($database_info['host'], $database_info['login'], 
                   $database_info['password'], $database_info['database'], $this->CONFIG->get('DEBUG_QUERIES'));
        }
        else    {
            exit("DO NOT HAVE DB INFO SET");
        }

        //Secondary db connection
        $database_info = $this->CONFIG->get('SECONDARY_DB_INFO');
        if($database_info)  {
            $secondHandle = new $mySQLClass($database_info['host'], $database_info['login'], 
                                   $database_info['password'], $database_info['database'], $this->CONFIG->get('DEBUG_QUERIES'));
        }
        //END DB CONN

        $nameSpacedController = "SmallFry\\Controller\\$controllerName";
        if (class_exists($nameSpacedController) && is_subclass_of($nameSpacedController, __NAMESPACE__.'\AppController')) {  
            $app_controller  = new $nameSpacedController($this->_session, $this->CONFIG, $firstHandle, $secondHandle); 
        } else {
            return false;
        }
        return $app_controller;
    }

    /**
     * @param array $path_info
     * @param array $folderIntersect 
     */
    private function rewrite(array $path_info, array $folderIntersect){

        $find_path = array_keys($folderIntersect);
        $find_length = count($find_path) - 1;
        $file_name = implode(DIRECTORY_SEPARATOR,array_slice($path_info, $find_path[$find_length]));

        $file = \SmallFry\Config\DOCROOT."webroot".DIRECTORY_SEPARATOR.$file_name;

        if(is_file($file)){ //if the file is a real file
            header("Last-Modified: " . date("D, d M Y H:i:s", getlastmod()));
            include \SmallFry\Config\BASEROOT.DIRECTORY_SEPARATOR.'functions'.DIRECTORY_SEPARATOR.'mime_type.php'; // needed for setups without `mime_content_type`
            header('Content-type: ' . mime_content_type($file));
            readfile($file);
        }
        else {
            header("HTTP/1.1 404 Not Found");
        }
        exit;
    }
}

AppController.php:

<?php
namespace SmallFry\lib;
/**
 * Description of AppController
 *
 * @author nlubin
 */
class AppController {

    private $pageOn;
    protected $name = __CLASS__;
    protected $helpers = array();
    protected $validate = array();
    protected $posts = array();
    protected $session;
    protected $validator;
    protected $template;
    protected $CONFIG;

    /**
     *
     * @param SessionManager $SESSION
     * @param Config $CONFIG
     * @param MySQL_Interface $firstHandle
     * @param MySQL_Interface $secondHandle 
     */
    public function __construct(SessionManager $SESSION, Config $CONFIG, MySQL_Interface $firstHandle, MySQL_Interface $secondHandle = null) {

        $this->CONFIG = $CONFIG;
        $this->pageOn = $this->CONFIG->get('page');
        $this->session = $SESSION;
        $model_name = isset($this->modelName) ? $this->modelName : $this->name;

    /* Build the AppModel */
        $this->$model_name = &AppModelFactory::buildModel($model_name, $CONFIG, $firstHandle, $secondHandle);

    /* Get all posts */
        $this->posts = $this->$model_name->getPosts();

        $this->CONFIG->set('view', strtolower($model_name));

        if(!$this->session->get(strtolower($model_name))){
            $this->session->set(strtolower($model_name), array());
        }

    }

    private function getPublicMethods(){
        $methods = array();
        $r = new \ReflectionObject($this);
        $r_methods = $r->getMethods(\ReflectionMethod::IS_PUBLIC);
        $notAllowedMethods = array("__construct", "init", "__destruct"); //list of methods that CANNOT be a view and are `keywords`
        foreach($r_methods as $method){
            if($method->class !== 'SmallFry\lib\AppController' && !in_array($method->name, $notAllowedMethods)){ 
                //get only public methods from extended class
                $methods[] = $method->name;
            }
        }
        return $methods;
    }

    /**
     *
     * @param Template $TEMPLATE 
     */
    public function setTemplate(Template $TEMPLATE){
        $this->template = $TEMPLATE;
        $this->setHelpers();
    }

    /**
     * Function to run before the constructor's view function
     */
    public function init(){} //function to run right after constructor

    /**
     * Show the current page in the browser
     *
     * @param array $args
     * @return string 
     */
    public function displayPage($args)  {
        $this->CONFIG->set('method', $this->pageOn);
        $public_methods = $this->getPublicMethods();
        if(in_array($this->pageOn, $public_methods))    {  
            call_user_func_array(array($this, $this->pageOn), $args);
        }
        else    {
            if(!in_array($this->pageOn, $public_methods))   {
                header("HTTP/1.1 404 Not Found");
            }
            else {
                $this->CONFIG->set('method', '../missingfunction'); //don't even allow trying the page
                return($this->getErrorPage($this->CONFIG->get('view')."/{$this->pageOn} does not exist."));
            }
            exit;
        }
    }

    /**
     *
     * @return string 
     */
    function index() {}

    /**
     *
     * @param string $msg
     * @return string 
     */
    protected function getErrorPage($msg = null)    {
        $err = '<div class="error errors">%s</div>';
        return sprintf($err, $msg);
    }

    protected function setHelpers(){
        $helpers = array();
        foreach($this->helpers as $helper){
            $help = "{$helper}Helper";
            $nameSpacedHelper = "SmallFry\\helper\\$help";
            if(class_exists($nameSpacedHelper) && is_subclass_of($nameSpacedHelper, 'SmallFry\\helper\\Helper')){
                $this->$helper = new $nameSpacedHelper();
                $helpers[$helper] = $this->$helper;
            }
        }
        $this->template->set('helpers', (object) $helpers);
    }

    protected function logout(){
        session_destroy();
        header('Location: '.WEBROOT.'index.php');
        exit;
    }

    /**
     *
     * @param array $validate
     * @param array $values
     * @param boolean $exit
     * @return boolean 
     */
    protected function validateForm($validate = null, $values = null, $exit = true){

        $this->validator = new FormValidator(); //create new validator

        if($validate == null){
            $validate = $this->validate;
        }

        foreach($validate as $field => $rules){
            foreach($rules as $validate=>$message){
                $this->validator->addValidation($field, $validate, $message);
            }
        }

        return $this->doValidate($values, $exit);
    }

    protected function doValidate($values = null, $exit = true){
        if(!(!isset($_POST) || count($_POST) == 0)){
            //some form was submitted
            if(!$this->validator->ValidateForm($values)){
                $error = '';
                $error_hash = $this->validator->GetErrors();
                foreach($error_hash as $inpname => $inp_err)
                {
                  $error .= $inp_err.PHP_EOL;
                }
                return $this->makeError($error, $exit);                
            }
        }
        return true;
    }

    protected function makeError($str, $exit = true){
        $return = $this->getErrorPage(nl2br($str));
        if($exit) exit($return);
        return $return;
    }

    protected function killPage(){ //Throw a 404 for the page
        header("HTTP/1.1 404 Not Found");
        exit;
    }
}

Answer 1

A few things:

Firstly, It's not MVC. In MVC, controllers do not feed the view data. This is a common misconception but the architecture you're using here is closer to PAC. See http://www.garfieldtech.com/blog/mvc-vs-pac and http://r.je/views-are-not-templates.html for more info (Full disclosure: I wrote the second article.)

Secondly, your models aren't models they're simple data access and barely even that. They should at least abstract SQL away. You have domain logic, presentation logic and application logic all in the controller. In MVC the model should store application state (and have access to domain state), the controller should respond to user actions and the view should fetch the data from the model. See http://blog.astrumfutura.com/2008/12/the-m-in-mvc-why-models-are-misunderstood-and-unappreciated/ for a description of the model in MVC.

Thirdly, forcing models, views and controllers to extend from specific base classes heavily limits flexibility. Your controllers, views and models should implement interfaces instead of extending base classes. Although from experience, I have found that controllers and models don't even need to do that.

Finally, you should always favour Dependency injection rather than constructing objects arbitrarily throughout the code. See http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/ (and the rest of his very excellent site) for more information.

Sorry to sound very critical, but it's important to get the fundamental architecture correct before asking people to critique code clarity and quality.

Answer 2

There is plenty of stuff to criticize in your approach, and it boils down to the usual stuff:

1. Your classes do too much different stuff
2. Your classes do not implement proper dependency injection.
3. But if they do, they have very weird dependencies that will fit a certain use case, but not all.

Some details:

    $this->CONFIG->set('page_title', $this->CONFIG->get('DEFAULT_TITLE'));
    $this->CONFIG->set('template', $this->CONFIG->get('DEFAULT_TEMPLATE'));

I find it weird that the bootstrap class actually changes the configuration. Configuration to me is a read-only value storage. It gets written in the config object only once in the life cycle of the application request: When it's read from it's permanent storage.

The AppController for some weird reason has dependencies on a session manager, a configuration object, and TWO database connections. None of them should be the business of a controller. The controllers task is to be the combining layer between the incoming request on the input side, a number of models that act upon the data in the request as the processing step, and passing data back to the response as the output.

Analyzing the input data from the request usually is done by helping objects that represent HTML forms to do validation (none of the controllers business). The models usually make use of some database access or session, but this also is not for the controller to know. Preparing a response means to push some values into the answer, which might actually be rendered by a template, but this also is not really the business of the controller.

When I look at your AppController class, I see that it offers a whole lot of methods that have nothing to do with controller tasks, but with implementation details of a concrete application. I see methods like displayPage, validateForm, logout, that shouldn't be there. I especially wonder why logout() does not use the SessionManager to terminate the session.

It was mentioned before, but I want to underline that if you force all the application classes to be extended from your frameworks classes, you make it hard for others to use your framework. For example the Helpers - for some reason they have to be extended from your mother helper class. A much better approach would be to only force the implementation of an interface, and offer an abstract helper class that already has a basic implementation that can be extended if there is no need to build from scratch.