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.

I had asked a first question about this class a while ago and got a few answers here which made me rewrite it completely.

I removed all statics and globals, added my variables as arguments for the constructor, and pass them again as reference through my constructor (for creating a controller from within a controller).

UPDATE: There is now a SVN with read-access: You have to do svn export https://babna.com/svn/_appui_pub ./public_html from an empty folder, and to reach public_html from your browser.

So here is my new class:

namespace bbn\cls;
class mvc 
{
    use \bbn\traits\info;
    private
        $is_routed,
        $is_controlled,
        // The name of the controller
        $dest,
        // The path to the controller
        $path,
        // The controller file (with full path)
        $controller,
        // The mode of the output (dom, html, json, txt, xml...)
        $mode;
    public
        // The data model
        $data,
        // The output object
        $obj,
        // The file extension of the view
        $ext,
        // The request sent to the server
        $original_request,
        // The first controller to be called at the top of the script
        $original_controller,
        // The list of used controllers with their corresponding request, so we don't have to look for them again.
        $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.
        $loaded_views = array(),
        // Mustage templating engine.
        $mustache,
        // Reference to $appui variable (that's my framework's name)
        $appui,
        // List of possible outputs with their according possible file extension
        $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/default 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.
        $ucontrollers = array(
        'dom' => 1,
        'html' => 1,
        'image' => 1,
        'json' => 1,
        'pdf' => 1,
        'text' => 1,
        'xml' => 1,
        'js' => 1
    );
    // Paths are in constants
    const
        cpath = 'mvc/controllers/',
        mpath = 'mvc/models/',
        vpath = 'mvc/views/',
        opath = 'mvc/_output/';

    public function __construct(&$appui,$parent='')
    {
        // The initial call should only have $appui as parameter
        if ( is_object($appui) && isset($appui->params,$appui->post) ){
            $this->mustache = new \Mustache_Engine;
            $this->appui = $appui;
            // If an available mode starts the URL params, it will be picked up
            if ( count($this->appui->params) > 0 && isset($this->outputs[$this->appui->params[0]]) ){
                $this->appui->mode = $this->appui->params[0];
                array_shift($this->appui->params);
            }
            // Otherwise in the case there's a POST we'll throw back JSON
            else if ( count($this->appui->post) > 0 ){
                $this->appui->mode = 'json';
            }
            // Otherwise we'll return a whole DOM (HTML page)
            else{
                $this->appui->mode = 'dom';
            }
            $this->mode = $this->appui->mode;
            $this->original_request = implode('/',$appui->params);
            $path = $this->original_request;
        }
        // Another call should have the initial controler and the path to reach as parameters
        else if ( is_string($appui) && is_object($parent) && isset($parent->original_request) ){
            $this->original_request =& $parent->original_request;
            $this->mustache =& $parent->mustache;
            $this->appui =& $parent->appui;
            $this->original_controller =& $parent->original_controller;
            $this->known_controllers =& $parent->known_controllers;
            $this->loaded_views =& $parent->loaded_views;
            $this->ucontrollers =& $parent->ucontrollers;
            $path = $appui;
            while ( strpos($path,'/') === 0 ){
                $path = substr($path,1);
            }
            while ( substr($path,-1) === '/' ){
                $path = substr($path,0,-1);
            }
            $params = explode('/',$path);
            if ( isset($params[0]) && isset($this->outputs[$params[0]]) ){
                $this->mode = array_shift($params);
                $path = implode('/',$params);
            }
            else{
                $this->mode = $this->appui->mode;
            }
        }
        if ( isset($path) ){
            $this->route($path);
        }
    }

    private function check_path()
    {
        $ar = func_get_args();
        foreach ( $ar as $a ){
            if ( strpos($a,'./') !== false || strpos($a,'../') !== false || strpos($a,'/') === 0 ){
                die("The path $p is not an acceptable value");
            }
        }
        return 1;
    }

    // This function returns the content of a view file and adds it to the loaded_views array.
    private function add_view($p)
    {
        if ( !isset($this->loaded_views[$p]) && is_file(self::vpath.$p) ){
            $this->loaded_views[$p] = file_get_contents(self::vpath.$p);
        }
        if ( !isset($this->loaded_views[$p]) ){
            die("The view $p doesn't exist");
        }
        return $this->loaded_views[$p];
    }

