Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[Routing] Transform dynamic routes with static path variables to static routes #35821

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

[Routing] Transform dynamic routes with static path variables to static routes #35821

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8f70316
[Routing] Convert static variables to text on compilation for generator
mtarld Feb 21, 2020
336a6f8
WIP: Implementation v2
mtarld Feb 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[Routing] Convert static variables to text on compilation for generator
  • Loading branch information
@mtarld
mtarld committed Feb 22, 2020
commit 8f703163b32ca779844a19e4a32e5d50077c97e8
6 changes: 4 additions & 2 deletions 6 src/Symfony/Component/Routing/Matcher/Dumper/CompiledUrlMatcherDumper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ private function groupStaticRoutes(RouteCollection $collection): array
$dynamicRoutes = new RouteCollection();

foreach ($collection->all() as $name => $route) {
$compiledRoute = $route->compile();
$compiledRoute = $route->compile(true);
$staticPrefix = rtrim($compiledRoute->getStaticPrefix(), '/');
$hostRegex = $compiledRoute->getHostRegex();
$regex = $compiledRoute->getRegex();
Expand All @@ -182,7 +182,9 @@ private function groupStaticRoutes(RouteCollection $collection): array

if (!$compiledRoute->getPathVariables()) {
$host = !$compiledRoute->getHostVariables() ? $route->getHost() : '';
$url = $route->getPath();
$url = preg_replace_callback('#{([^}]+)}#', static function (array $matches) use ($route) {
return $route->getRequirement($matches[1]);
}, $route->getPath());
if ($hasTrailingSlash) {
$url = substr($url, 0, -1);
}
Expand Down
73 changes: 53 additions & 20 deletions 73 src/Symfony/Component/Routing/Route.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,14 @@ class Route implements \Serializable
private $condition = '';

/**
* @var CompiledRoute|null
* @var CompiledRoute|null Route compiled for the matcher
*/
private $compiled;
private $matcherCompiled;

/**
* @var CompiledRoute|null Route compiled for the generator
*/
private $generatorCompiled;

/**
* Constructor.
Expand Down Expand Up @@ -73,7 +78,8 @@ public function __serialize(): array
'schemes' => $this->schemes,
'methods' => $this->methods,
'condition' => $this->condition,
'compiled' => $this->compiled,
'matcherCompiled' => $this->matcherCompiled,
'generatorCompiled' => $this->generatorCompiled,
];
}

Expand All @@ -98,8 +104,15 @@ public function __unserialize(array $data): void
if (isset($data['condition'])) {
$this->condition = $data['condition'];
}
if (isset($data['matcherCompiled'])) {
$this->matcherCompiled = $data['matcherCompiled'];
}
if (isset($data['generatorCompiled'])) {
$this->generatorCompiled = $data['generatorCompiled'];
}
// Make serialized representation of a route in older symfony version also works in current
if (isset($data['compiled'])) {
$this->compiled = $data['compiled'];
$this->matcherCompiled = $data['compiled'];
}
}

Expand Down Expand Up @@ -146,7 +159,8 @@ public function setPath(string $pattern)
// A pattern must start with a slash and must not have multiple slashes at the beginning because the
// generated path for this route would be confused with a network path, e.g. '//domain.com/path'.
$this->path = '/'.ltrim(trim($pattern), '/');
$this->compiled = null;
$this->matcherCompiled = null;
$this->generatorCompiled = null;

return $this;
}
Expand All @@ -171,7 +185,8 @@ public function getHost()
public function setHost(?string $pattern)
{
$this->host = (string) $pattern;
$this->compiled = null;
$this->matcherCompiled = null;
$this->generatorCompiled = null;

return $this;
}
Expand Down Expand Up @@ -200,7 +215,8 @@ public function getSchemes()
public function setSchemes($schemes)
{
$this->schemes = array_map('strtolower', (array) $schemes);
$this->compiled = null;
$this->matcherCompiled = null;
$this->generatorCompiled = null;

return $this;
}
Expand Down Expand Up @@ -239,7 +255,8 @@ public function getMethods()
public function setMethods($methods)
{
$this->methods = array_map('strtoupper', (array) $methods);
$this->compiled = null;
$this->matcherCompiled = null;
$this->generatorCompiled = null;

return $this;
}
Expand Down Expand Up @@ -282,7 +299,8 @@ public function addOptions(array $options)
foreach ($options as $name => $option) {
$this->options[$name] = $option;
}
$this->compiled = null;
$this->matcherCompiled = null;
$this->generatorCompiled = null;

return $this;
}
Expand All @@ -299,7 +317,8 @@ public function addOptions(array $options)
public function setOption(string $name, $value)
{
$this->options[$name] = $value;
$this->compiled = null;
$this->matcherCompiled = null;
$this->generatorCompiled = null;

return $this;
}
Expand Down Expand Up @@ -364,7 +383,8 @@ public function addDefaults(array $defaults)
foreach ($defaults as $name => $default) {
$this->defaults[$name] = $default;
}
$this->compiled = null;
$this->matcherCompiled = null;
$this->generatorCompiled = null;

return $this;
}
Expand Down Expand Up @@ -399,7 +419,8 @@ public function hasDefault(string $name)
public function setDefault(string $name, $default)
{
$this->defaults[$name] = $default;
$this->compiled = null;
$this->matcherCompiled = null;
$this->generatorCompiled = null;

return $this;
}
Expand Down Expand Up @@ -444,7 +465,8 @@ public function addRequirements(array $requirements)
foreach ($requirements as $key => $regex) {
$this->requirements[$key] = $this->sanitizeRequirement($key, $regex);
}
$this->compiled = null;
$this->matcherCompiled = null;
$this->generatorCompiled = null;

return $this;
}
Expand Down Expand Up @@ -477,7 +499,8 @@ public function hasRequirement(string $key)
public function setRequirement(string $key, string $regex)
{
$this->requirements[$key] = $this->sanitizeRequirement($key, $regex);
$this->compiled = null;
$this->matcherCompiled = null;
$this->generatorCompiled = null;

return $this;
}
Expand All @@ -502,30 +525,40 @@ public function getCondition()
public function setCondition(?string $condition)
{
$this->condition = (string) $condition;
$this->compiled = null;
$this->matcherCompiled = null;
$this->generatorCompiled = null;

return $this;
}

/**
* Compiles the route.
*
* @param bool $forGenerator If the route should be compiler for the generator
* @return CompiledRoute A CompiledRoute instance
*
* @throws \LogicException If the Route cannot be compiled because the
* path or host pattern is invalid
*
* @see RouteCompiler which is responsible for the compilation process
*/
public function compile()
public function compile(bool $forGenerator = false)
Copy link
Member

@nicolas-grekas nicolas-grekas Feb 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure to get why we need a different compilation result for generator vs dumper?
If we really need this, then I would prefer to deal with the optimization inside the dumper, so that no public API needs to change.
This should remain an internal implementation detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's unclear but I meant compiling differently for matching vs generation.
Generation shouldn't be modified because of strictRequirements and stuff. Therefore in fact I made a mistake, $forGeneration should instead be $forMatching.
I would rather as well keeping implementation inside the dumper.
I'll try another approach thanks

{
if (null !== $this->compiled) {
return $this->compiled;
$class = $this->getOption('compiler_class');

if ($forGenerator) {
if (null !== $this->generatorCompiled) {
return $this->generatorCompiled;
}

return $this->generatorCompiled = $class::compile($this, true);
}

$class = $this->getOption('compiler_class');
if (null !== $this->matcherCompiled) {
return $this->matcherCompiled;
}

return $this->compiled = $class::compile($this);
return $this->matcherCompiled = $class::compile($this, false);
}

private function sanitizeRequirement(string $key, string $regex)
Expand Down
33 changes: 30 additions & 3 deletions 33 src/Symfony/Component/Routing/RouteCompiler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,15 @@ class RouteCompiler implements RouteCompilerInterface
* @throws \DomainException if a variable name starts with a digit or if it is too long to be successfully used as
* a PCRE subpattern
*/
public static function compile(Route $route)
public static function compile(Route $route /**, bool $fixStaticVariables = false */)
{
$hostVariables = [];
$variables = [];
$hostRegex = null;
$hostTokens = [];

$fixStaticVariables = 1 < \func_num_args() ? (int) func_get_arg(1) : false;

if ('' !== $host = $route->getHost()) {
$result = self::compilePattern($route, $host, true);

Expand All @@ -63,7 +65,7 @@ public static function compile(Route $route)

$path = $route->getPath();

$result = self::compilePattern($route, $path, false);
$result = self::compilePattern($route, $path, false, $fixStaticVariables);

$staticPrefix = $result['staticPrefix'];

Expand Down Expand Up @@ -92,7 +94,7 @@ public static function compile(Route $route)
);
}

private static function compilePattern(Route $route, string $pattern, bool $isHost): array
private static function compilePattern(Route $route, string $pattern, bool $isHost, bool $fixStaticVariables = false): array
{
$tokens = [];
$variables = [];
Expand Down Expand Up @@ -184,6 +186,12 @@ private static function compilePattern(Route $route, string $pattern, bool $isHo
$regexp = self::transformCapturingGroupsToNonCapturings($regexp);
}

// Fixed variable, could be transformed as text token
if ($fixStaticVariables && preg_quote($regexp, self::REGEX_DELIMITER) === $regexp) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this really work?
if the static data is a.b, $regexp will be a\.b:

  • preg_quote($regexp, self::REGEX_DELIMITER) => a\\\.b !== $regexp

Instead, we should do preg_quote(unpreg_quote($regexp), self::REGEX_DELIMITER) === $regexp - with unpreg_quote() to be written.

$tokens[] = ['text', ($isSeparator ? $precedingChar : '').$regexp];
continue;
}

if ($important) {
$token = ['variable', $isSeparator ? $precedingChar : '', $regexp, $varName, false, true];
} else {
Expand All @@ -198,6 +206,10 @@ private static function compilePattern(Route $route, string $pattern, bool $isHo
$tokens[] = ['text', substr($pattern, $pos)];
}

if ($fixStaticVariables) {
$tokens = self::mergeContiguousTextTokens($tokens);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thre is a critical thing missing: the static value must be set as the default of the corresponding attribute.
Please add a few test cases that cover the need.

}

// find the first optional token
$firstOptional = PHP_INT_MAX;
if (!$isHost) {
Expand Down Expand Up @@ -237,6 +249,21 @@ private static function compilePattern(Route $route, string $pattern, bool $isHo
];
}

private static function mergeContiguousTextTokens(array $tokens): array
{
$mergedTokens = [$tokens[0]];
for ($i = 1; $i < \count($tokens); ++$i) {
if ('text' !== $tokens[$i][0] || 'variable' === end($mergedTokens)[0]) {
$mergedTokens[] = $tokens[$i];
continue;
}

$mergedTokens[\count($mergedTokens) - 1][1] .= $tokens[$i][1];
}

return $mergedTokens;
}

/**
* Determines the longest static prefix possible for a route.
*/
Expand Down
3 changes: 2 additions & 1 deletion 3 src/Symfony/Component/Routing/RouteCompilerInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,11 @@ interface RouteCompilerInterface
/**
* Compiles the current route instance.
*
* @param $fixStaticVariables Tell the compiler to convert static variables to text
* @return CompiledRoute A CompiledRoute instance
*
* @throws \LogicException If the Route cannot be compiled because the
* path or host pattern is invalid
*/
public static function compile(Route $route);
public static function compile(Route $route /**, bool $fixStaticVariables = false */);
}
13 changes: 5 additions & 8 deletions 13 src/Symfony/Component/Routing/Tests/Fixtures/dumper/compiled_url_matcher8.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,22 +8,19 @@
return [
false, // $matchHost
[ // $staticRoutes
'/a' => [[['_route' => 'a'], null, null, null, false, false, null]],
],
[ // $regexpList
0 => '{^(?'
.'|/(a)(*:11)'
.')/?$}sD',
.'|/(.)(*:11)'
.')/?$}sDu',
11 => '{^(?'
.'|/(.)(*:22)'
.')/?$}sDu',
22 => '{^(?'
.'|/(.)(*:33)'
.')/?$}sD',
],
[ // $dynamicRoutes
11 => [[['_route' => 'a'], ['a'], null, null, false, true, null]],
22 => [[['_route' => 'b'], ['a'], null, null, false, true, null]],
33 => [
11 => [[['_route' => 'b'], ['a'], null, null, false, true, null]],
22 => [
[['_route' => 'c'], ['a'], null, null, false, true, null],
[null, null, null, null, false, false, 0],
],
Expand Down
Loading
Morty Proxy This is a proxified and sanitized view of the page, visit original site.