3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Split it up in more functions. \$\endgroup\$ – AlucardTheRipper May 20 '13 at 10:26
  • \$\begingroup\$ That's it? Any suggestions on the functions? \$\endgroup\$ – James May 20 '13 at 10:32
  • \$\begingroup\$ It's just a comment not an Answer. \$\endgroup\$ – AlucardTheRipper May 20 '13 at 11:24
  • \$\begingroup\$ @AlucardTheRipper Of course. So, in response to your comment, any suggestions on the functions (i.e. what for, etc). \$\endgroup\$ – James May 20 '13 at 11:30
3
\$\begingroup\$

The first thing I notice is that you are using the superglobals directly in your class. Normally you want to inject a request object into the class which contains all the info about the request.

The next thing I notice is the use of static methods, e.g.:

\Models\Session::checkSession()

and

Config::$defaultLoggedInController

Which not only makes you method / class hard to test, but also hard to maintain both for somebody else as for the future you. It makes it very hard to see the class / method has those dependencies by looking at the signatures.

I think if you leave the request parsing to your request object you not only clean up this method / class, but you are also following SRP better.

In general it would be nice to GRASP SOLID. To improve both readability and maintainability.

\$\endgroup\$
  • \$\begingroup\$ From what you're saying, I need to do the following?: (1) Pass in the $_SERVER superglobal to the class via DI. (2) Pass in the defaults via DI. (3) Do something ?? with the Session Model... (also pass it in via DI, and lose the 'static') If these are all correct, apart from the aforementioned architectural differences, is there anything code-wise specifically for the parsing you could suggest / help clean up? :) \$\endgroup\$ – James May 20 '13 at 11:49
  • 1
    \$\begingroup\$ You shouldn't pass in the SERVER superglobal directly, but an instance of a request class. Also I don't get why you would want to parse the querystring manually when your request class can simply leverage the $_GET superglobal. \$\endgroup\$ – PeeHaa May 20 '13 at 11:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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