A PHP MVC working with Mustache (and now nested templates!)

Question

I'd love some feedback about this code that I'm editing and shortening now.

<?php
namespace bbn\cls;
class mvc 
{
    use \bbn\traits\info;
    // From \bbn\traits\info.
    protected static $info=array();
    protected static $cli=false;
    // Is set to null while not routed, then false if routing was unsucessful, and 1 if routing was done.
    private $is_routed;
    // Is set to null while not controled, then false if controller was not found, and 1 otherwise.
    private $is_controled;
    // The name of the controller.
    private $dest;
    // The path to the controller.
    private $path;
    // The controller file (with full path)
    private $controller;
    // The mode of the output (dom, html, json, txt, xml...)
    private $mode;
    // The data model
    public $data;
    // The output object
    public $obj;
    // The file extension of the view
    public $ext;
    // The request sent to the server to get the first controller.
    public static $original_request;
    // The first controller to be called at the top of the script.
    public static $original_controller;
    // The list of used controllers with their corresponding request, so we don't have to look for them again.
    public static $known_controllers = array();
    // The list of views which have been loaded. We keep their content in an array to not have to include the file again. This is useful for loops.
    private static $loaded_views = array();
    // Mustage templating engine.
    private static $mustache;
    // Reference to $appui variable
    private static $appui;
    // List of possible outputs with their according file extension possibilities
    private static $outputs=array('dom'=>'html','html'=>'html','image'=>'jpg,jpeg,gif,png,svg','json'=>'json','pdf'=>'pdf','text'=>'txt','xml'=>'xml','js'=>'js');
    /**
     * List of possible and existing universal controller. 
     * First every item is set to one, then if a universal controller is needed, self::universal_controller() will look for it and sets the according array element to the file name if it's found and to false otherwise.
     * @var array
     */
    private static $ucontrollers=array('dom'=>1,'html'=>1,'image'=>1,'json'=>1,'pdf'=>1,'text'=>1,'xml'=>1,'js'=>1);
    // Path to the controllers.
    private static $cpath='mvc/controllers/';
    // Path to the models.
    private static $mpath='mvc/models/';
    // Path to the views.
    private static $vpath='mvc/views/';
    // Path to the outputs.
    private static $opath='mvc/_output/';

    /**
     * @return void 
     * This function is called once and for all for each request and create a new mustache engine
     */
    private static function init()
    {
        if ( !isset(self::$appui) )
        {
            global $appui;
            self::$appui =& $appui;
            self::$mustache = new \Mustache_Engine;
            // Sets the mode, which is the unique (so static) way of final output
            if ( count(self::$appui->params) > 0 && isset(self::$outputs[self::$appui->params[0]]) )
            {
                self::$appui->mode = self::$appui->params[0];
                array_shift(self::$appui->params);
            }
            else if ( count(self::$appui->post) > 0 )
                self::$appui->mode = 'json';
            else
                self::$appui->mode = 'dom';
            self::$original_request = implode('/',self::$appui->params);
        }
        return self::$original_request;
    }

    /**
     * This checks whether an argument used for getting controller, view or model - which are files - doesn't contain malicious content.
     *
     * @param string $p The request path <em>(e.g books/466565 or html/home)</em>
     * @return bool
     */
    private static function check_path($p)
    {
        return ( strpos($p,'./') === false && strpos($p,'../') === false && strpos($p,'/') !== 0 );
    }

    /**
     * This fetches the universal controller for the according mode if it exists.
     *
     * @param string $c The mode (dom, html, json, txt, xml...)
     * @return string controller full name 
     */
    private static function universal_controller($c)
    {
        if ( !isset(self::$ucontrollers[$c]) )
            return false;
        if ( self::$ucontrollers[$c] === 1 )
            self::$ucontrollers[$c] = @file_exists(self::$cpath.$c.'.php') ? self::$cpath.$c.'.php' : false;
        return self::$ucontrollers[$c];
    }

