1
\$\begingroup\$

I was wondering if you could provide some feedback on this Router class I built. Worth noting that it relies on an Apache rewrite rule that always redirects to this file, but passes the path into $_GET['url'] as a string (e.g. A request to http://foo.com/foo/bar) goes into $_GET['url] => 'foo/bar'.

You can then define routes that match against this path string, and the function defined with the route can execute any aribtrary code, like handing off to a controller.

Mainly I would like to know if I should be sanitizing any strings? How could I handle paths that don't match any defined routes?

class Router
{
    private $routes = null;
    private $req_path = null;

    function __construct()
    {
        if (isset($_GET['url']))
            $this->req_path = $_GET['url'];

        // Build the requested path.
        $this->req_path = is_null($this->req_path) ? "/" : "/" . $this->req_path;
    }

    // Create a route with a path and a function associated with that path that we
    // can call.
    function route($path, $handler)
    {
        $this->routes[] = array('path' => $path, 'handler' => $handler);
    }

    // Run the router to compare the request against any defined routes, run the 
    // associated function.
    function run()
    {
        if (is_null($this->routes))
            throw new RuntimeException("No routes have been defined");

        foreach($this->routes as $route) {
            if ($route['path'] == $this->req_path) {
                $route['handler']();
                return;
            }
        }
    }
}

$router = new Router();

$router->route("/steve", function() { 
    echo "steve"; 
});

$router->run();
\$\endgroup\$
2
\$\begingroup\$

Mainly I would like to know if I should be sanitizing any strings?

Assuming that the only unsafe input is in $_GET['url'], you're safe: junk that doesn't match a defined route will be ignored. The routes work effectively like a white list.

How could I handle paths that don't match any defined routes?

Return a 404 error page (and correctly set status code in the header).


This seems quite redundant:

function __construct()
{
    if (isset($_GET['url']))
        $this->req_path = $_GET['url'];

    // Build the requested path.
    $this->req_path = is_null($this->req_path) ? "/" : "/" . $this->req_path;
}

This should be equivalent and shorter:

function __construct()
{
    // Build the requested path.
    $this->req_path = '/' . @$_GET['url'];
}

Instead of iterating over the list of routes and stop when you find a match, why not use an associative array instead? Like this:

function route($path, $handler)
{
    $this->routes[$path] = $handler;
}

function run()
{
    if (is_null($this->routes))
        throw new RuntimeException("No routes have been defined");

    if (isset($this->routes[$this->req_path])) {
        $this->routes[$this->req_path]();
        return;
    }
}
\$\endgroup\$

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.