PHP MVC implementation without a framework

Question

I've been using PHP for years and in every project I've ever done I've included the HTML, PHP, SQL etc ... in the same page. Upon looking through Github at large projects like WordPress and MediaWiki it appears there are much better ways to lay out your code. I want to use MVC for my next project because I plan on making the code open source. I don't want to use a framework because I don't want lots of overhead and I don't want to have to learn a new framework to be honest.

I've started implementing what I think is a very simple MVC framework for my project. Can anyone who has experience with MVC tell me if I'm doing this correctly?

My project is called How, hence the class names.

The file structure of my project looks like this:

Root directory   -> index.php /includes/ .htaccess
In /includes/    -> /classes/ /controllers/ /routes/ /views/
In /classes/     -> _Globals.php How.php Route.php View.php
In /controllers/ -> Controller.php Root.php
In /Routes/      -> Route.php
In /views/       -> root.php

index.php

<?php
include_once( 'includes/classes/How.php' );

$how = new How();
$how->run();

/includes/classes/How.php

<?php
include_once( './includes/routes/Routes.php' );

class How {
    public function getRoute() {
    global $Routes;
    $uri = $_SERVER['REQUEST_URI'];

    if (!in_array($uri, $Routes)) {
        die("Invalid route.");
    }
    return $uri;
    }

    public function run() {
        try {
            $this->getRoute();
        } catch (Exception $e) {
            die("Failed to get route.");
        }
    }
}

/includes/classes/Route.php

<?php
include_once( '_Globals.php' );
class Route {

    private function registerRoute($route) {
        global $Routes;
        $Routes[] = BASEDIR.$route;
    }

    public static function set($route, $closure) {
        self::registerRoute($route);
        $closure->__invoke();
    }
}

/includes/classes/View.php

<?php
    class View {
        public static function make($view) {
            try {
                include( './includes/controllers/'.$view.'.php' );
                include('./includes/views/'.$view.'.php');
                return 1;
            } catch ( Exception $e ) {
                return 0;
            }
        }
    }

/includes/classes/_Globals.php

<?php
$Routes = array();
define( 'BASEDIR', '/how/' );

/includes/controllers/Root.php (Controller.php is currently blank)

<?php
class RootController {
    public static $username = "Francis";
}

/includes/Routes/Route.php

<?php
include_once( './includes/classes/Route.php' );
include_once( './includes/classes/View.php' );

Route::set('', function() {
    View::make('root');
});

Route::set("about-us", function() {
});

/includes/views/root.php

<h1>Welcome <?php echo RootController::$username; ?>!</h1>

.htaccess

RewriteEngine On
RewriteRule ^([^/]+)/? index.php?url=$1 [L,QSA]

It works like this:

1. The .htaccess redirects all requests to index.php.
2. Index.php calls how->run() which checks if the current route is valid.
3. Valid routes are stored in the global $Routes array, they are added to the $Routes array by the Route::set() method in the Routes/Route.php.
4. If the route is valid the View::make() method is called which loads the view from /includes/views/ and the ViewController is loaded from /includes/controllers/.

Answer 1

Before you consider any answer, always consider the rule that you should not write code which is not helpful and avoid code which is unused. When you are using an external framework there is a community which maintains it for you. Here its only you who is the expert.

• Less code you write, less maintenance & faster development time.
• More readable and maintainable code, easier it is.

Feedback on your code.

1. Routes.php is quite simplistic. It is not necessarily bad but you havent considered how you would pass on different types of values.
2. The framework considers only routing as the primary goal of the framework. But you havent considered other things like DB query, Caching, Templating, Security. etc
3. You are loosing a lot of flexibility in file management when with the current routes framework.

In addition to the above I suggest including two more pieces of code.

• A general file (include_top.php) which gets included in top of all files. It might look something like the following

  <?
  session_start();
  //do some global configurations
  function __autoload($className) {
      // Put your own directory structure here //
      if(file_exists("../classes/$className.php"))
      include_once "../classes/$className.php";
  }
  ?>
  

  This helps you to easily structure your code into different folders without worry about certain import statements having right path and doing additional global configurations.

• An ORM (Object-relational mapping) is a must for any scalable framework. You can checkout existing frameworks already. I will put a plug for my own light weight ORM.

Answer 2

Some thoughts:

• First, with regards to directory structure, move all your includes, classes, etc. out of the web directory. There is no reason they should be there. You can just have index.php in the web directory as well as any static assets you may need.

• Consider moving .htaccess rewrite rules into the actual Apache .conf file for your service and turn off directory overrides. This saves overhead of having to read .htaccess file with every single request coming into the service.

• You mentioned this as an MVC framework, but we seem to be missing the M (model)? Where is it? Hard to review without understanding where data comes from.

• It seems like your index.php front controller should be doing more than just including and instantiating a single class. What about setting up core dependencies that might be needed regardless of route passed (i.e. session, application configurations, database connections,etc.)?

• Why is you main main named How? This name conveys nothing about what this class actually does.

• Not sure about purpose of _GLobals.php include. Do you really need an include for a single define statement and to set up and empty array? Not clear to me what purpose BASEDIR serves.

• The root.php file seems trivial. What is it intended to do? Just store static reference to user name?

