[Routing] fix matching host patterns, utf8 prefixes and non-capturing groups #27511
Conversation
|
I ran into the /**
* @Route(
* "/file/{filename}",
* name = "file_download",
* requirements = { "filename": ".+" },
* options = { "utf8": true }
* )
* @Method("GET")
*/
public function download($filename, LocalFileSystem $localFileSystem)
{
// ..
}Similar as https://symfony.com/blog/new-in-symfony-3-2-unicode-routing-support As soon as I remove the |
c16efb4 to
302492e
Compare
|
@nicolas-grekas yes exactly, I noticed you pointed there to this fix. Thanks! |
b01e533 to
36e4f29
Compare
|
PR is ready, sorry about the huge diff, its fixtures... |
36e4f29 to
3dc6639
Compare
| @@ -187,6 +185,11 @@ private function getCommonPrefix(string $prefix, string $anotherPrefix): array | ||
| } | ||
| } | ||
| restore_error_handler(); | ||
| if ($i < $end && 0b10 === (\ord($prefix[$i]) >> 6) && preg_match('//u', $prefix.' '.$anotherPrefix)) { |
There was a problem hiding this comment.
I would add a comment here explaining what that snippet of code do.
3dc6639 to
465b15c
Compare
|
Status: needs review |
|
Thank you @nicolas-grekas. |
…n-capturing groups (nicolas-grekas) This PR was merged into the 4.1 branch. Discussion ---------- [Routing] fix matching host patterns, utf8 prefixes and non-capturing groups | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27448, #27461, #27504, #27512 | License | MIT | Doc PR | - Commits ------- 465b15c [Routing] fix matching host patterns, utf8 prefixes and non-capturing groups
|
Shouldn't we consider using a proper parser for the Routing component? Similar to: https://github.com/rcs/route-parser |
|
@teohhanhui we are already parsing routes properly, and compiling them to a regex each (which is what this route-parser is doing) is battle-tested in symfony since many years. the hard thing here is that Symfony 4.1 is now generating only a few regexes combining routes together to make matching much faster. And the new logic is not yet as battle-tested for all edge-cases (and the fact that we combine routes creates much more weird cases than when matching them separately, as we need to think about how the cases interact together). |
|
Sorry, I must have misunderstood. Because I cannot find any parser in the Symfony Routing component: like I could in Twig: or the Symfony ExpressionLanguage component: |
Uh oh!
There was an error while loading. Please reload this page.