Note #2: This was a post from a while back. Although the top rated answer is useful (generally), I'm looking for something more specific to this issue including routing / architecture. I've even tried a bounty, which failed. As I don't have enough rep to keep adding bounties, I'll leave it open until someone decides to post :)
Note #1: My code currently works well and I've progressed further than this - however I feel that this router is, frankly, crap, and I'm looking for honest opinions and constructive criticism.
I'm in the process of creating my own framework for educational purposes, and for fun! This question pertains to the routing of my application using the front controller pattern.
.htaccess
Every request hits my .htaccess file which, using Apache's mod_rewrite, routes all requests through to my index.php file using ?url=.
<IfModule mod_rewrite.c>
# -------------------------------------------------------------------------
# This file forwards all requests through to index.php as long as the
# request is not for a file or directory. This uses ?url and index.php
# internally, so any requests including these add an __iserror=1 which the
# framework picks up in the routing and redirects to a 404 error page.
# -------------------------------------------------------------------------
# Turn mod_rewrite on (please make sure it's enabled in Apache)
RewriteEngine On
# Block direct access to "index.php" and query strings with "?url="/"&url="
RewriteCond %{QUERY_STRING} !^__iserror=1$
RewriteCond %{THE_REQUEST} ^(\S+)\ +/(\S+)\/index\.php\b [OR]
RewriteCond %{THE_REQUEST} (?:&|\?)url=
RewriteRule .* /framework/index.php?__iserror=1 [L]
# As long as we're not requesting access to a file...
RewriteCond %{REQUEST_FILENAME} !-f
# And we're also not requesting access to a directory...
RewriteCond %{REQUEST_FILENAME} !-d
# Route everything through /framework/index.php
RewriteRule .* /framework/index.php?url=$0 [L,QSA]
# -------------------------------------------------------------------------
</IfModule>
So the .htaccess passes the request to index.php with ?url=<request>, and an optional is_error=1 set if a user has manually appended ?url= or &url= into the url as a GET parameter.
The Router
The code I'm showing here is not for the actual dispatching of the request to the correct controller / method - it's for the parsing of the request to get the actual data. This is where I am foremost looking for suggestions.
/**
* Parses URL for routing purposes. Shows 404 if __iserror found in URL.
* Sets up default controller and action is none specified. Also handles
* any user defined routes in Core/CustomRoutes.php
*
* @return \Core\Router Instance of self for method chaining
*/
public function resolvePath()
{
/** __isserror check **/
/** Passed in via htaccess if URL contains index.php or ?url= **/
if (!empty($_GET['__iserror']))
{
header("HTTP/1.0 404 Not Found");
include('404.php');
exit();
}
$qs = $_SERVER['QUERY_STRING'];
/** If no controller requested, use defaults **/
if (empty($qs))
{
/** If already logged in, use dashboard controller **/
if (\Models\Session::checkSession())
{
$controller = Config::$defaultLoggedInController;
}
else
{
$controller = Config::$defaultController;
}
$action = Config::$defaultAction;
}
/** Strip out any GET vars **/
if (!empty($qs))
{
$query = explode('&', $qs);
/** Remove url if exists **/
foreach ($query as $key => $q)
{
if (strstr($q, 'url='))
{
unset($query[$key]);
}
}
/** Remove getVars from $query (not $qs, used elesewhere) **/
$remove = implode('&', $query);
if ($remove !== '')
{
$remove = sprintf('%s%s', '&', $remove);
$qs = str_replace($remove, '', $qs);
}
$getVars = array();
foreach ($query as $pair)
{
if (strstr($pair, '='))
{
list($key, $value) = explode('=', $pair);
$getVars[$key] = $value;
}
}
$this->getVars = $getVars;
/** Parse for controller / action **/
$request = array_filter(explode('/', $qs));
/** Only Controller Specified **/
if (count($request) === 1)
{
$controller = str_replace('url=', '', $request[0]);
$action = Config::$defaultAction;
}
/** Controller & Action Specified **/
if (count($request) >= 2)
{
$controller = str_replace('url=', '', array_shift($request));
$action = array_shift($request);
/** Remaining Params **/
if ($request)
{
foreach ($request as $param)
{
$this->params[] = $param;
}
}
}
}
/** Are there any user defined routes? If so, parse for them **/
/** Note: This is just a file containing an array, and it allows
/login to be mapped to /home/login, for example **/
if (isset($_SERVER['REDIRECT_URL']))
{
$customRoute = str_replace(PATH_ROOT, '', $_SERVER['REDIRECT_URL']);
if (array_key_exists($customRoute, customRoutes()))
{
$customRoute = array_values(array_filter(explode('/', customRoutes()[$customRoute])));
$controller = isset($customRoute[0]) ? $customRoute[0] : false;
$action = isset($customRoute[1]) ? $customRoute[1] : false;
}
}
/** Finally, set everything up at the end **/
$this->controller = $controller;
$this->action = $action;
/** Allow Method Chaining **/
return $this;
}
Hopefully the comments I placed are useful - they're pretty self explanatory. I want to know if there's a better way of getting from request to $this->controller / $this->action.
Main Questions
Firstly, the bloody thing is massive, and there are already issues with GET parameters in this (I have added to this code over time and not really properly re-factored it, so now it's just a mess). If I add ?x= to the url, I get a missing $controller and missing $action PHP error. I know I can fix this, but it feels like it'll just be a patch, and I really think the thing needs being written properly.
Before I get on with the rest of the framework (currently implementing a proper dependency injection container and making the whole thing very modular / easily testable), I really want to get this working properly.
Any suggestions, criticisms or even complete overhauls (within reason) will be very welcome.
