Routing in MVC with PHP

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:

```
Error Connecting to Database");
}
if(!mysql_select_db(DATABASE, $connection)) {
die("Database Does Not Exist");
}
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();

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 adju

StackExchange Code Review Q#13827, answer score: 7