Gfrktyl

I tried to make a MVC in PHP with OOP approach. Code Review this so I can understand whether am I using the best practices and can I legally call it a MVC framework or not. Also, I am going with 1 controller for each action.

This blockquote might help you review my code better.

The default thing is actually works like this:

http://localhost/coolcodes/something/test/ (This is bring up the TestController)

http://localhost/coolcodes/something/ (This will bring up the default controller for something).

There is a function called getDefaultRouteName.

getDefaultRouteName => Returns the key for which the default controller is located in the $routesTree variable.

My code:

index.php (The page the client will go to):

<?php

include_once("App.php");



$app = new App();



$app->init();

$app->execute();



?>

App.php (The main application starter):

<?php

include_once("RoutesTree.php");

include_once("Config.php");



class App {



  private $controller;



  public function init() {



    $routesTree = new RoutesTree;



    if (Config::isUnderDevelopment()) {



      $this->controller = $routesTree->getUnderDevelopmentController();



    } else {



      $pageURL = str_replace("/coolcodes/api/", "", $_SERVER['REQUEST_URI']);



      $path = explode("/", $pageURL);



      if ($_SERVER['REQUEST_URI']{strlen($_SERVER['REQUEST_URI']) - 1} == "/") {



        $path = array_slice($path, 0, count($path) - 1);



      }



      $this->controller = $routesTree->getController($path);





    }



  }



  public function execute() {



    $this->controller->execute();



  }





}

?>

Routes.php (The routes for MVC):

<?php

include_once("Config.php");



class RoutesTree {



  private $routesTree;



  private $notFoundController;



  function __construct() {



    $this->initRoutesTree();



  }



  public function initRoutesTree() {



    $this->routesTree = Config::getPreparedRoutesTree();



    $this->notFoundController = Config::getNotFoundController();



  }



  public function getRoutesTree() {



    return $this->routesTree;



  }



  public function getUnderDevelopmentController() {



    return Config::getUnderDevelopmentController();



  }



  public function getController($path) {



    $route = $this->routesTree;



    foreach ($path as $pathSegments) {



      if (array_key_exists($pathSegments, $route)) {



          $route = $route[$pathSegments];



      } else {



        return $this->notFoundController;



      }



    }



    if (is_array($route)) {



      return $route[Config::getDefaultRouteName()];



    }



    return $route;



  }



}



?>

Controller.php (MVC Controller base class):

<?php



abstract class Controller {



  abstract public function execute();



}



?>

Some Controllers:

TestController.php:

<?php

include_once("Controller.php");



class TestController extends Controller {



  public function execute() {



    echo json_encode(["value" => "Hello, World!!!!"]);



  }



}



?>

SomethingController.php:

<?php

include_once("Controller.php");



class SomethingController extends Controller {



  public function execute() {



    echo json_encode(["value" => "From something controller."]);



  }



}



?>

Controllers that are required:

NotFoundController.php (The Error 404 Controller)

<?php

include_once("Controller.php");



class NotFoundController extends Controller {



  public function execute() {



    echo "<h1>Sorry, Page not found.</h1>";



  }



}



?>

UnderDevelopementController (Controller shown if the website is under construction):

<?php

include_once("Controller.php");



class UnderDevelopmentController extends Controller {



  public function execute() {



    echo json_encode(["value" => "This website is under construction. See you soon."]);



  }



}



?>

Config.php (Settings for MVC (inspired by settings.py in Django))

I made functions instead of variables because I thought that it will be more OOP oriented in PHP (Because I do not want to make variables outside of any classes).

<?php

include_once("allclasses.php");



class Config {



  public static function isUnderDevelopment() {



    return false;



  }



  public static function getUnderDevelopmentController() {



    return new UnderDevelopmentController;



  }



  public static function getNotFoundController() {



    return new NotFoundController;



  }



  public static function getDefaultRouteName() {



    return "default";



  }



  public static function getPreparedRoutesTree() {



    return [



      "something" => [



        "default" => new SomethingController,



        "test" => new TestController



      ]



    ];



  }



}



?>

addclasses.php (To Group all the controllers)

<?php



include_once("controllers/TestController.php");

include_once("controllers/SomethingController.php");

include_once("controllers/NotFoundController.php");

include_once("controllers/UnderDevelopmentController.php");



?>

The .htaccess file:

Options +FollowSymLinks

RewriteEngine On



RewriteCond %{SCRIPT_FILENAME} !-d

RewriteCond %{SCRIPT_FILENAME} !-f



RewriteRule ^.*$ ./index.php

As Your Common Sense noted in the comments, leveraging auto loading classes in PHP can help clean up your includes for the controller files (and model classes too).

Some additional improvements:

• 
  

  Tight Coupling to Config class: Create an instance of your Config class, and pass it to the constructor of your App class, instead of using static methods.

  
  
  

  $app = new App(new Config());
  
  

  
  
  

  Then pass this the instance of Config into your RoutesTree class:

  
  
  

  $routesTree = new RoutesTree($this->config);
  
  

  
  
  

  This gives you loose coupling between the App, RoutesTree and Config classes by way of Dependency Injection.

  
  



• Separate controllers from ouput: I think this is the area where you are violating the "V" in MVC. Your example controller echo's directly to the standard output. A controller should produce a result, but something else should be responsible for sending this back to the client. Most frameworks opt for an object that encompasses the data to be rendered, and the name of a view used to render it.



• 
  

  Convention based URL mappings: Instead of keeping an array of controller names to controllers, use the controller name from the URL to return the right controller:

  
  
  

  // http://localhost/app/test maps to TestController
  
  $controller = ucfirst('test') . 'Controller';
  
  
  
  return new $controller();
  
  

  
  
  

  You can still build in a way to do custom routing, but you'll find a pattern to your URLs and the controllers that respond to them

  
  


• 
  

  No restriction on HTTP methods: Your controller actions appear to respond to both GET and POST requests. This can be a security flaw when you allow a GET request to modify data. Consider this request:

  
  
  

  HTTP/1.1 POST http://localhost/app/createComment
  
  
  
  comment=Hacked&post_id=3
  
  

  
  
  

  That's all fine and dandy until someone puts this HTML on a random page on the internet, and tricks your users into visiting it:

  
  
  

  <script src="http://localhost/app/comments?comment=Hacked&post_id=3"></script>
  
  

  
  
  

  Upon visiting the page, if you are logged in to your site the browser will issue a GET request to http://localhost/app/comments?comment=Hacked&post_id=3. Since your application does not check if the client has issued a GET or POST request, blog post #3 gets a new comment each time you visit the page.

  
  
  

  Sure the browser can't understand the result of this HTTP request as JavaScript, but that doesn't stop the browser from sending the request, along with your session cookies.