• I am left with the overall impression that this framework, in its current state, is really not adding much value in comparison to having simple Apache rewrites (to give friendly URL's) the point to individual PHP scripts for each view (of course these scripts could have common includes).

• More specific code review notes by file are given below. I have included my thoughts inside multi-line comments

How.php

<?php
/*
Consider changing this to require instead of include.
If this dependency is missing you want to stop code execution.
Consider having this require in index.php as it it makes no sense to hide
this key piece of application set up inside some class file. It took me
several minutes of looking at this code to even understand where the route
definition were even set up. This along with this file having same name as
a class file is confusing.
*/
include_once( './includes/routes/Routes.php' );

/*
Should class methods here be static?  I don't see anything here that
makes me think you need to actually instantiate a How object to execute
this logic.
*/
class How {
/*
Should this be public? Why does this method exist on this class instead of
on Route class?
*/
    public function getRoute() {
/*
Indent all this code inside method.
Don't use globals. This is a really bad coding habit. Pass this method
the dependencies it needs.
You also are trying to reference $Routes from global name space, but
it has not been defined by the time you call run() which call this method.
*/
    global $Routes;
    $uri = $_SERVER['REQUEST_URI'];
/*
$Routes should probably be an associate array with routes as keys or object
with property names that match routes.  Why would you want to iterate the
entire $Routes array to see if you have a route set?  That is O(n) operation
when associative array or object would allow for O(1) lookup.
*/  
    if (!in_array($uri, $Routes)) {
/*
Don't die here. You wrap call to this method in try-catch block,
yet the method does not throw any excpections. Perhaps this should be
exception that caller would need to catch and redirect. I would argue
that most routing logic should actually expect invalid routes do something
like server up 404 header and message.  This is something that will
undoubtedly happen for any application so I don't know that it really
should even be an exception, much less die
*/
        die("Invalid route.");
    }
    return $uri;
    }

    public function run() {
/*
No need for try-catch here as getRoute() call doesn't throw.  You may want
getRoute() to return false of something for bad route and have this method
provide 404 headers/messaging
*/
        try {
/*
Why are you calling a getter here to get the route URI and then doing
nothing with it? I would think you would set this to variable,
determine appropriate controller to handle the route, etc. As it stands,
you application does absolutely nothing other than fail if a route
doesn't exist.
*/
            $this->getRoute();
        } catch (Exception $e) {
            die("Failed to get route.");
        }
    }
}

Route.php

<?php
include_once( '_Globals.php' );

/*
I don't understand why you are using $Routes as a global here.  If
you intend to instantiate a Route object (not clear where that happens
since it doesn't happen in How class), why not put route info as property
of this object, not a global variable.
*/
class Route {
/*
This method is called statically and should likely be explicitly
defined as static method.
*/
    private function registerRoute($route) {
/*
Don't start php variable names with capital letters.  This would really be again standard coding convention in most any programming language.
*/
        global $Routes;
        $Routes[] = BASEDIR.$route;
    }

/*
Not clear to me why just setting a route should also invoke closure
associated to route. Why would caller specify a closure (controller?)
to be attached to route?
I would think that would be part of the application configuration
and known to the Route class, not set from outside caller.
*/
    public static function set($route, $closure) {
        self::registerRoute($route);
        $closure->__invoke();
    }
}

View.php

<?php
    class View {
/*
I don't understand the return values from this method. Why 1 & 0? Should
this method return void and just fail if includes are not succesfful?
*/
        public static function make($view) {
/*
These includes (probably should be requires) don;t throw, so wrapping in
try-catch is meaningless. These relative paths are poor form. Perhaps
setup up constant in config that determine full path to where includes
are located.
*/
            try {
                include( './includes/controllers/'.$view.'.php' );
                include('./includes/views/'.$view.'.php');
                return 1;
            } catch ( Exception $e ) {
                return 0;
            }
        }
    }

Answer 3

You can be excused for not using a framework. But not using libraries and reinventing the wheel is just not what you should be doing. You should:

• Autoload all your files. It will clean a lot more mess for you. Check out spl_autoload_* functions to learn how to autoload.

• You should not use global variables within classes. You're violating an Object Oriented Programming (OOP) concept called "Encapsulation." If you really want the variable to be accessible from other places, there's a neater workaround. What you could do is, you can have a file like this:

  <?php return [ ['', function() { View::make('root'); } ], ['about-us', function() { //callback code } ] ];

• So, whenever you want access to that routes, you can just include the file like this:

$routes = include('path/to/routes.php');

• Naming seems to be a problem. The name of the class How is quite vague. A better name would be Bootstrapper. Someone else reading your code will immediately get an idea of what the class is meant for.
• Variable name format must be in $camelCase as per PHPs unofficial standards. You have done a good job in this case except for one variable, $Routes.
• If you wish to adhere to the PHPs unofficial standards, your curly braces should be placed like this:

Classes

class ClassName
{
    // code
}

Methods

public function method()
{

}

That's your code reviewed! Let's move on to how to not reinvent the wheel by not using a framework.

In my honest opinion, if you do not use a framework, you should at least use a dependency manager. What this basically does is it helps you import PHP libraries that have been well developed and tested for you. You should definitely check out Composer. What is Composer? It basically autoloads all your files and it also allows you to import opensource libraries listed on Packagist.

This way, you do not need to develop your own Router. You can use some really nice routers like FastRoute.

I highly recommend you to check this tutorial out to get you started.

Answer 4

This does indeed look good in terms of MVC but I am really wondering what withholds you from learning a new framework.

Frameworks have a built in support for security and so on and mostly get peer reviewed by a community which in turn adds to safety and quality, better than anyone could do alone to be honest.

Furthermore, I would never use die()

Look into try - catch - finally! more info is located here