    // This fetches the universal controller for the according mode if it exists.
    private function universal_controller($c)
    {
        if ( !isset($this->ucontrollers[$c]) ){
            return false;
        }
        if ( $this->ucontrollers[$c] === 1 ){
            $this->ucontrollers[$c] = is_file(self::cpath.$c.'.php') ? self::cpath.$c.'.php' : false;
        }
        return $this->ucontrollers[$c];
    }

    // Adds the newly found controller to the known controllers array, and sets the original controller if it has not been set yet
    private function set_controller($c, $f)
    {
        if ( !isset($this->known_controllers[$this->mode.'/'.$c]) ){
            $this->known_controllers[$this->mode.'/'.$c] = $f;
        }
        if ( is_null($this->original_controller) && !empty($c) ){
            $this->original_controller = $this->mode.'/'.$c;
        }
    }

    // This directly renders content with arbitrary values using the existing Mustache engine.
    public function render($view, $model)
    {
        return $this->mustache->render($view,$model);
    }

    // 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.
    private function get_controller($p)
    {
        if ( !$this->controller )
        {
            if ( !is_string($p) ){
                return false;
            }
            if ( isset($this->known_controllers[$this->mode.'/'.$p]) ){
                $this->dest = $p;
                $this->controller = $this->known_controllers[$this->mode.'/'.$p];
            }
            else{
                if ( isset($this->appui->routes[$this->mode][$p]) && is_file(self::cpath.$this->mode.'/'.$this->appui->routes[$this->mode][$p].'.php') ){
                    $this->controller = self::cpath.$this->mode.'/'.$this->appui->routes[$this->mode][$p].'.php';
                }
                else if ( is_file(self::cpath.$this->mode.'/'.$p.'.php') ){
                    $this->controller = self::cpath.$this->mode.'/'.$p.'.php';
                }
                else if ( is_dir(self::cpath.$p) && is_file(self::cpath.$p.'/'.$this->mode.'.php') ){
                    $this->controller = self::cpath.$p.'/'.$this->mode.'.php';
                }
                else{
                    return false;
                }
                $this->dest = $p;
                $this->set_controller($p,$this->controller);
            }
        }
        return 1;
    }

    // This will fetch the route to the controller for a given path. Chainable
    private function route($path='')
    {
        if ( !$this->is_routed && self::check_path($path) )
        {
            $this->is_routed = 1;
            $this->path = $path;
            $fpath = $path;
            while ( strlen($fpath) > 0 )
            {
                if ( $this->get_controller($fpath) ){
                    break;
                }
                else{
                    $fpath = strpos($fpath,'/') === false ? '' : substr($this->path,0,strrpos($fpath,'/'));
                }
            }
            if ( !$this->controller ){
                $this->get_controller('default');
            }
        }
        return $this;
    }

    // This will reroute a controller to another one seemlessly. Chainable
    public function reroute($path='')
    {
        $this->is_routed = false;
        $this->controller = false;
        $this->is_controlled = null;
        $this->route($path);
        $this->check();
        return $this;
    }

    // This function encloses the controller's inclusion
    private function control()
    {
        if ( $this->controller && is_null($this->is_controlled) ){
            ob_start();
            require($this->controller);
            $output = ob_get_contents();
            ob_end_clean();
            if ( isset($this->obj->error) ){
                die($this->obj->error);
            }
            else if ( !isset($this->obj->output) ){
                $this->obj->output = $output;
            }
            $this->is_controlled = 1;
        }
    }

    // This will launch the control process. Chainable
    private function process()
    {
        if ( $this->controller && is_null($this->is_controlled) ){
            $this->obj = new \stdClass();
            $this->control();
            if ( $this->data && is_array($this->data) && isset($this->obj->output) ){
                $this->obj->output = $this->render($this->obj->output,$this->data);
            }
        }
        return $this;
    }

