Small PHP Viewer/Controller template

Question

I have written a small template Viewer/Controller esque library that I would like some critiques on.

The library is located here

If you are looking for where to start, check out App.php and AppController.php in the classes folder

I would really love to hear your critiques on:

• Code quality
• Code clarity
• How to improve
• Anything else that needs clarification expansion etc
• I have also heard that my static methods (like my get and set methods in App) should be replaced with something more dynamic, How would I go about doing that?

I'm more interested in what I'm doing wrong than right.

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

App.php:

<?php
/**
 * Description of App
 *
 * @author nlubin
 */
class App {
    /**
     * Holds all of the app variables
     * @var array
     */
    private static $app_vars = array();
    /**
     * Will be an App object
     * @var App
     */
    private static $app = null;

    /**
     * Get a single app_vars variable
     * @param string $v
     * @return mixed 
     */
    public static function get($v){
        return isset(self::$app_vars[$v])?self::$app_vars[$v]:false;
    }

    /**
     * Get all app_vars variables
     * @return array app_vars
     */
    public static function getAll(){
        return self::$app_vars;
    }

    /**
     * Set an app_vars variable
     * 
     * @param string $v
     * @param mixed $va
     * @return mixed
     */
    public static function set($v, $va){
        if(self::$app == null){ //create App on first set. if not, the app does not exist
            self::$app = new self();
        }
        return self::$app_vars[$v] = $va;
    }

    /**
     * Clean up the app_vars variable
     */
    public static function clean(){
        self::$app_vars = array();
    }

    public function __construct() {
        $this->_connection = Database::getConnection();
    }

    private function render_template(){
        $rPath = $this->read_path();
        foreach($rPath as $key=>$value){
            $$key = $value;
        }
        unset($rPath);

        ob_start();

        App::set('page_title',App::get('DEFAULT_TITLE'));
        App::set('template',App::get('DEFAULT_TEMPLATE'));
        App::set('page',$page);

        //LOGIN
        if(!isset($_SESSION['LOGIN']) || $_SESSION['LOGIN'] == false){
            Login::check_login();
        }
        else {
            $modFolders = array('images', 'js', 'css');

            //load controller
            if(strlen($controller) == 0) $controller = App::get('DEFAULT_CONTROLLER');

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

        $main = ob_get_clean();
        App::set('main', $main);
        //LOAD VIEW
        ob_start();
        $this->load_view($app_controller, 0);
        //END LOAD VIEW

        //LOAD TEMPLATE
        $main = ob_get_clean();
        App::set('main', $main);
        $this->load_template($app_controller, $app_controller->get('jQuery'));
        //END LOAD TEMPLATE
    }


    private function read_path(){
        $path = isset($_SERVER["PATH_INFO"])?$_SERVER["PATH_INFO"]:'/'.App::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 = isset($path_info[1])?$path_info[1]:App::get('DEFAULT_CONTROLLER');
        return array(
            'path_info'=>$path_info,
            'page'=>$page,
            'args'=>$args,
            'controller'=>$controller
        );
    }

    private function create_controller($controllerName, $args = array()){
        if (class_exists($controllerName)) {  
            $app_controller  = new $controllerName(); 
        } else {
            //show nothing 
            header("HTTP/1.1 404 Not Found");
            exit;
        }
        echo $app_controller->display_page($args);
        return $app_controller;
    }

    private function load_template($controllerName, $jQuery = null){

        $page_title = $controllerName->get('title')?$controllerName->get('title'):App::get('DEFAULT_TITLE');
        //display output
        $cwd = dirname(__FILE__);
        $template_file = $cwd.'/../view/'.App::get('template').'.stp';
        if(is_file($template_file)){
            include $template_file;
        }
        else {
            include $cwd.'/../view/missingfile.stp'; //no such file error
        }
    }

    private function load_view($controllerName, $saveIndex){

        //Bring the variables to the global scope
        $vars = $controllerName->getAll();
        foreach($vars as $key=>$variable){
            $$key = $variable;
        }
        $cwd = dirname(__FILE__);
        if(App::get('view')){
            $template_file = $cwd.'/../view/'.App::get('view').'/'.App::get('method').'.stp';
            if(is_file($template_file)){
                include $template_file;
            }
            else {
                include $cwd.'/../view/missingview.stp'; //no such view error
            }
        }
        else {
            App::set('template', 'blank');
            include $cwd.'/../view/missingfunction.stp'; //no such function error
        }
    }


    private function rewrite($path_info){
        $rewrite = $path_info[count($path_info) - 2];
        $file_name = $path_info[count($path_info) - 1];

        $file = WEBROOT.$rewrite."/".$file_name;
//                echo $file; 
        header('Location: '.$file);
        exit;
    }

    public function __destruct() {
        $this->render_template();
    }
}

?>

AppController.php

<?php
/**
 * Description of AppController
 *
 * @author nlubin
 */
class AppController {