    /**
     * Adds the newly found controller to the known controllers array, and sets the original controller if it has not been set yet
     *
     * @param string $c The name of the request or how set by the controller 
     * @param file $c The actual controller file ($this->controller)
     * @return void 
     */
    private static function set_controller($c, $f)
    {
        if ( !isset(self::$known_controllers[$c]) )
            self::$known_controllers[$c] = $f;
        if ( is_null(self::$original_controller) && !empty($c) )
            self::$original_controller = $c;
    }

    /**
     * This directly renders content with arbitrary values using the existing Mustache engine.
     *
     * @param string $view The view to be rendered
     * @param array $model The data model to fill the view with
     * @return void 
     */
    public static function render($view, $model)
    {
        self::init();
        return self::$mustache->render($view,$model);
    }

    /**
     * This will call the initial routing with the path in appui->params.
     * This constructor is chainable
     *
     * @param string $path
     * @return void 
     */
    public function __construct($path='')
    {
        $this->route(empty($path) ? self::init() : $path);
        return $this;
    }

    /**
     * This looks for a given controller in the file system if it has not been already done and returns it if it finds it, false otherwise.
     *
     * @param string $p
     * @return void 
     */
    private function get_controller($p)
    {
        if ( !$this->controller )
        {
            if ( !is_string($p) || !is_dir(self::$cpath.$this->mode) )
                return false;
            if ( isset(self::$known_controllers[$p]) )
            {
                $this->dest = $p;
                $this->controller = self::$known_controllers[$p];
            }
            else if ( file_exists(self::$cpath.$this->mode.'/'.$p.'.php') )
            {
                $this->dest = $p;
                $this->controller = self::$cpath.$this->mode.'/'.$p.'.php';
                self::set_controller($p,$this->controller);
            }
            else if ( is_dir(self::$cpath.$p) && file_exists(self::$cpath.$p.'/'.$this->mode.'.php') )
            {
                $this->dest = $p;
                $this->controller = self::$cpath.$p.'/'.$this->mode.'.php';
                self::set_controller($p);
            }
            else
                return false;
        }
        return 1;
    }

    /**
     * This looks for a given controller in the database
     *
     * @param string $path The request path <em>(e.g books/466565 or xml/books/48465)</em>
     * @return void 
     */
    private function fetch_route($path='default')
    {
        if ( self::check_path($path) )
        {
            global $bbn;
            return $bbn->db->query("
                SELECT route
                FROM controllers
                WHERE path LIKE ?
                AND output LIKE ?",
                $path,
                $this->mode)->fetchColumn();
        }
        return false;
    }

    /**
     * This will fetch the route to the controller for a given path, using fetch_route() and get_controller()
     *
     * @param string $path The request path <em>(e.g books/466565 or xml/books/48465)</em>
     * @return void 
     */
    private function route($path='')
    {
        if ( !$this->is_routed && self::check_path($path) )
        {
            $this->is_routed = 1;
            if ( strpos($path,'/') !== false )
            {
                $t = explode('/',$path);
                if ( isset(self::$outputs[$t[0]]) )
                {
                    $this->mode = array_shift($t);
                    $path = implode('/',$t);
                }
            }
            $this->path = $path;
            if ( !$this->mode )
                $this->mode = self::$appui->mode;
            $this->ext = explode(',',self::$outputs[$this->mode]);
            $p = false;
            $fpath = $path;
            while ( strlen($fpath) > 0 && !$p )
            {
                if ( $this->get_controller($fpath) )
                    return;
                else if ( $this->get_controller($this->fetch_route($fpath)) )
                    return;
                else if ( strpos($fpath,'/') === false )
                    $fpath = '';
                else
                    $fpath = substr($this->path,0,strrpos($fpath,'/'));
            }
            $this->get_controller($this->fetch_route());
        }
        return false;
    }

