[Routing] Transform dynamic routes with static path variables to static routes #35821
Conversation
d93649f to
6517cc7
Compare
6517cc7 to
8f70316
Compare
| * @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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| @@ -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) { | |||
There was a problem hiding this comment.
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.
| @@ -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); | |||
There was a problem hiding this comment.
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.
|
As we found a better approach, I close that PR |
Implementation of idea mentioned here: #35735 (review)
Improve route matching performances by turning dynamic routes with static path variables to actual static routes.
To do if implementation is accepted