    /**
     *
     * @var mySQL
     */
    protected $_mysql;
    protected $_page_on,
            $_allowed_pages = array(),
            $_not_allowed_pages = array(
                '__construct', 'get', 'set', 
                'getAll', 'display_page', 'error_page',
                'include_jQuery', 'include_js', '_setHelpers',
                '_validate_posts', '_doValidate', '_make_error'
            );
    protected $app_vars = array();
    var $name = __CLASS__;
    var $helpers = array();
    var $validate = array();
    var $posts = array();
    protected $validator;

    public function __construct()   {
        $this->_mysql = Database::getConnection();
        $this->_page_on = App::get('page');
        App::set('view', strtolower($this->name));
        $this->_allowed_pages = get_class_methods($this);
        $this->set('jQuery', $this->include_jQuery());
        $this->setHelpers();
        $this->validator = new FormValidator();
        $this->_validate_posts();
        $this->posts = (object) $this->posts;
        if(!isset($_SESSION[App::get('APP_NAME')][strtolower($this->name)])){
            $_SESSION[App::get('APP_NAME')][strtolower($this->name)] = array();
        }
        return;
    }

    public function init(){

    }

    public function get($v){
        return isset($this->app_vars[$v])?$this->app_vars[$v]:false;
    }

    protected function set($v, $va){
        return $this->app_vars[$v] = $va;
    }

    public function getAll(){
        return $this->app_vars;
    }
    /**
     * Show the current page in the browser
     * @return string 
     */
    public function display_page($args)  {
        App::set('method', $this->_page_on);
        $private_fn = (strpos($this->_page_on, '__') === 0);
        if(in_array($this->_page_on, $this->_allowed_pages) 
                && !in_array($this->_page_on, $this->_not_allowed_pages)
                        && !$private_fn)    {  
            $home = $this->include_jQuery();
            call_user_func_array(array($this, $this->_page_on), $args);
        }
        else    {
            if(App::get('view') == strtolower(__CLASS__) || $private_fn ||
                    in_array($this->_page_on, $this->_not_allowed_pages)){
                header("HTTP/1.1 404 Not Found");
            }
            else {
                App::set('method', '../missingfunction'); //don't even allow trying the page
                return($this->error_page(App::get('view')."/{$this->_page_on} does not exist."));
            }
            exit;
        }
    }

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

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

    /**
     *
     * @return string 
     */
    protected function include_jQuery(){
        $ret = '<script type="text/javascript" src="js/jquery-1.6.2.min.js"></script>'.PHP_EOL;
        $ret .= '        <script type="text/javascript" src="js/jquery-ui-1.8.9.custom.min.js"></script>'.PHP_EOL;
        return $ret;
    }

    /**
     *
     * @param string $src
     * @return string 
     */
    protected function include_js($src){
        $script = '<script type="text/javascript" src="js/%s.js"></script>'.PHP_EOL;
        return sprintf($script, $src);
    }

    protected function setHelpers(){
        $helpers = array();
        foreach($this->helpers as $helper){
            $help = "{$helper}Helper";
            $this->$helper = new $help();
            $helpers[$helper] = $this->$helper;
        }
        self::set('helpers', (object) $helpers);
    }

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

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