    private function get_view($path='', $mode='')
    {
        if ( $this->mode && !is_null($this->dest) && $this->check_path($path, $this->mode) ){
            if ( empty($mode) ){
                $mode = $this->mode;
            }
            if ( empty($path) ){
                $path = $this->dest;
            }
            if ( isset($this->outputs[$mode]) ){
                $ext = explode(',',$this->outputs[$mode]);
                /* First we look into the loaded_views if it isn't there already */
                foreach ( $ext as $e ){
                    $file1 = $mode.'/'.$path.'.'.$e;
                    $t = explode('/',$path);
                    $file2 = $mode.'/'.$path.'/'.array_pop($t).'.'.$e;
                    if ( isset($this->loaded_views[$file1]) ){
                        return $this->loaded_views[$file1];
                    }
                    else if ( isset($this->loaded_views[$file2]) ){
                        return $this->loaded_views[$file2];
                    }
                    else if ( is_file(self::vpath.$file1) ){
                        return $this->add_view($file1);
                    }
                    else if ( is_file(self::vpath.$file2) ){
                        return $this->add_view($file2);
                    }
                }
            }
        }
        return false;
    }

    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) && $this->check_path($a) ){
                    $path = $a;
                }
            }
            if ( !isset($path) ){
                $path = $this->dest;
            }
            if ( !isset($d) ){
                $d = array();
            }
            if ( strpos($path,'..') === false && is_file(self::mpath.$path.'.php') ){
                $appui =& $this->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 controlled or not.
    public function check()
    {
        $this->process();
        return $this->is_controlled;
    }

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

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

    // Returns the rendered result from the current mvc if successfully processed
    public function get_rendered()
    {
        if ( isset($this->obj->output) )
            return $this->obj->output;
        return false;
    }

    // Merges the existing data if there is with this one. Chainable.
    public function add_data($data)
    {
        $ar = func_get_args();
        foreach ( $ar as $d )
        {
            if ( is_array($d) )
            {
                if ( !is_array($this->data) )
                    $this->data = $d;
                else
                    $this->data = array_merge($this->data,$d);
            }
        }
        return $this;
    }

    // Creates a new MVC instance, link to the parent. All controller calls outside the router should be done this way
    public function add($d)
    {
        return new mvc($d,$this);
    }

    // Outputs the result.
    public function output()
    {
        if ( $this->check() && $this->obj ){

            if ( isset($this->obj->prescript) ){
                if ( empty($this->obj->prescript) ){
                    unset($this->obj->prescript);
                }
                else{
                    $this->obj->prescript = \bbn\cls\str\text::clean($this->obj->prescript,'code');
                }
            }
            if ( isset($this->obj->script) ){
                if ( empty($this->obj->script) ){
                    unset($this->obj->script);
                }
                else{
                    $this->obj->script = \bbn\cls\str\text::clean($this->obj->script,'code');
                }
            }
            if ( isset($this->obj->postscript) ){
                if ( empty($this->obj->postscript) ){
                    unset($this->obj->postscript);
                }
                else{
                    $this->obj->postscript = \bbn\cls\str\text::clean($this->obj->postscript,'code');
                }
            }

            switch ( $this->mode ){

                case 'json':
                    if ( !ob_start("ob_gzhandler" ) ){
                        ob_start();
                    }
                    else{
                        header('Content-Encoding: gzip');
                    }
                    if ( isset($this->obj->output) ){
                        $this->obj->html = $this->obj->output;
                        unset($this->obj->output);
                    }
                    header('Content-type: application/json; charset=utf-8');
                    echo json_encode($this->obj);
                    break;
                case 'dom':
                case 'html':
                case 'js':
                case 'text':
                case 'xml':
                    if ( !isset($this->obj->output) ){
                        header('HTTP/1.0 404 Not Found');
                        exit();
                    }
                case 'dom':
                case 'html':
                    if ( !ob_start("ob_gzhandler" ) ){
                        ob_start();
                    }
                    else{
                        header('Content-Encoding: gzip');
                    }
                    header('Content-type: text/html; charset=utf-8');
                    echo $this->obj->output;
                    break;
                case 'js':
                    header('Content-type: application/javascript; charset=utf-8');
                    echo $this->obj->output;
                    break;
                case 'text':
                    header('Content-type: text/plain; charset=utf-8');
                    echo $this->obj->output;
                    break;
                case 'xml':
                    header('Content-type: text/xml; charset=utf-8');
                    echo $this->obj->output;
                    break;
                case 'image':
                    if ( isset($this->obj->img) ){
                        $this->obj->img->display();
                    }
                    else{
                        header('HTTP/1.0 404 Not Found');
                    }
                    break;
                default:
                    if ( isset($this->obj->file) ){
                        if ( !is_file($this->obj->file) ){
                            unset($this->obj->file);
                        }
                    }
                    if ( !isset($this->obj->file) ){
                        header('HTTP/1.0 404 Not Found');
                        exit();
                    }
                    header("X-Sendfile: ".$this->obj->file);
                    header("Content-type: application/octet-stream");
                    header('Content-Disposition: attachment; filename="' . basename($this->obj->file) . '"');
            }
        }
    }
}