    /**
     * This will launch the controller in a new process.
     * It is publicly launched through check(). In between
     *
     * @return void 
     */
    private function process()
    {
        if ( $this->controller && is_null($this->is_controled) )
        {
            $this->obj = new \stdClass();
            $this->is_controled = 0;
            $mvc =& $this;
            $appui =& self::$appui;
            call_user_func(
                function() use ($mvc, $appui)
                {
                    ob_start();
                    require($mvc->controller);
                    $output = ob_get_contents();
                    ob_end_clean();
                    if ( isset($mvc->obj->error) )
                        die($mvc->obj->error);
                    else if ( !isset($mvc->obj->output) )
                        $mvc->obj->output = $output;
                }
            );
            if ( $this->data && is_array($this->data) && isset($this->obj->output) )
                $this->obj->output = self::render($this->obj->output,$this->data);
            if ( $this->obj )
                $this->is_controled = 1;
        }
        return $this;
    }

    /**
     * This will get a view.
     *
     * @param string $path
     * @param string $mode
     * @return string|false 
     */
    private function get_view($path='', $mode='')
    {
        if ( $this->mode && !is_null($this->dest) && self::check_path($path) && self::check_path($this->mode) )
        {
            if ( empty($mode) )
                $mode = $this->mode;
            if ( empty($path) )
                $path = $this->dest;
            if ( isset(self::$outputs[$mode]) )
            {
                $ext = explode(',',self::$outputs[$mode]);
                foreach ( $ext as $e )
                {
                    if ( @file_exists(self::$vpath.$mode.'/'.$path.'.'.$e) )
                        return file_get_contents(self::$vpath.$mode.'/'.$path.'.'.$e);
                    else
                    {
                        $t = explode('/',$path);
                        $last = array_pop($t);
                        if ( @file_exists(self::$vpath.$mode.'/'.$path.'/'.$last.'.'.$e) )
                            return file_get_contents(self::$vpath.$mode.'/'.$path.'/'.$last.'.'.$e);
                    }
                }
            }
        }
        return false;
    }

    /**
     * This will get the model.
     *
     * @params There is no order, but one can put a path (string) or data (array)
     * @return array|false A data model 
     */
    private function get_model()
    {
        if ( $this->dest )
        {
            $args = func_get_args();
            foreach ( $args as $a )
            {
                if ( is_array($a) )
                    $d = $a;
                else if ( is_string($a) && self::check_path($a) )
                    $path = $a;
            }
            if ( !isset($path) )
                $path = $this->dest;
            if ( !isset($d) )
                $d = array();
            if ( strpos($path,'..') === false && @file_exists(self::$mpath.$path.'.php') )
            {
                $appui =& self::$appui;
                $file = self::$mpath.$path.'.php';
                $data = $d;
                return call_user_func(
                    function() use ($appui, $file, $data)
                    {
                        include($file);
                        if ( isset($model) )
                            return $model;
                    }
                );
            }
        }
        return false;
    }

    /**
     * Processes the controller and checks whether it has been routed or not.
     *
     * @return bool 
     */
    public function check()
    {
        $this->process();
        return $this->is_routed;
    }

    /**
     * Returns the output object.
     *
     * @return object|false 
     */
    public function get()
    {
        if ( $this->check() && $this->is_controled )
            return $this->obj;
        return false;
    }

    /**
     * Checks if data exists
     *
     * @return bool
     */
    public function has_data()
    {
        return ( isset($this->data) && is_array($this->data) ) ? 1 : false;
    }

    /**
     * Returns the rendered result from the current mvc if successufully processed
     * process() (or check()) must have been called before.
     *
     * @return string|false
     */
    public function get_rendered()
    {
        if ( isset($this->obj->output) )
            return $this->obj->output;
        return false;
    }

    /**
     * Sets the data. Chainable. Should be useless as $this->data is public.
     *
     * @param array $data
     * @return void 
     */
    public function set_data(array $data)
    {
        $this->data = $data;
        return $this;
    }

    /**
     * Merges the existing data if there is with this one. Chainable.
     *
     * @return void 
     */
    public function add_data($data)
    {
        if ( !is_array($this->data) )
            $this->data = $data;
        else
            $this->data = array_merge($this->data,$data);
        return $this;
    }

