Small PHP MVC Template

Question

The following is a new question based on answers from here: Small PHP Viewer/Controller 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 smallFry/application 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.

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
/**
 * Description of Bootstrap
 *
 * @author nlubin
 */
Class Bootstrap {

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

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

    /**
     *
     * @return stdClass
     */
    private function readPath(){
        $path = isset($_SERVER["PATH_INFO"])?$_SERVER["PATH_INFO"]:'/'.Config::get('DEFAULT_CONTROLLER');

        $path_info = explode("/",$path);
        $page = (isset($path_info[2]) && strlen($path_info[2]) > 0)?$path_info[2]:'index';
        list($page, $temp) = explode('.', $page) + array('index', null);
        $args = array_slice($path_info, 3);
        $controller = $path_info[1] ?: Config::get('DEFAULT_CONTROLLER');
        return (object) array(
            'path_info'=>$path_info,
            'page'=>$page,
            'args'=>$args,
            'controller'=>$controller
        );
    }

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

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

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

        if(count(array_intersect($this->_path->path_info, $modFolders)) == 0){ //load it only if it is not in one of those folders
            $controllerName = "{$this->_path->controller}Controller";
            return $this->create_controller($controllerName); 
        }
        else {  //fake mod-rewrite
            $this->rewrite($this->_path->path_info);
        }
        //END LOAD CONTROLLER
    }

    /**
     * @return AppController
     */
    private function create_controller($controllerName) {
        if (class_exists($controllerName) && is_subclass_of($controllerName, 'AppController')) {  
            $app_controller  = new $controllerName($this->_session); 
        } else {
            //show nothing 
            header("HTTP/1.1 404 Not Found");
            exit;
        }
        return $app_controller;
    }

    /**
     *
     * @param array $path_info 
     */
    private function rewrite($path_info){
        $rewrite = $path_info[count($path_info) - 2];
        $file_name = $path_info[count($path_info) - 1];

        $file = DOCROOT."webroot/".$rewrite."/".$file_name;
        include DOCROOT.'/smallFry/functions/mime_type.php'; // needed for setups without `mime_content_type`
        header('Content-type: '.mime_content_type($file));
        readfile($file);

        exit;
    }
}

AppController.php:

<?php
/**
 * 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;

    /**
     *
     * @param SessionManager $SESSION
     */
    public function __construct(SessionManager $SESSION) {

        $this->pageOn = Config::get('page');
        $this->session = $SESSION;
        $model_name = $this->name;
        if(class_exists($model_name) && is_subclass_of($model_name, 'AppModel')){
            /**
             * @var AppModel $model_name
             */
            $this->$model_name = new $model_name();
        }
        else {
            //default model (no database table chosen)
            $this->$model_name = new AppModel();
        }
        /* Get all posts */
        $this->posts = $this->$model_name->getPosts();

        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);
        foreach($r_methods as $method){
            if($method->class !== 'AppController'){ //get only public methods from extended class
                $methods[] = $method->name;
            }
        }
        return $methods;
    }

    /**
     *
     * @param Template $TEMPLATE 
     */
    public function setTemplate(Template $TEMPLATE){
        $this->template = $TEMPLATE;
        $model_name = $this->name;
        $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)  {
        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(Config::get('view') == strtolower(__CLASS__) || 
                    !in_array($this->pageOn, $public_methods)){
                header("HTTP/1.1 404 Not Found");
            }
            else {
                Config::set('method', '../missingfunction'); //don't even allow trying the page
                return($this->getErrorPage(Config::get('view')."/{$this->pageOn} does not exist."));
            }
            exit;
        }
    }

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

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

    protected function setHelpers(){
        $helpers = array();
        foreach($this->helpers as $helper){
            $help = "{$helper}Helper";
            if(class_exists($help) && is_subclass_of($help, 'Helper')){
                $this->$helper = new $help();
                $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;
    }
}

Answer 1

I think your code is at a point where you need to stop worrying about it. It's not perfect, but it's way past the point of anything obvious.

The one thing I really don't like is your Config class. I get it that static is heplful, but you are abusing it a bit. You could:

1. Consider turning it into a Singleton, if you are so inclined in static state. Singletons are evil, but an improvement upon your code. All static classes are virtually un-testable (in the context of unit tests) and I can only describe them as a procedural code masquerading as an object. It's a style you don't want to adopt.

2. Consider getting the configuration files from a configuration file, and not requiring the developer to hardcode them.

   The simplest way would be ini files, PHP's support via parse_ini_file() is excellent. That way your configuration class could be used autonomously.

And, you seem to have ignored my advice on prefixing private members with an underscore. That hurts my feelings :)

Further suggestions (that are outside the scope of a code review):

1. PHPUnit is your best friend! Learn how to write unit tests, framework code especially should be 100% covered by unit tests.
2. PHP has support for interfaces and you should always program to an interface.

I honestly can't find anything else worth mentioning.