Routing in MVC with PHP

Question

I am in the progress of creating a MVC structured website. I've opted to make my own instead of using pre-constructed MVC systems as a way to teach myself better coding and understanding how applications work. I've got everything laid out, but I'm not happy with my routing system. It is VERY crude and I'd like it to be more robust.

I'd like to get feedback to help improve my code.

Below is the full code for index.php, where my routing code is:

<?php

session_start();

// Estabilish Database Connections and system defaults
include('config.php');

function setReporting() {
    if (DEVELOPMENT_ENVIRONMENT == true) {
        error_reporting(E_ALL);
        ini_set('display_errors','On');
    } else {
        error_reporting(E_ALL);
        ini_set('display_errors','Off');
        ini_set('log_errors', 'On');
        ini_set('error_log', ROOT.DS.'tmp'.DS.'logs'.DS.'error.log');
    }
}
function db_connect() {
    $connection = mysql_connect(DB_HOST,DB_USERNAME,DB_PASSWORD);
    if (!$connection) {
        die("<h2>Error Connecting to Database</h2>");
    }
    if(!mysql_select_db(DATABASE, $connection)) {
        die("<h2>Database Does Not Exist</h2>");
    }
    return $connection;
}
function hook() {
    $params = parse_params();
    $url = $_SERVER['REQUEST_URI'];
    $url = str_replace('?'.$_SERVER['QUERY_STRING'], '', $url);

    $urlArray = array();
    $urlArray = explode("/",$url);
    var_dump($urlArray);
    if (isset($urlArray[2]) & !empty($urlArray[2])) {
        $route['controller'] = $urlArray[2];
    } else {
        $route['controller'] = 'front'; // Default Action
    }
    if (isset($urlArray[3]) & !empty($urlArray[3])) {
        $route['view'] = $urlArray[3];
    } else {
        $route['view'] = 'index'; // Default Action
    }
    include(CONTROLLER_PATH.$route['controller'].'.php');
    include(VIEW_PATH.$route['controller'].DS.$route['view'].'.php');
    var_dump($route['controller']);
    var_dump($route['view']);
    var_dump($urlArray);
    var_dump($params);

    // reseting messages
    $_SESSION['flash']['notice'] = '';
    $_SESSION['flash']['warning'] = '';
}

// Return form array
function parse_params() {
    $params = array();
    if(!empty($_POST)) {
        $params = array_merge($params, $_POST);
    }
    if(!empty($_GET)) {
        $params = array_merge($params, $_GET);
    }
    return $params;
}

// Prepare General Application Models
require($_SERVER['DOCUMENT_ROOT'].'/'.APP.'/'.'models/'.'general.php');

setReporting();
date_default_timezone_set(get_timezone());
$current_theme = current_theme();
hook();

if($_SESSION['flash']['notice']) {
    echo $_SESSION['flash']['notice'];
}

One big problem is that my hook call won't work properly when the urls are two levels deep. Say I wanted to visit mywebsite.com/admin, it would work, however mywebsite.com/admin/dashboard would not. The problem is in the arrays. How could I get the array to load content after the 2nd level along with the second level?

Would it be best to create an array like this?

Array
     - controller
     - view
          - dashboard

What would be the best way to set up "custom" URLs?

If I were to put in mywebsite.com/announcement, it would check to see if it has controllers. Failing that, it would check to see if it has custom content (maybe a file of the same name in "customs" folder, and then if there's nothing, execute the 404 page not found stuff). This isn't a priority question though, but loosely associated in how the code works so I thought it best to add.

Answer 1

First off, congrats on making your own, that's the best way to learn and usually you don't need all the added overhead that a prefab provides.

In the implementation of MVC I like to use, there is a router between the view and controller, so its more of a MVRC rather than MVC. Just another level of abstraction, but I like it. It seems like what you are trying to do here is similar. So maybe my experience will help. My routers are typically classes, but that's a preference. Here are some suggestions on your code, along with a possible solution to your problem.

Error Reporting

When you set error reporting, make sure you don't do the same thing twice, such as setting the same reporting level, it should always be the same, so just declare it before the if statement. This actually holds true for anything. This is called the DRY principle (Don't Repeat Yourself). Also, you might want to consider logging errors even while in the development environment. This ensures that you don't miss those "silent" errors and allows you to ensure that the logs are working correctly. So a possible rewrite of your setReporting() function.

function setReporting() {
    error_reporting(E_ALL);

    ini_set( 'display_errors', DEVELOPMENT_ENVIRONMENT );
    ini_set('log_errors', 'On');
    ini_set('error_log', ROOT.DS.'tmp'.DS.'logs'.DS.'error.log');
}