    /**
     * Outputs the result.
     *
     * @return void 
     */
    public function output()
    {
        if ( $this->check() && $this->obj )
        {
            $obj = $this->obj;
            $path = self::$opath;
            $mode = $this->mode;
            call_user_func(
                function() use ($obj,$path,$mode)
                {
                    include_once($path.$mode.'.php');
                }
            );
        }
    }
}
?>

Here is the router (index.php), where constants, $bbn and $appui are defined in the includes.

<?php
include_once('config/cfg.php');
include_once('config/env.php');
include_once('config/vars.php');
include_once('config/custom.php');
if ( defined('BBN_SESS_NAME') && $appui->db )
{
    if ( !isset($_SESSION[BBN_SESS_NAME]) )
        include_once('config/session.php');
    $bbn->mvc = new \bbn\cls\mvc();
    if ( !$bbn->mvc->check() )
        die('No controller has been found for this request');
    $bbn->mvc->output();
}
?>

Well, and here's an example on how it works on a whole HTML document. Two views are used: the DOM structure, and a list element that is a part of a multi-level menu with no depth limit.

The DOM view:

...
</head>
<body itemscope itemtype="http://schema.org/WebPage">
<div id="example" class="k-content">
    <div id="vertical">
        <div id="top-pane" style="overflow:visible; width:100%">
            <ul id="menu">{{{menu_content}}}</ul>
...

The HTML list element view:

{{#menus}}
<li{{specs}}>
    {{#icon}}
    <i class="icon-{{icon}}"></i>
    &nbsp; &nbsp; &nbsp;
    {{/icon}}
    {{{title}}}
    {{#has_menus}}
    <ul>
        {{{content}}}
    </ul>
    {{/has_menus}}
</li>
{{/menus}}

And I have a nested model used by the controller for displaying the menu:

array (
    'menus' => array (
        0 => array (
            'title' => 'Hello',
            'icon' => 'cloud',
            'has_menus' => false
        ),
        1 => array (
            'title' => '1',
            'icon' => 'user',
            'has_menus' => 1,
            'menus' => array (
                0 => array (
                    'title' => '11',
                    'icon' => 'cloud',
                    'has_menus' => false
                ),
                1 => array (
                    'title' => '12',
                    'icon' => 'wrench',
                    'has_menus' => false
                ),
                2 => array (
                    'title' => '13',
                    'icon' => 'remove',
                    'has_menus' => 1,
                    'menus' => array (
                        0 => array (
                            'title' => '131',
                            'icon' => 'cloud',
                            'has_menus' => false
                        ),
                        1 => 
...

And now here is my controller for the DOM:

<?php
$mvc->data = array(
    'site_url' => BBN_URL,
    'is_dev' => BBN_IS_DEV ? 1 : "false",
    'shared_path' => BBN_SHARED_PATH,
    'static_path' => BBN_STATIC_PATH,
    'year' => date('Y'),
    'javascript_onload' => $mvc->get_view('init','js'),
    'theme' => isset($_SESSION['atl']['cfg']['theme']) ? $_SESSION['atl']['cfg']['theme'] : false
);
$tmp = new \bbn\cls\mvc("html/menu");
if ( $tmp->check() )
    $mvc->data['menu_content'] = $tmp->get_rendered();
echo $mvc->get_view('_structure','dom');
?>

Which is calling the controller for the nested menu:

<?php
if ( !$mvc->has_data() )
    $mvc->data = $mvc->get_model();
if ( isset($mvc->data['menus']) && is_array($mvc->data['menus']) )
{
    foreach ( $mvc->data['menus'] as $i => $m )
    {
        $tmp = (new \bbn\cls\mvc("html/menu"))->set_data($m);
        if ( $tmp->check() )
            $mvc->data['menus'][$i]['content'] = $tmp->get_rendered();
    }
}
echo $mvc->get_view();
?>

It is working great now! Please let me know the flaws or whatever outrageous mistake or even any advice you might come with about this code.

As you can see - if ever you've reached this part! - all the menus look alike and they all use the third menu of the first children. I have tried many combinations, the array is fine at start. It is transformed at some stage when the MVC functions are called within the controller, but I can't figure out now.

Answer 1

First of all, I'm really impressed that you are using comments. That besing said, in the future, please exclude them. Especially if they account for half of the scrollbar. It is intimidating to those of us trying to answer this, which means less meaningful answers for you. It is also unnecessary as most of us can get by without them. If you MUST have comments to explain your code then your code obviously has something wrong with it. Comments should be used in the public API, interfaces, or abstract classes, not in the code.

Another point I would like to bring up before diving in. Peter gave you some really good advice. Vague, but good all the same. The fact that you shot it down almost immediately worries me. Yes, a better explanation would have been nice, but flat out saying that you aren't going to change something is not very smart. What we are telling you, for the most part, is all VERY good advice that comes from experience. They are suggestions, but not without purpose. Some comments, like those about globals and static, will always be the same. There is never, or almost never in the case of static, a good reason to use them. If there is some reason you must not or can not change something, then you must factor that in while reading a review. But ask why someone said something before just shooting it down.

---

Static in OOP

OOP promotes:

• Encapsulization: Properties and methods are scoped and thus unavailable, or only available via namespace or pseudo-namespace.
• Inheritance: Properties and methods are inherited by child classes in order to extend functionality.
• Polymorphism: Properties and methods are adapted during inheritance to extend functionality.
• etc...

Static promotes:

• Properties and methods that are similarly scoped, but content that is static and does not change from implementation.

The distinction is subtle, and may be a bit difficult to understand. Especially since I can't think of a good example for it, but static is rarely, if ever, used. In the five years I've been doing this, not that long I suppose, I have never needed static for anything. There are exceptions, but I doubt you will ever need them.

---

Globals in OOP

There is no reason to use a global, ever, but especially not in a class. Globals were replaced by properties.

$this->$appui;
//is the same as 
global $appui;

To prove this, check out this cool constructor.

public function __construct( $appui ) {
    $this->appui = $appui;
}
//or for those assign-by-reference junkies
public function __construct( &$appui ) {
    $this->appui =& $appui;
}

There, no more need for globals. If you need to access it outside of the class, then there are a few different ways to go about it. The first I already showed you with referencing, though I don't suggest it due to lack of legibility. In fact I only mentioned it because you were already doing it. The simplest way is to manually access it from the class scope. Again, not really the best way as you normally don't want to be directly accessing these properties, but a possibility. The final way is to create a getter for it and call that getter as you need it. This final one is the one I'd recommend.

$appui = $mvc->appui;
//or
$appui = $mvc->getAppui();
//same as
global $appui;

There are quite a few different reasons why globals are considered to be bad. Chief among them is security and lack of legibility. Globals are accessable from any script. So, imagine if you had a $user global with all of the user's credentials. This is a huge security risk. Any script can access this. A private class property, on the other hand, is unavailable outside of that class and therefore is not subject to these issues. Also, globals are not not legible. You have no idea where they could be coming from and they could change at a whim.

---

Review

This actual review is going to be short. I'm usually much more thorough, but as Peter pointed out, this is very difficult to read. Make the suggestions we recommended and post a new question and I'll try and make a more thorough review. At the moment I can't get past the first globals and statics. Sorry for that harsh truth, but there it is.

---

Properties

This is not a functionality suggestion, but one of legibility, which is equally important, if not more so. If you group like properties/methods together and use whitespace to separate them then it will make it much easier to find what you are looking for. For instance:

protected $info;
//etc...

private $is_routed;
//etc...

public $data
//etc...

Going along with the above suggestion, you can then also reuse an access modifier. Though I seem to be one of the minority in this regard. So this is subjective.

protected
    /*Also, doccomments*/
    $ifo = array(),

    /*are fine here too*/
    $cli = FALSE,
    //etc...
;

That being said, some of these properties would be better as constants. For instance, those path variables. Properties/Variables are "variable", they change. A path isn't likely to change, thus it should be declared as a constant. Technically they shouldn't be hard coded into your classes at all, but for the time being I'm just going to suggest you change them to constants. For more on this take a look at: Separation of Concerns (SoC), Inversion of Control (IoC), Dependency Injection (DI).

---

Constructors and Initiators

Because your code is so dependent upon static properties many of its methods must also be static. This also includes your constructor. You can't properly instantiate your class because everything is static. You have to manually check it instead of relying on it. This extermely inhibits your abilities and makes your code repetitive, which violates the "Don't Repeat Yourself" (DRY) principle.

---

Another Point For Legibility

Sometimes it is important to abstract information from your statements just to improve legibility. This is especially the case in long statements or statements that have nested braces. Long statements are any that break 80 characters, including whitespace, usually denoted as the 80th column on editors.

$count = count( self::$appui->params );
$param = self::$appui->params[ 0 ];
if ( $count > 0 && isset( self::$outputs[ $param ] ) ) {

---

Braceless Syntax

PHP inherrently requires braces in its syntax, otherwise you wouldn't have to add them after your statements became longer than one line. To be consistent, and to avoid accidents, it is best to just always add those braces to your statements. This can also make nested statements easier to read.

else if ( count(self::$appui->post) > 0 ) {
    self::$appui->mode = 'json';
}

---

Ternary

Ternary statements are a powerful tool. But you should know when to use them. If your statement becomes too long, see above, or to complex, then you should revert to using if/else statements. And above all you should never nest ternary. However, sometimes you can just abstract a portion and that will fix it, but not always. Here's an example:

$file = self::$cpath . $c . '.php';
self::$ucontrollers[ $c ] = @file_exists( $file ) ? $file : FALSE;

---

Error Supressors

As Peter said, don't use the error suppressor @. This is a sure sign of bad code. Do whatever checks you need to do to ensure that those suppressors are unnecessary. Specifically in the above code you were suppressing the warning you get when file_exists() fails. I honestly don't know what would cause this unless the value being passed to it was not a string but an object or array. So you can use is_string() on self::$cpath to determine if you should use it. Do that check before attempting to append on to it.

if( is_string( self::$cpath ) ) {

---

Variable-Variables

self::$cpath from above is a variable-variable. These are almost as bad as globals. MVC is one of the few frameworks where they are usually considered acceptable, but you are abusing that here.

---

File Exists vs. Is File

If you know the file you are looking for is actually a file and not a directory, use is_file(). If you are not sure, use file_exists(). If you know it's a directory use is_dir().

---

Return This

When you return $this you are returning the object so that further methods can be chained to it. For instance:

$obj->meth()->chain();

This is pretty neat, but not typically done. I personally find this rather difficult to read. There are ways of making it better, such as adding new lines and whitespace after each method, but overall, it just seems cluttered. Another thing to note: The constructor can not be chained and already returns the class instance, thus explicitly returning $this is pointless and redundant.

---

PHP as a Template

Peter mentioned that PHP is already a templating language. By this he meant that it is fairly easy to add PHP variables to HTML unobstrusively.

<div id="<?php echo $id; ?>"></div>
//or with short tags
<div id="<?= $id; ?>"></div>

I would not suggest using short tags unless your server is using one of the most recent versions of PHP where they are always available, otherwise you will have to make sure your server enables them manually.

---

Conclusion

Resubmit this with Peter's suggested changes and I'll go into more details about your code and some of the principles I talked about and how to apply them. Sorry I can't do more.

Answer 2

First rewrite you code without the following things:

• global
• static
• @

Your code isn't looking an object oriented approach right now. If you can forget the usage of the two mentioned keywords (global is really bad and the static can be a hard question where to use it) we can give you further advice how you can separate in code the functionalities.

I don't know you know this but the PHP is basically a template engine and therefore you don't have to apply any other template angine magic.