Skip to main content
added 514 characters in body
Source Link
janos
  • 111.7k
  • 15
  • 152
  • 391

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;
    }
}

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;
    }
}

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;
    }
}
added 514 characters in body
Source Link
janos
  • 111.7k
  • 15
  • 152
  • 391

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;
    }
}

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'];
}

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;
    }
}
Source Link
janos
  • 111.7k
  • 15
  • 152
  • 391

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'];
}