Would change DEVELOPMENT_ENVIRONMENT to just DEVELOPMENT or my preferred STAGING.

Edit Above I meant to say ENVIRONMENT instead of DEVELOPMENT. This is because you are no longer using this constant to check the development environment, you are using it to set an environment.

You might actually want to set up your error reporting and db connection using a config file of some sort and then just reference it in the router. Another level of abstraction, but a very common one.

Development Tools

Don't use die(), var_dump(), print_r(), etc... outside of you development environment. There are better, more elegant, ways of displaying data to the customer.

Unnecessary Work

Instead of using "REQUEST_URI" and removing "QUERY_STRING" from it, why not just use "SCRIPT_NAME"? It does the same thing.

$_SERVER[ 'SCRIPT_NAME' ];

You don't have to define $urlArray as an array if you are immediately going to assign an array to it. Essentially you are assigning it a value and then never using it, which is bad and confusing. This isn't Java or C, you don't have to type hint a variable before using it.

Always check to see if the language you are working in provides a way for you to do something easier than you could do it yourself.

Defining a Default Value

This is a stylistic choice, but instead of using else statements to define default values, just define the default values first. If those values need to change, then change them.

$route[ 'view' ] = 'index';
if (isset($urlArray[3]) & !empty($urlArray[3])) {
    $route['view'] = $urlArray[3];
}

Distinguishing Between isset() and empty()

isset() returns TRUE only if a variable is set, and its value is not NULL.

empty() returns FALSE only if a variable is not FALSE, NULL, an empty string, any form of zero, or an empty array.

Unless you explicitly need to distinguish between a variable not being set, or empty, then I would suggest only using isset(), not both.

list()

A better way to assign array values to specific variables is to use a list(). You'll need to use array_pad() to ensure the array is the proper length first, but don't worry, it won't add anything if its already past that length. Also, I don't know why you are passing these variables to an array and then never using that array. It's unnecessary. However, I do show how to pass these variables back to an array in case you are planning on expanding upon that function.

$urlArray = array_pad( $urlArray, 4, NULL );
list( , , $controller, $view ) = $urlArray;
$route = compact( 'controller', 'view' );

Your Problem

Your array, at least in the examples you provided, is not four fields long, its three. You've skipped the first empty field, but then, instead of using "1" as your index, you use two. Arrays start indexing their fields at zero. So When you call for the indices at "2" and "3", you are actually calling for the third and fourth elements. This is why your example will work for one level, but not another. I'm surprised your examples aren't throwing out of bounds errors.

Anyways, a better way to write your hook.

function hook() {
    $params = parse_params();

    $url = $_SERVER[ 'SCRIPT_NAME' ];
    $url = trim( $url, '/' );//remove forward slash from beginning and end of $url

    $urlArray = explode( '/', $url );
    $urlArray = array_pad( $urlArray, 2, NULL );
    list( $controller, $view ) = $urlArray;

    if( ! $controller ) { $controller = 'front'; }
    if( ! $view ) { $view = 'index'; }

    $route = compact( 'controller', 'view' );
}

Now if you want to extend to another level you would just adjust the padding, add to the list, add a default, and then add it to the compact.

Custom URLs

These are done through the htaccess file with mod_rewrite. At least, I think that's right. I haven't tried figuring this out yet, so I can't give you details, but it is well documented on the internet, just google it.

UPDATE

Looks good. I put an edit in one of the above sections because of a typo. You followed most of my previous advice, so there's not much I can add. I did just notice a couple of things though.

It looks as if you are passing your post and get data directly to your PHP scripts. This is bad. You should always validate and sanitize them. If your PHP version is >= 5.2 you can use a helpful PHP function called filter_input_array(), or filter_input().

It is best to only use the concatenate . operator if you are adding variables to a string. Even though it doesn't look like much, it is still an operator and takes processing power. Admitedly not much, but you should still code so that you aren't blattantly ignoring such minor inefficiencies. Not to say that you need to go out of your way to make your script as quick as possible. Far from it. But if its a quick fix that doesn't hurt anything, why not. It may not present much improvement now, but say you find yourself working on a script with a massive array that you need to loop over. At such a point every little efficiency matters. If you are already taking care of some of the easier stuff automatically it won't be as difficult to refactor the rest.

require( $_SERVER[ 'DOCUMENT_ROOT' ] . '/' . APP . '/models/general.php' );

Last but not least, don't just copy-paste suggestions into your code. I notice my coding style made it into your code. Take time to understand suggestions and write them yourself. As you found out, my answer was not 100% operational and you had to adjust it. Even if all you are doing is manually typing out these suggestions, without any change, you will find you will remember it a little better for the next time you need it and something might even click so that you understand it better.

Hope this helps!