patternphpMinor
PHP routing system
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
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
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.
Typically those would not have extra indentation - so the asterisks on each line are aligned vertically - e.g.
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
Method length and simplification
The method
Instead of splicing together substrings, a simpler approach would be to use
And the
It seems that when the path is
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.