1
\$\begingroup\$

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
\$\endgroup\$
  • 1
    \$\begingroup\$ Apparently the biggest improvement would be implementation of autoload \$\endgroup\$ – Your Common Sense Dec 17 '18 at 15:04
  • 2
    \$\begingroup\$ @YourCommonSense Please do not answer in comments. All suggestions for improvements should be posted as answers. \$\endgroup\$ – 200_success Dec 17 '18 at 17:44
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I guess that I will make two different execute functions, one for GET request and another for POST request. So, will it be fine? \$\endgroup\$ – Siddharth Bose Dec 19 '18 at 12:39
  • \$\begingroup\$ @SiddharthBose: Since you have one controller per request, the controller itself could define a method called something like canExecuteRequest('GET'). If it returns false then the App object would throw an exception, or return a 400 bad request response back to the client. \$\endgroup\$ – Greg Burghardt Dec 19 '18 at 12:54

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.