Here is the router:

// Different environments personal settings
include_once('config/cfg.php');
// Setting up an environment
include_once('config/env.php');
// Creating $bbn and $appui vars
include_once('config/vars.php');
// Loading routes configuration
include_once('config/routes.php');

if ( defined('BBN_SESS_NAME') && $appui->db ){
    // Loading users scripts
    include_once('config/custom.php');
    // Setting up the session
    if ( !isset($_SESSION[BBN_SESS_NAME]) ){
        include_once('config/session.php');
    }

    $bbn->mvc = new \bbn\cls\mvc($appui);

    if ( !$bbn->mvc->check() ){
        die('No controller has been found for this request');
    }
    else
    {
        array_push($_SESSION[BBN_SESS_NAME]['history'],$bbn->mvc->original_request);
        if ( count($_SESSION[BBN_SESS_NAME]['history']) > $bbn->vars['history_max'] ){
            array_shift($_SESSION[BBN_SESS_NAME]['history']);
        }
    }

    $bbn->mvc->output();
}

Well, and here's an example on how it works on a whole HTML document. 2 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 (inside #menu):

{{#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:

$this->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 = $this->add("html/menu");
if ( $tmp->check() ){
    $this->data['menu_content'] = $tmp->get_rendered();
}
echo $this->get_view('_structure','dom');

Which is calling the controller for the nested menu:

if ( !$this->has_data() ){
    $this->data = $this->get_model();
}
if ( isset($this->data['menus']) && is_array($this->data['menus']) )
{
    foreach ( $this->data['menus'] as $i => $m )
    {
        $tmp = $this->add("html/menu")->add_data($m);
        if ( $tmp->check() ){
            $this->data['menus'][$i]['content'] = $tmp->get_rendered();
        }
    }
}
echo $this->get_view();

Thanks a lot for your feedbacks !

Oh, and here is the filesystem: Filesystem

share|improve this question
1  
MVC is about separating things in an application so a class names mvc not the best idea. I'm working on a MVC framework, and i have 80+ classes to serve just one request. –  Peter Kiss Dec 5 '12 at 16:48
    
The idea here is that the mvc class is not the controller, but the class which will put together the controllers, the models and the views. It is more a router. –  Nabab Dec 5 '12 at 17:11

1 Answer 1

Don't like it too much.

Passing variables into functions as references always scare me because usually you cannot track anymore what's going on. __construct(&appui,...) is one such thing.

References communicate to me the following: "I expect this value to be not an object, and I expect the value to be changed inside, and the change also made intentionally effective on the outside."

This communication is clearly wrong in your case: a) Either you get an object - then the reference is not needed because objects are ALWAYS passed as reference. b) If you get a string, then you do nothing with the string but copy it to another variable.

Additionally there are some cases where you copy values as references that seem unnecessary to me.

Completely unrelated: Have you heard about autoloading? You seem to do some extensive stuff with paths to find your models for example, and then try to include them with a tricky wrapping.

I assume models are classes. If they are, then autoloading will load them the very time they are first needed because some code stumples upon it's classname. Check "PSR-0 autoloading" to get more info.

I just came across this part:

// Processes the controller and checks whether it has been controlled or not.
public function check()
{
    $this->process();
    return $this->is_controlled;
}

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

I would thing that inside get() the if might be easier made like this:

        if ($this->check()) return $this->obj;

A final comment: I do not like the idea too much that you basically only have one controller class that does everything, and that your controllers are no real classes, but code fragments that act inside your single controller, stacking stuff.

And I miss type hinting very much in your code.

share|improve this answer
    
Thanks for the comments! The _output folder doesn't exist anymore, but is now included in the class. I removed references as arguments. –  Nabab Dec 5 '12 at 15:50
    
True for "controllers are no real classes, but code fragments that act inside your single controller". I'll think about it. I'm updating my question with a SVN access if you or anyone is interested to give it a try. –  Nabab Dec 5 '12 at 15:52

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.