This tool is currently proof-of-concept. Your feedback and evaluation is valuable in helping to improve it and ensure its reports are meaninful.
Please click here to complete a short survey to tell us what you think. It should take less than 5 minutes and help further this research project!
Symfony\Component\Routing\Matcher\Dumper\CompiledUrlMatcherDumper
Detected issues
| Issue | Method | Line number |
|---|---|---|
| Use of static methods | getVars | 266 |
| Use of static methods | string | 466 |
Code
Click highlighted lines for details
<?php/** This file is part of the Symfony package.** (c) Fabien Potencier <fabien@symfony.com>** For the full copyright and license information, please view the LICENSE* file that was distributed with this source code.*/namespace Symfony\Component\Routing\Matcher\Dumper;use Symfony\Component\ExpressionLanguage\ExpressionFunctionProviderInterface;use Symfony\Component\ExpressionLanguage\ExpressionLanguage;use Symfony\Component\Routing\Route;use Symfony\Component\Routing\RouteCollection;/*** CompiledUrlMatcherDumper creates PHP arrays to be used with CompiledUrlMatcher.** @author Fabien Potencier <fabien@symfony.com>* @author Tobias Schultze <http://tobion.de>* @author Arnaud Le Blanc <arnaud.lb@gmail.com>* @author Nicolas Grekas <p@tchwork.com>*/class CompiledUrlMatcherDumper extends MatcherDumper{private $expressionLanguage;private $signalingException;/*** @var ExpressionFunctionProviderInterface[]*/private $expressionLanguageProviders = [];/*** {@inheritdoc}*/public function dump(array $options = []){return <<<EOF<?php/*** This file has been auto-generated* by the Symfony Routing Component.*/return [{$this->generateCompiledRoutes()}];EOF;}public function addExpressionLanguageProvider(ExpressionFunctionProviderInterface $provider){$this->expressionLanguageProviders[] = $provider;}/*** Generates the arrays for CompiledUrlMatcher's constructor.*/public function getCompiledRoutes(bool $forDump = false): array{// Group hosts by same-suffix, re-order when possible$matchHost = false;$routes = new StaticPrefixCollection();foreach ($this->getRoutes()->all() as $name => $route) {if ($host = $route->getHost()) {$matchHost = true;$host = '/'.strtr(strrev($host), '}.{', '(/)');}$routes->addRoute($host ?: '/(.*)', [$name, $route]);}if ($matchHost) {$compiledRoutes = [true];$routes = $routes->populateCollection(new RouteCollection());} else {$compiledRoutes = [false];$routes = $this->getRoutes();}[$staticRoutes, $dynamicRoutes] = $this->groupStaticRoutes($routes);$conditions = [null];$compiledRoutes[] = $this->compileStaticRoutes($staticRoutes, $conditions);$chunkLimit = \count($dynamicRoutes);while (true) {try {$this->signalingException = new \RuntimeException('Compilation failed: regular expression is too large');$compiledRoutes = array_merge($compiledRoutes, $this->compileDynamicRoutes($dynamicRoutes, $matchHost, $chunkLimit, $conditions));break;} catch (\Exception $e) {if (1 < $chunkLimit && $this->signalingException === $e) {$chunkLimit = 1 + ($chunkLimit >> 1);continue;}throw $e;}}if ($forDump) {$compiledRoutes[2] = $compiledRoutes[4];}unset($conditions[0]);if ($conditions) {foreach ($conditions as $expression => $condition) {$conditions[$expression] = "case {$condition}: return {$expression};";}$checkConditionCode = <<<EOFstatic function (\$condition, \$context, \$request) { // \$checkConditionswitch (\$condition) {{$this->indent(implode("\n", $conditions), 3)}}}EOF;$compiledRoutes[4] = $forDump ? $checkConditionCode.",\n" : eval('return '.$checkConditionCode.';');} else {$compiledRoutes[4] = $forDump ? " null, // \$checkCondition\n" : null;}return $compiledRoutes;}private function generateCompiledRoutes(): string{[$matchHost, $staticRoutes, $regexpCode, $dynamicRoutes, $checkConditionCode] = $this->getCompiledRoutes(true);$code = self::export($matchHost).', // $matchHost'."\n";$code .= '[ // $staticRoutes'."\n";foreach ($staticRoutes as $path => $routes) {$code .= sprintf(" %s => [\n", self::export($path));foreach ($routes as $route) {$code .= sprintf(" [%s, %s, %s, %s, %s, %s, %s],\n", ...array_map([__CLASS__, 'export'], $route));}$code .= " ],\n";}$code .= "],\n";$code .= sprintf("[ // \$regexpList%s\n],\n", $regexpCode);$code .= '[ // $dynamicRoutes'."\n";foreach ($dynamicRoutes as $path => $routes) {$code .= sprintf(" %s => [\n", self::export($path));foreach ($routes as $route) {$code .= sprintf(" [%s, %s, %s, %s, %s, %s, %s],\n", ...array_map([__CLASS__, 'export'], $route));}$code .= " ],\n";}$code .= "],\n";$code = preg_replace('/ => \[\n (\[.+?),\n \],/', ' => [$1],', $code);return $this->indent($code, 1).$checkConditionCode;}/*** Splits static routes from dynamic routes, so that they can be matched first, using a simple switch.*/private function groupStaticRoutes(RouteCollection $collection): array{$staticRoutes = $dynamicRegex = [];$dynamicRoutes = new RouteCollection();foreach ($collection->all() as $name => $route) {$compiledRoute = $route->compile();$staticPrefix = rtrim($compiledRoute->getStaticPrefix(), '/');$hostRegex = $compiledRoute->getHostRegex();$regex = $compiledRoute->getRegex();if ($hasTrailingSlash = '/' !== $route->getPath()) {$pos = strrpos($regex, '$');$hasTrailingSlash = '/' === $regex[$pos - 1];$regex = substr_replace($regex, '/?$', $pos - $hasTrailingSlash, 1 + $hasTrailingSlash);}if (!$compiledRoute->getPathVariables()) {$host = !$compiledRoute->getHostVariables() ? $route->getHost() : '';$url = $route->getPath();if ($hasTrailingSlash) {$url = substr($url, 0, -1);}foreach ($dynamicRegex as [$hostRx, $rx, $prefix]) {if (('' === $prefix || str_starts_with($url, $prefix)) && (preg_match($rx, $url) || preg_match($rx, $url.'/')) && (!$host || !$hostRx || preg_match($hostRx, $host))) {$dynamicRegex[] = [$hostRegex, $regex, $staticPrefix];$dynamicRoutes->add($name, $route);continue 2;}}$staticRoutes[$url][$name] = [$route, $hasTrailingSlash];} else {$dynamicRegex[] = [$hostRegex, $regex, $staticPrefix];$dynamicRoutes->add($name, $route);}}return [$staticRoutes, $dynamicRoutes];}/*** Compiles static routes in a switch statement.** Condition-less paths are put in a static array in the switch's default, with generic matching logic.* Paths that can match two or more routes, or have user-specified conditions are put in separate switch's cases.** @throws \LogicException*/private function compileStaticRoutes(array $staticRoutes, array &$conditions): array{if (!$staticRoutes) {return [];}$compiledRoutes = [];foreach ($staticRoutes as $url => $routes) {$compiledRoutes[$url] = [];foreach ($routes as $name => [$route, $hasTrailingSlash]) {$compiledRoutes[$url][] = $this->compileRoute($route, $name, (!$route->compile()->getHostVariables() ? $route->getHost() : $route->compile()->getHostRegex()) ?: null, $hasTrailingSlash, false, $conditions);}}return $compiledRoutes;}/*** Compiles a regular expression followed by a switch statement to match dynamic routes.** The regular expression matches both the host and the pathinfo at the same time. For stellar performance,* it is built as a tree of patterns, with re-ordering logic to group same-prefix routes together when possible.** Patterns are named so that we know which one matched (https://pcre.org/current/doc/html/pcre2syntax.html#SEC23).* This name is used to "switch" to the additional logic required to match the final route.** Condition-less paths are put in a static array in the switch's default, with generic matching logic.* Paths that can match two or more routes, or have user-specified conditions are put in separate switch's cases.** Last but not least:* - Because it is not possible to mix unicode/non-unicode patterns in a single regexp, several of them can be generated.* - The same regexp can be used several times when the logic in the switch rejects the match. When this happens, the* matching-but-failing subpattern is excluded by replacing its name by "(*F)", which forces a failure-to-match.* To ease this backlisting operation, the name of subpatterns is also the string offset where the replacement should occur.*/private function compileDynamicRoutes(RouteCollection $collection, bool $matchHost, int $chunkLimit, array &$conditions): array{if (!$collection->all()) {return [[], [], ''];}$regexpList = [];$code = '';$state = (object) ['regexMark' => 0,'regex' => [],'routes' => [],'mark' => 0,'markTail' => 0,'hostVars' => [],'vars' => [],];Static methods
Summary of issues
- Tight Coupling
- Hidden dependencies
- Global state (if also using static variables)
Tight Coupling
Use of static methods always reduces flexibility by introducing tight coupling[1]. A static method tightly couples the calling code to the specific class the method exists in.
function totalAbs(double value, double value2) {
return abs(value) + abs(value2);
}Here, the method
totalAbshas a dependency on theMathclass and the.abs()method will always be called. Although for testing purposes this may not be a problem, the coupling reduces flexibility because thetotalmethod can only work with doubles/integers, as that's all theMath.abs()function can use. Although type coercion will allow the use of any primitive numeric type, these types have limitations. It's impossible to use another class such asBigIntegeror a class for dealing with greater precision decimals or even alternative numbering systems such as Roman numerals.The totalAbs function takes two doubles and converts them to their absolute values before adding them. This is inflexible because it only works with doubles. It's tied to doubles because that's what the
Math.abs()static method requires. If, instead, using OOP an interface was created to handle any number that had this method:interface Numeric {
public function abs(): Numeric;
}It would then be possible to rewrite the
totalAbsmethod to work with any kind of number:function totalAbs(Numeric value, Numeric value): Numeric {
return value.abs() + value2.abs();
}
By removing the static method and using an instance method in its place the
totalAbsmethod is now agnostic about the type of number it is dealing with. It could be called with any of the following (assuming they implement theNumericinterface)
totalAbs(new Integer(4), new Integer(-53));
totalAbs(new Double(34.4), new Integer(-2));
totalAbs(new BigInteger('123445454564765739878989343225778'), new Integer(2343));
totalAbs(new RomanNumeral('VII'), new RomanNumeral('CXV'));
Making the method reusable in a way that it wasn't when static methods were being used. By changing the static methods to instance methods, flexibility has been enhanced as the method can be used with any numeric type, not just numeric types that are supported by the
Math.abs()method.Broken encapsulation
Static methods also break encapsulation. Encapsulation is defined by Rogers[2] as:
the bundling of data with the methods that operate on that dataBy passing the numeric value into the
absmethod, the data being operated on is being separated from the methods that operate on it, breaking encapsulation. Instead usingnum.abs()the data is encapsulated in thenuminstance and its type is not visible or relevant to the outside world.abs()will work on the data and work regardless ofnum's type, providing it implements theabsmethod.This is a simple example, but applies to all static methods. Use of polymorphic instance methods that work on encapsulated data will always be more flexible than static method calls which can only ever deal with specific pre-defined types.
Further reading
- What is tight coupling?
- Why Static is Bad and How to Avoid It
- Static Methods Will Shock You
- Flaw: Brittle Global State & Singletons
- Static Methods are Death to Testability
Exceptions
The only exception to this rule is when a static method is used for object creation in place of the
newkeyword[3]. This is because thenewkeyword is already a static call. However, even here a non-static factory is often preferable for testing purposes[4][5].References
- Popov, N. (2014) Don't be STUPID: GRASP SOLID! [online]. Available from: https://nikic.github.io/2011/12/27/Dont-be-STUPID-GRASP-SOLID.html
- Rogers, P. (2001) Encapsulation is not information hiding [online]. Available from: http://www.javaworld.com/article/2075271/core-java/encapsulation-is-not-information-hiding.html
- Sonmez, J. (2010) Static Methods Will Shock You [online]. Available from: http://simpleprogrammer.com/2010/01/29/static-methods-will-shock-you/
- Hevery, M. (2008) Static Methods are Death to Testability [online]. Available from: http://misko.hevery.com/2008/12/15/static-methods-are-death-to-testability/
- Butler, T. (2013) Are Static Methods/Variables bad practice? [online]. Available from: https://r.je/static-methods-bad-practice.html
if ('_route' === $m[1]) {return '?:';}$state->vars[] = $m[1];return '';};$chunkSize = 0;$prev = null;$perModifiers = [];foreach ($collection->all() as $name => $route) {preg_match('#[a-zA-Z]*$#', $route->compile()->getRegex(), $rx);if ($chunkLimit < ++$chunkSize || $prev !== $rx[0] && $route->compile()->getPathVariables()) {$chunkSize = 1;$routes = new RouteCollection();$perModifiers[] = [$rx[0], $routes];$prev = $rx[0];}$routes->add($name, $route);}foreach ($perModifiers as [$modifiers, $routes]) {$prev = false;$perHost = [];foreach ($routes->all() as $name => $route) {$regex = $route->compile()->getHostRegex();if ($prev !== $regex) {$routes = new RouteCollection();$perHost[] = [$regex, $routes];$prev = $regex;}$routes->add($name, $route);}$prev = false;$rx = '{^(?';$code .= "\n {$state->mark} => ".self::export($rx);$startingMark = $state->mark;$state->mark += \strlen($rx);$state->regex = $rx;foreach ($perHost as [$hostRegex, $routes]) {if ($matchHost) {if ($hostRegex) {preg_match('#^.\^(.*)\$.[a-zA-Z]*$#', $hostRegex, $rx);$state->vars = [];$hostRegex = '(?i:'.preg_replace_callback('#\?P<([^>]++)>#', $state->getVars, $rx[1]).')\.';$state->hostVars = $state->vars;} else {$hostRegex = '(?:(?:[^./]*+\.)++)';$state->hostVars = [];}$state->mark += \strlen($rx = ($prev ? ')' : '')."|{$hostRegex}(?");$code .= "\n .".self::export($rx);$state->regex .= $rx;$prev = true;}$tree = new StaticPrefixCollection();foreach ($routes->all() as $name => $route) {preg_match('#^.\^(.*)\$.[a-zA-Z]*$#', $route->compile()->getRegex(), $rx);$state->vars = [];$regex = preg_replace_callback('#\?P<([^>]++)>#', $state->getVars, $rx[1]);if ($hasTrailingSlash = '/' !== $regex && '/' === $regex[-1]) {$regex = substr($regex, 0, -1);}$hasTrailingVar = (bool) preg_match('#\{\w+\}/?$#', $route->getPath());$tree->addRoute($regex, [$name, $regex, $state->vars, $route, $hasTrailingSlash, $hasTrailingVar]);}$code .= $this->compileStaticPrefixCollection($tree, $state, 0, $conditions);}if ($matchHost) {$code .= "\n .')'";$state->regex .= ')';}$rx = ")/?$}{$modifiers}";$code .= "\n .'{$rx}',";$state->regex .= $rx;$state->markTail = 0;// if the regex is too large, throw a signaling exception to recompute with smaller chunk sizeset_error_handler(function ($type, $message) { throw str_contains($message, $this->signalingException->getMessage()) ? $this->signalingException : new \ErrorException($message); });try {preg_match($state->regex, '');} finally {restore_error_handler();}$regexpList[$startingMark] = $state->regex;}$state->routes[$state->mark][] = [null, null, null, null, false, false, 0];unset($state->getVars);return [$regexpList, $state->routes, $code];}/*** Compiles a regexp tree of subpatterns that matches nested same-prefix routes.** @param \stdClass $state A simple state object that keeps track of the progress of the compilation,* and gathers the generated switch's "case" and "default" statements*/private function compileStaticPrefixCollection(StaticPrefixCollection $tree, \stdClass $state, int $prefixLen, array &$conditions): string{$code = '';$prevRegex = null;$routes = $tree->getRoutes();foreach ($routes as $i => $route) {if ($route instanceof StaticPrefixCollection) {$prevRegex = null;$prefix = substr($route->getPrefix(), $prefixLen);$state->mark += \strlen($rx = "|{$prefix}(?");$code .= "\n .".self::export($rx);$state->regex .= $rx;$code .= $this->indent($this->compileStaticPrefixCollection($route, $state, $prefixLen + \strlen($prefix), $conditions));$code .= "\n .')'";$state->regex .= ')';++$state->markTail;continue;}[$name, $regex, $vars, $route, $hasTrailingSlash, $hasTrailingVar] = $route;$compiledRoute = $route->compile();$vars = array_merge($state->hostVars, $vars);if ($compiledRoute->getRegex() === $prevRegex) {$state->routes[$state->mark][] = $this->compileRoute($route, $name, $vars, $hasTrailingSlash, $hasTrailingVar, $conditions);continue;}$state->mark += 3 + $state->markTail + \strlen($regex) - $prefixLen;$state->markTail = 2 + \strlen($state->mark);$rx = sprintf('|%s(*:%s)', substr($regex, $prefixLen), $state->mark);$code .= "\n .".self::export($rx);$state->regex .= $rx;$prevRegex = $compiledRoute->getRegex();$state->routes[$state->mark] = [$this->compileRoute($route, $name, $vars, $hasTrailingSlash, $hasTrailingVar, $conditions)];}return $code;}/*** Compiles a single Route to PHP code used to match it against the path info.*/private function compileRoute(Route $route, string $name, $vars, bool $hasTrailingSlash, bool $hasTrailingVar, array &$conditions): array{$defaults = $route->getDefaults();if (isset($defaults['_canonical_route'])) {$name = $defaults['_canonical_route'];unset($defaults['_canonical_route']);}if ($condition = $route->getCondition()) {$condition = $this->getExpressionLanguage()->compile($condition, ['context', 'request']);$condition = $conditions[$condition] ?? $conditions[$condition] = (str_contains($condition, '$request') ? 1 : -1) * \count($conditions);} else {$condition = null;}return [['_route' => $name] + $defaults,$vars,array_flip($route->getMethods()) ?: null,array_flip($route->getSchemes()) ?: null,$hasTrailingSlash,$hasTrailingVar,$condition,];}private function getExpressionLanguage(): ExpressionLanguage{if (null === $this->expressionLanguage) {if (!class_exists(ExpressionLanguage::class)) {throw new \LogicException('Unable to use expressions as the Symfony ExpressionLanguage component is not installed.');}$this->expressionLanguage = new ExpressionLanguage(null, $this->expressionLanguageProviders);}return $this->expressionLanguage;}private function indent(string $code, int $level = 1): string{return preg_replace('/^./m', str_repeat(' ', $level).'$0', $code);}/*** @internal*/Static methods
Summary of issues
- Tight Coupling
- Hidden dependencies
- Global state (if also using static variables)
Tight Coupling
Use of static methods always reduces flexibility by introducing tight coupling[1]. A static method tightly couples the calling code to the specific class the method exists in.
function totalAbs(double value, double value2) {
return abs(value) + abs(value2);
}Here, the method
totalAbshas a dependency on theMathclass and the.abs()method will always be called. Although for testing purposes this may not be a problem, the coupling reduces flexibility because thetotalmethod can only work with doubles/integers, as that's all theMath.abs()function can use. Although type coercion will allow the use of any primitive numeric type, these types have limitations. It's impossible to use another class such asBigIntegeror a class for dealing with greater precision decimals or even alternative numbering systems such as Roman numerals.The totalAbs function takes two doubles and converts them to their absolute values before adding them. This is inflexible because it only works with doubles. It's tied to doubles because that's what the
Math.abs()static method requires. If, instead, using OOP an interface was created to handle any number that had this method:interface Numeric {
public function abs(): Numeric;
}It would then be possible to rewrite the
totalAbsmethod to work with any kind of number:function totalAbs(Numeric value, Numeric value): Numeric {
return value.abs() + value2.abs();
}
By removing the static method and using an instance method in its place the
totalAbsmethod is now agnostic about the type of number it is dealing with. It could be called with any of the following (assuming they implement theNumericinterface)
totalAbs(new Integer(4), new Integer(-53));
totalAbs(new Double(34.4), new Integer(-2));
totalAbs(new BigInteger('123445454564765739878989343225778'), new Integer(2343));
totalAbs(new RomanNumeral('VII'), new RomanNumeral('CXV'));
Making the method reusable in a way that it wasn't when static methods were being used. By changing the static methods to instance methods, flexibility has been enhanced as the method can be used with any numeric type, not just numeric types that are supported by the
Math.abs()method.Broken encapsulation
Static methods also break encapsulation. Encapsulation is defined by Rogers[2] as:
the bundling of data with the methods that operate on that dataBy passing the numeric value into the
absmethod, the data being operated on is being separated from the methods that operate on it, breaking encapsulation. Instead usingnum.abs()the data is encapsulated in thenuminstance and its type is not visible or relevant to the outside world.abs()will work on the data and work regardless ofnum's type, providing it implements theabsmethod.This is a simple example, but applies to all static methods. Use of polymorphic instance methods that work on encapsulated data will always be more flexible than static method calls which can only ever deal with specific pre-defined types.
Further reading
- What is tight coupling?
- Why Static is Bad and How to Avoid It
- Static Methods Will Shock You
- Flaw: Brittle Global State & Singletons
- Static Methods are Death to Testability
Exceptions
The only exception to this rule is when a static method is used for object creation in place of the
newkeyword[3]. This is because thenewkeyword is already a static call. However, even here a non-static factory is often preferable for testing purposes[4][5].References
- Popov, N. (2014) Don't be STUPID: GRASP SOLID! [online]. Available from: https://nikic.github.io/2011/12/27/Dont-be-STUPID-GRASP-SOLID.html
- Rogers, P. (2001) Encapsulation is not information hiding [online]. Available from: http://www.javaworld.com/article/2075271/core-java/encapsulation-is-not-information-hiding.html
- Sonmez, J. (2010) Static Methods Will Shock You [online]. Available from: http://simpleprogrammer.com/2010/01/29/static-methods-will-shock-you/
- Hevery, M. (2008) Static Methods are Death to Testability [online]. Available from: http://misko.hevery.com/2008/12/15/static-methods-are-death-to-testability/
- Butler, T. (2013) Are Static Methods/Variables bad practice? [online]. Available from: https://r.je/static-methods-bad-practice.html
{if (null === $value) {return 'null';}if (!\is_array($value)) {if (\is_object($value)) {throw new \InvalidArgumentException('Symfony\Component\Routing\Route cannot contain objects.');}return str_replace("\n", '\'."\n".\'', var_export($value, true));}if (!$value) {return '[]';}$i = 0;$export = '[';foreach ($value as $k => $v) {if ($i === $k) {++$i;} else {$export .= self::export($k).' => ';if (\is_int($k) && $i < $k) {$i = 1 + $k;}}$export .= self::export($v).', ';}return substr_replace($export, ']', -2);}}