    protected function _doValidate(){
        if(!(!isset($_POST) || count($_POST) == 0)){
            //some form was submitted
            if(!$this->validator->ValidateForm()){
                $error = '';
                $error_hash = $this->validator->GetErrors();
                foreach($error_hash as $inpname => $inp_err)
                {
                  $error .= "$inp_err<br/>\n";
                }
                $this->_make_error($error);                
            }

            foreach($_POST as $key=>$post){
                $this->posts[$key] = $post;
            }
        }
    }

    function __get($var_name){
//        echo $var_name."<br>";
        if(isset($this->posts->$var_name)){
            return $this->posts->$var_name;
        }
        else{
            ?><div class="errors"><?php
               echo "$var_name is not set<br/>\n";
            ?></div><?php
            exit;
        }
    }

    function __call($name, $arguments){
        if($name == 'mysql'){
            return (strlen($this->$arguments[0])==0?"NULL":"'{$this->$arguments[0]}'");
        }
    }

    function _make_error($str){
        ?><div class="errors"><?php
        echo $str;
        ?></div><?php
        exit;
    }
}

?>

Answer 1

I've spent about 20 minutes reading your code and I've identified several issues.

Relative paths

private function load_template($controllerName, $jQuery = null){
    $page_title = $controllerName->get('title') ? $controllerName->get('title') : App::get('DEFAULT_TITLE');

    //display output
    $cwd = dirname(__FILE__);
    $template_file = $cwd.'/../view/'.App::get('template').'.stp';
    if(is_file($template_file)){
        include $template_file;
    }
    else {
        include $cwd.'/../view/missingfile.stp'; //no such file error
    }
}   

What happens if at a later point you decide to change your directory structure? Instead of using dirname(__FILE__) you could just pass that base directory as a function parameter.

Function names inconsistencies

You prefix some private / protected variables and functions with an underscore:

protected $_mysql;

Two points:

1. Either do it for every private / protected variable and function or don't do it at all
2. Don't do it at all

Some justification on the second point:

• Every decent IDE out there allows for sorting of variables and functions based on their visibillity / scope. Eclipse goes a step further and uses green for public, and red for private / public in the Outline view, so everything is easily identifiable.
• In PHP functions that are prefixed with an underscore might be mistaken for magic methods (which are prefixed by two underscores).

Note that while almost all php IDEs will treat:

function foo() 

as:

public function foo()

you could help the IDE by adding the public keyword. That's also a readibillity point and a PHP4 style (function foo()) vs PHP5 style (public function foo()).

Inline HTML

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

protected function include_jQuery(){
    $ret = '<script type="text/javascript" src="js/jquery-1.6.2.min.js"></script>'.PHP_EOL;
    $ret .= '        <script type="text/javascript" src="js/jquery-ui-1.8.9.custom.min.js"></script>'.PHP_EOL;
    return $ret;
}

function __get($var_name){
    // echo $var_name."<br>";
    if(isset($this->posts->$var_name)){
        return $this->posts->$var_name;
    }
    else{
        ?><div class="errors"><?php
           echo "$var_name is not set<br/>\n";
        ?></div><?php
        exit;
    }
}   

Don't do that, you must separate presentation from logic.

Sessions

In AppController.php you utilize sessions:

if(!isset($_SESSION[App::get('APP_NAME')][strtolower($this->name)])){
    $_SESSION[App::get('APP_NAME')][strtolower($this->name)] = array();
}

But you don't have session_start() at the beginning of the script. Granted, you probably start the session in the caller script, but that means that your class will throw notices ($_SESSION array not initialized) if called from anywhere where session_start() hasn't been called. You can do something like:

if( !isset($_SESSION) ) session_start();

at the top of each script that uses sessions.

Comments

You have some phpDoc comments, you need one for every function. But please note that I've been accused of overcommenting :)

PHP4 style class members

You are using some PHP4 style class members:

var $name = __CLASS__;
var $helpers = array();
var $validate = array();
var $posts = array();

You should be using PHP5 access control (public / private / protected) members everywhere.

Ternary operator readability

This:

return isset(self::$app_vars[$v])?self::$app_vars[$v]:false;

is not very readable. Consider something like:

return
    isset(self::$app_vars[$v])
    ? self::$app_vars[$v]
    : false;

or at least something like this:

return isset(self::$app_vars[$v]) ? self::$app_vars[$v] : false;

Similarly, please add spaces before and after concatenation points:

$cwd.'/../view/'.App::get('template').'.stp'; // somewhat unreadable
$cwd . '/../view/'.App::get('template') . '.stp'; // friendlier to the eyes

Minor stuff

private function load_template($controllerName, $jQuery = null) {}

You don't utilize the $jQuery variable in the function. Is your code complete? If not, you shouldn't have posted it up for review.

function __get($var_name){
    // echo $var_name."<br>";
    if(isset($this->posts->$var_name)){
        return $this->posts->$var_name;
    }
    else{
        ?><div class="errors"><?php
           echo "$var_name is not set<br/>\n";
        ?></div><?php
        exit;
    }
}

else is not needed (as if block returns, which means that execution of the function stops there):

function __get($var_name){
    // echo $var_name."<br>";
    if(isset($this->posts->$var_name)){
        return $this->posts->$var_name;
    }

    ?><div class="errors"><?php
       echo "$var_name is not set<br/>\n";
    ?></div><?php
    exit;        
}

Here:

private function create_controller($controllerName, $args = array()){
    if (class_exists($controllerName)) { 

You treat $controllerName as a parameter that holds a class name, but in the very next function:

private function load_template($controllerName, $jQuery = null){
    $page_title = $controllerName->get('title')?$controllerName->get('title'):App::get('DEFAULT_TITLE');

you also have a $controllerName parameter that holds an object. Rename the second one to $controller.

In conclusion

Your code has a lot of minor inconsistencies, several readabillity issues, a few major problems (especially the html thingy) and it's clearly still a work in progress (especially AppController.php). You need to try and at least rewrite it in a more consistent matter.

PS. You should remove ?> from the end of every class script, as leo suggests.

Answer 2

Your code looks acutally great, and it has also good PHP-Doc, but I think the comments inside the methods are a bit confusing, because they have always a different writing style (sometimes uppercase, sometimes lowercase, sometimes sentences sometimes just simple words etc.), so you should cleanup them and set a convention how you document these points in the code.

Also you should be a bit carefully with the foreach loop in load_view, because there you set $$key = $variable;. If you know what you are setting to the controller, this isn't anything bad, but what is if you try to set $this? I didn't try it, but it would actually generate an error, so I would recommend you just to create an array of reserved variables, which will be checked before loading the view. There you should also add the $cwd and the $template_file, perhaps there are more vars like these.

Also something I actually see in your code, what is just a bit of coding convention, is the closing PHP tag at the end of the file (?>). Of course I see many developers using that right now, but I recommend you to leave that away, you could instead just add a comment like /* End of File */. The problem is, if you have a space after the closing PHP Tag and you try to modify the header later, it won't work, you'll just get errors.

And the replacement of your get and set methods could be done with PHP magic methods.. You should use __get($var) and __set($var) than you could access them like normal object properties. E.g. $this->newTemplateVar = "Hello World!";.

I hope my post helps you; if not, just say it.

Answer 3

I agree with yannis. But 1 last thing.

Constructors are simply supposed to assign values to properties. Your constructor seems to do a lot more than that. Switch it up to accept a database connection. Remember statics are evil cause they make testing difficult.

$Settings   = new FileSettings(new File('Config.php'));
$MFactory   = new ModelFactory;
$TFactory   = new TemplateFactory($Settings['UI']['Template']);

$Database   = $MFactory->createDB($Settings['DB']['Driver'], ....);
$Request    = $MFactory->createRequest($_SERVER['PHP_SELF'], $_GET, $_POST);
$View       = $TFactory->createView($Request->getView());

$Controller = new Controller($Database, $TFactory, $MFactory);
$Action     = $Request->getAction()?: 'index';

if (is_callable(array($Controller, $Action))) {
    call_user_function(array($Controller, $Action));
}

else $Controller->error($Request);