A very simple router for a PHP application

Question

/.htaccess

ErrorDocument 404 /inc/index.php
ErrorDocument 500 /inc/index.php

RewriteEngine ON
RewriteRule ^([a-z_-]+)$ index.php?page=$1 [L]

/index.php

<?php
if(isset($_GET['page'])) {
    switch ($_GET['page']) {
        case 'login':
            require 'inc/login.php';
            include 'tpl/login.tpl.php';
            break;
        case 'logout':
            require 'inc/logout.php';
            include 'tpl/logout.tpl.php';
            break;

        default:
            if ($_GET['page'] == '' || (empty($_GET))) {
                require 'inc/home.php';
                include 'tpl/home.tpl.php';
            } else {
                header('Location: /');
                exit;
            }
            break;
    }
} else {
    require 'inc/home.php';
    include 'tpl/home.tpl.php';
}
?>

/inc/index.php /tpl/index.php

<?php
header('Location: /');
exit;
?>

Could really do with a professional opinion, it seems to work but like to know if there is a more efficient way or reasons against. :)

This is a repost from the PHP group, I was told this would be a better place to find answers.

Answer 1

There are many takes at that. And the most suggested one is to use a ready-made framework. But assuming this code as is:

First of all, I don't see the real need for this router. Why not to request login.php directly and inside it include tpl/login.tpl.php? If you want to hide the .php extension (though not sure why), it's also possible with a simple .htaccess rule.

Speaking of the code itself

Simplifying the code

We can remove the extra nesting and duplication

<?php
$page = $_GET['page'] ?? 'home';
switch ($page) {
    case 'login':
        require 'inc/login.php';
        include 'tpl/login.tpl.php';
        break;
    case 'logout':
        require 'inc/logout.php';
        include 'tpl/logout.tpl.php';
        break;
    case 'home':
        require 'inc/home.php';
        include 'tpl/home.tpl.php';
        break;
    default:
        http_response_code(404);
}

I must note that checking for empty($_GET) is essentially useless because we are inside a condition that already made sure that $_GET is not empty.

Now we can't help but notice the pattern, and every pattern in the code must be deflated, by means of a loop or similar measure. Here we will use some string/array tricks

$pages = ['login', 'logout', 'home'];
$page = $_GET['page'] ?? 'home';
if (in_array($page, $pages)) {
    require "inc/$page.php";
    include "tpl/$page.tpl.php";
} else {
    http_response_code(404);
}

Making it extensible

What I don't like about this code is that it's static. It can serve only three pages. You'd have to add another branch/array item for the every page. Not very convenient. Why not to make it automatic?

$page = basename($_GET['page'] ?? 'home');
if (file_exists("inc/$page.php")) {
    require "inc/$page.php";
    include "tpl/$page.tpl.php";
} else {
    http_response_code(404);
}

An important note: I was reminded of the necessity to validate the page parameter when it's not whitelisted as in the codes above. basename() is a good start though one may consider a regexp.

Now you will only have to add a new file to inc folder and it will be served as well

The automation revisited

Through invaluable comments I was also reminded of more subtle details, such as SEO rule of not having two pages with same content. So more extensive validation is a good thing to add.

First of all I would get rid of that page thing. Your web-server already provides the requested resource, through REQUEST_URI variable. No need to pollute the query string with a useless variable that could be easily overwritten. So make your rule just RewriteRule ^([a-z_-]+)$ index.php [L,QSA] (the original query string is a good thing to keep, so we're adding QSA)

$page = parse_url($_SERVER['REQUEST_URI'], PHP_URL_PATH);
$page = trim($page, "/"); // original request may have them
if (in_array($page, ['index.php', 'home'], true)) {
    header("Location: /", true, 301);
    die; // always die if there is any code after Location:
}
$page = $page ?: 'home'; // make it home if empty

if (preg_match('~^[a-z_-]+$~', $page) && file_exists("inc/$page.php")) {
    require "inc/$page.php";
    include "tpl/$page.tpl.php";
} else {
    http_response_code(404);
}

Here we are making a uniform home page which is just /, and also impose a stricter validation for the page parameter.

Adhering to HTTP standards

HTTP status codes are not for decoration. They must be used in the exact situations they are intended for. When a page is not found, your server should respond with 404 status, not 302. I would advise to create corresponding pages (i.e. 404.php, 500.php) in the inc directory and include them along with setting appropriate HTTP status instead of redirect.

Answer 2

I'm just going to comment specifically on the first two lines of the htaccess.

ErrorDocument 404 /inc/index.php
ErrorDocument 500 /inc/index.php

That is asking for trouble, especially because index.php holds:

<?php
header('Location: /');
exit;
?>

Putting the first two lines of the htaccess together with the PHP they say "When a 404 or 500 error occurs, issue a 302 status code". Beyond being confusing to robots and thus potentially causing SEO issues, debugging in this situation is going to be a real pain. As Your Common Sense noted in their answer, the HTTP status codes have specific meanings, and it is best to use them as intended.

If I remember correctly (I don't have Apache available currently), I think you can use _SERVER['REDIRECT_STATUS'] to get the original server status code. So if you wanted a common error page you should be able to do something like:

$httpStatus = $_SERVER['REDIRECT_STATUS'] ?? 599;

// Propagate - not sure if needed
http_response_code((int)$httpStatus);

$errorMessage = match($httpStatus) {
    '404' => 'Page not found',
    '500' => 'Server error',
    '599' => 'Unknown error',
}

echo $errorMessage;

This would also not have the redirect code in it anymore.

Answer 3

Quite aside from the above comments it's worth reading up on the PSR1 PSR2 and PSR12 linting rules. It has simple rules like a space after the if keywords if ( and also no trailing ?> in a php only file.

The other main thing is avoid using the exit statement, if you wanted to test your code in something like phpunit the exit statement would not only kill your script but also the script that called it, leading to a lot of head scratching and wondering why php appeared to end so drastically. Something less drastic such as throw new Exception(); or if your script was in a function/method then a return.

Finally the require and include are quite rigid and inflexible you'll find as your program grows you can't move your php files around easily without refactor all of the other files that require them. Instead autoloading (PSR4 autoloading is the way to go) and namespaces are the way to go, although that would require your code to be wrapped in classes or function (when I was first learning php because some much of it is class focused it wasn't clear you can also load functions https://www.php.net/manual/en/language.namespaces.importing.php). Though to be fair that really depends where you envision your project going and if it's big enough to worth having classes or functions.