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've written a framework in MVC, I call it Midget MVC, as it's so darn small. The reason I wrote it is because I wanted a lightweight and extendible framework to use in projects. It eventually got abandoned due to it's break-ability and my lack of experience in the subject. However, I've got a bigger project I'm working on and need a framework, so I want to start re-writing MMVC. Feel free to view the entire project here.

The main piece of code I want looked over is the router class. After talking a bit on the PHP IRC I discovered that it was indeed breakable. One of the recommendations made was for me not to use global $config. What's the best way to do this?

<?php

class Route {
    public function load(){
        global $config;
        if(isset($_SERVER['PATH_INFO'])){
            $route = explode('/', substr($_SERVER['PATH_INFO'], 1));
            $controller = ucfirst(strtolower($route[0]));
            $method = isset($route[1]) ? strtolower($route[1]) : 'index';

            $path = APPPATH . "controllers/" . $route[0] . ".php";
            if(realpath($path)){
                require $path;
                $controller = new $controller;

                if($method != 'index'){
                    $variables = array();
                    $i = 0;
                    foreach($route as $vars){
                        if ($i >= 2)
                            $variables[] = $route[$i];
                        $i++;
                    }

                    call_user_func_array(array($controller, $method), $variables);
                }else{
                    $controller->index();
                }
            }
        }else{
            $controller = $config['deafult_controller'];
            require APPPATH . "controllers/" . $controller . ".php";
            $controller = ucfirst($controller);
            $controller = new $controller;
            $method = 'index';
            $controller->$method();
        }
    }
}

The second thing I want help with is the load class. Is there a better way to load models into the controller? Loading library files was quite difficult; you have to load it in each file. If you load the library in the controller, you'll have to load it again in the model if you want to access it. What's teh best way to do this?

class Load{
    function __construct($controller){
        $this->controller = $controller;
    }

    // loads a specified model
    public function model($model){
        $model = strtolower($model);
        if(file_exists(APPPATH . "models/$model.php")){
            require(APPPATH . "models/$model.php");
            $modelclass = ucfirst($model . '_Model');
            // call function to include it in the controller
            $this->controller->$model = new $modelclass;
        }
    }

    // loads a veiw
    public function view($view, $data = NULL){
        // asign filepath
        $path = APPPATH . "views/" . $view . ".php";
        // asign variables if $data is set
        if($data != NULL)
            foreach($data as $var => $value)
                $$var = $value;

        if(file_exists($path)){
            ob_start();
            require($path);
            print(ob_get_clean());
        }else
            return false;
    }

    public function library($library){
        $library = strtolower($library);
        if(file_exists(LIBRARY . "$library.php")){
            require(LIBRARY . "$library.php");
            $libraryclass = ucfirst($library);

            $this->controller->$library = new $libraryclass;
        }
    }
}

And finally, how secure is the entire thing. What security flaws can you see? Are there any major points I should improve upon. I apologise if this post isn't quite up to scratch; it's my first time posting here.

Thank you very much! I look forward to reading any responses.

share|improve this question
    
+1 I just love it when a question has only the code needed for review, and the OP is articulate enough about what exactly it is that they want reviewed. –  Joseph Silber Feb 3 '13 at 2:07

1 Answer 1

Nice initiative on a lightweight MVC framework!

Note wanted to add more reference links, but it won't allow me to do more thanb 2

Config

  • Pass the default controller setting to the function

or

  • Pass all routing related settings to the __constructor. and use them as class attributes

Loader class

The setup isn't all that bad. Nice and lightweight. A few things, i would do different;

  • Seperations of conserns (One of the SOLID principles). Let the loader just load an return the instances.
    • Don't bind the loaded elements directly to controller in you class, so in future you could also use it for different cases then just the controller class.
    • Don't render the views in a loader. Make a render class or such. If you way of rendering changes, you would have to change the loader. Which ultimately don't make sense.
  • Make APP_PATH and LIBRARY settable within the loader, and call them differently

    • A loader class attribute for each load path
    • Default the attributes to APP_PATH and LIBRARY
    • Getter and Setter for load paths
    • Now you would be able to integrate different paths / modules etc. Making it more flexible
  • Autoloader for library classes

    • By registering the loader as php autoloader you can just instantiate a class.
    • If not yet loaded into mem, it will be sent to the autoloader.
    • There you resolve the path it should be on and instantiate.

Security

The big thing in this form of autoloading is the concern for input. Make sure all input is either given by the application itself or validate it.

  • Validate user strings that will be autoloaded (eg. controller names)
  • You can do this by first doing a realpath() check on the computed path. And make sure the realpath() is within the locations you would expect.
  • This prevenst users from entering stuff like '......\uploads\mayhackscript' which would be executed otherwise.

This is a real danger.

General remarks

Take a look at te following;

  • SOLID principles
  • PSR-2 coding standards; note, it extends PSR-1 which extends PSR-0

Good luck on your project!

share|improve this answer
    
PSR-0 is an autoloader interoperability standard. PSR-1 is a basic coding standard. PSR-2 is a coding style guide. –  Rob Apodaca Feb 4 '13 at 13:35

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.