HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

PHP routing system

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
phproutingsystem

Problem

I made a routing system for PHP inspired by Symfony's router composed of a few classes.

First I am using Symfony's HTTP Foundations component.
Then, I am emulating the classes in the routing component but with almost completely my own implementation (I did copy few lines).

The whole system is not simple so I won't be making it the focus of the question, though, this is the GitHub link, and I would be grateful for a full review (rip me apart).

I will provide the class that matches and parses the routes and I would like to know what I can improve.

There is a parser class:

```
getPath());

return ['variables' => $parsed['variables'], 'matcherReg' => $parsed['regex']];
}

/**
* Takes a string pattern, matches it with regexes
*
* @param string The pattern
*
* @return array Array with parsed values
*/
private static function parse($pattern)
{
$matches = '';
$variables = array();
$pos = 0;
$reg = '#'; //It seems that regex must start and end with a delimiter
$nextText = '';

if($pattern == '/')
{
$reg = '#^[\/]+$#';

return ['variables' => '', 'regex' => $reg];
}

//Check if generated regexes are stored, if so it skips the whole process
if(apc_exists($pattern))
{
$cacheI = apc_fetch($pattern);
return $cacheI;
}

//Extracts the variables enclosed in {}
preg_match_all('#\{\w+\}#', $pattern, $matches, PREG_OFFSET_CAPTURE | PREG_SET_ORDER);

//Puts each variable in array
//Uses the text before and after to create a regex for the rest of the pattern - $precedingText, $nextText
//If no wildcard is detected in the path it splits it into segments and compiles are regex
foreach ($matches as $match)
{
$varName = substr($match[0][0], 1, -1);
$pre

Solution

I apologize that nobody has reviewed this code in six years since it was posted. Perhaps you have learned a few things since then but hopefully the review below will still be helpful.
Array syntax

I like that the code takes advantage of the PHP 5.4 feature of short array syntax, though not in every array declaration - e.g. in RouteParser::parseRoute() the first declaration uses the legacy format while the return statement includes the shorthand format. There is nothing wrong with this but if this was my code then I would make it consistent.
Docblock format

While it may not exactly have a standard the widely accepted common docblock syntax contains no indentation for inner lines. Most of the docblocks used in the code contain extra indentation levels - e.g.

/**
    * Takes an instance of Routing\Route object
    * Extracts the variables from wildcards
    * Calculates a regex that can be used to match routes with urls
*/


Typically those would not have extra indentation - so the asterisks on each line are aligned vertically - e.g.

/**
 * Takes an instance of Routing\Route object
 * Extracts the variables from wildcards
 * Calculates a regex that can be used to match routes with urls
 */


New lines for all blocks

Adhering to a standard is not required but it does help anyone reading the code. Many PHP developers follow PSRs like PSR-12. While it wasn't approved until 2019, PSR-12 replaced PSR-2 which was approved in 2012.

I have grown accustom to starting class and method blocks on a new line (per PSR-12 section 4.4) but doing so for other control structures like loops and conditionals feels a bit excessive and is not in-line with PSR-12 section 5.

There MUST be one space between the closing parenthesis and the opening brace

Unused variable

In the method RouteParser::parseRoute() the variable $variables is declared but never used. It can be eliminated without effecting anything. This will help avoid confusion for anyone reading the code in the future - which may include yourself.
Method length and simplification

The method RouteParser::parse() is a bit on the long side. Some of the variables declared declared at the beginning are not used in the early returns and could be declared after those conditional returns - e.g. $matches, $variables, $pos, $nextText. Removing new lines before opening braces will decrease the length of that method.

Instead of splicing together substrings, a simpler approach would be to use str_replace() to simply replace what needs to be replaced. The sample below uses positive look-behind and positive look ahead to not capture the curly brackets - this allows those matches to be added directly to the list of variables.

$variables = array();
$reg = '#' . str_replace('/', '\\\\/', $pattern) . '#';
preg_match_all('#(?!\{)\w+(?=\})#', $pattern, $matches);
if (count($matches)) {
    $counts = array_count_values($matches[0]);
    $duplicates = array_filter($counts, function ($count) {
        return $count > 1;
    });
    if (count($duplicates)) {
        throw new \Exception(sprintf('More than one occurrence of variable(s): "%s".', implode(',',array_keys($duplicates))));
    }
    foreach ($matches[0] as $match) {
        $reg = str_replace("{" . $match . "}", '[a-zA-Z0-9]+', $reg);
        $variables[] = $match;
    }
}
apc_store($pattern, ['variables' => $variables, 'regex' => $reg]);

return ['variables' => $variables, 'regex' => $reg];


And the foreach loop could also be replaced with a single call to preg_replace_callback() though it would need to add the matches to $variables within the callback and some may find that impure for a functional approach.

$reg = preg_replace_callback('#\{\w+\}#', function($matches) use (&$variables) {
    $varName = substr($matches[0], 1, -1);
    if(is_numeric($varName)) {
        throw new \Exception('Argument cannot be a number');
    }
    if (in_array($varName, $variables)) {
        throw new \Exception(sprintf('More then one occurrence of variable name "%s".', $varName));
    }
    $variables[] = $varName;
    return '[a-zA-Z0-9]+';
}, $reg);


It seems that when the path is 'hi/{p1}/cicki/{p2}' then the regex pattern returned is '#\\/[a-zA-Z0-9]+\\/cicki\\/[a-zA-Z0-9]+#' (see this demo)- it seems like that should contain the 'hi' at the beginning but maybe you have a reason to remove that.

Code Snippets

/**
    * Takes an instance of Routing\Route object
    * Extracts the variables from wildcards
    * Calculates a regex that can be used to match routes with urls
*/
/**
 * Takes an instance of Routing\Route object
 * Extracts the variables from wildcards
 * Calculates a regex that can be used to match routes with urls
 */
$variables = array();
$reg = '#' . str_replace('/', '\\\\/', $pattern) . '#';
preg_match_all('#(?!\{)\w+(?=\})#', $pattern, $matches);
if (count($matches)) {
    $counts = array_count_values($matches[0]);
    $duplicates = array_filter($counts, function ($count) {
        return $count > 1;
    });
    if (count($duplicates)) {
        throw new \Exception(sprintf('More than one occurrence of variable(s): "%s".', implode(',',array_keys($duplicates))));
    }
    foreach ($matches[0] as $match) {
        $reg = str_replace("{" . $match . "}", '[a-zA-Z0-9]+', $reg);
        $variables[] = $match;
    }
}
apc_store($pattern, ['variables' => $variables, 'regex' => $reg]);

return ['variables' => $variables, 'regex' => $reg];
$reg = preg_replace_callback('#\{\w+\}#', function($matches) use (&$variables) {
    $varName = substr($matches[0], 1, -1);
    if(is_numeric($varName)) {
        throw new \Exception('Argument cannot be a number');
    }
    if (in_array($varName, $variables)) {
        throw new \Exception(sprintf('More then one occurrence of variable name "%s".', $varName));
    }
    $variables[] = $varName;
    return '[a-zA-Z0-9]+';
}, $reg);

Context

StackExchange Code Review Q#126267, answer score: 2

Revisions (0)

No revisions yet.