Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
    }
}
share|improve this question
    
Saw this on /r/php...you should read the fig standards wrt things like keyword case, prefixing underscores on properties, brace placement, using "exit", etc. At the least please try to maintain consistency with whitespace and formatting inside your own project. (sorry, wasn't trying to sound mean) –  Chad Retz Dec 13 '12 at 23:59
    
@ChadRetz the spacing got messed up somehow when I committed.... That was not my fault :-\ It does not look like that on my computer. Can you expand on your other comments? Just for a backstory, I had started out using this and then I expanded from there. –  Neal Dec 14 '12 at 14:13
add comment

2 Answers

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.

share|improve this answer
    
My classes are using Dependency Injection. Is there anything you can add on how I can improve my code? (forget the naming conventions, those can be changed) –  Neal Dec 13 '12 at 22:07
    
Occasionally you are, however things like : public final function addUseModel($usingModel) { $this->$usingModel = &AppModelFactory::buildModel($usingModel, $this->CONFIG, $this->firstHandle, $this->secondHandle); } are not DI. In DI you would pass in the fully constructed object instead of having a factory build it. The reason for this is flexibility: The appmodel (as you're calling it) should be agnostic about where its dependencies come from. –  Tom B Dec 13 '12 at 22:11
    
That buildModel function is only for when a model uses another model (or models). That way I can do: $this->ModelA->ModelB->getSomething(); (also side note: full code blocks do not show up well in comments like that, using pastebin or another bin site might fix that) –  Neal Dec 13 '12 at 22:12
    
How would I allow for a Model to use other models except for using that function? –  Neal Dec 13 '12 at 22:13
    
I'd argue that's bad practice in itself. See: misko.hevery.com/code-reviewers-guide/… any models which have a dependency on other models should define them as constructor (or method) arguments. –  Tom B Dec 13 '12 at 22:14
show 4 more comments

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.

share|improve this answer
    
Wow I did not realize "logout" was still there. That is from a very old version of the code when The SessionManager did not exist. –  Neal Dec 26 '12 at 20:30
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.