Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Routing] fix matching host patterns, utf8 prefixes and non-capturing groups#27511
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
rmsint commentedJun 6, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I ran into the /** * @Route( * "/file/{filename}", * name = "file_download", * requirements = { "filename": ".+" }, * options = { "utf8": true } * ) * @Method("GET") */publicfunctiondownload($filename,LocalFileSystem$localFileSystem){// ..} Similar ashttps://symfony.com/blog/new-in-symfony-3-2-unicode-routing-support As soon as I remove the |
nicolas-grekas commentedJun 6, 2018
rmsint commentedJun 6, 2018
@nicolas-grekas yes exactly, I noticed you pointed there to this fix. Thanks! |
b01e533 to36e4f29Comparenicolas-grekas commentedJun 6, 2018
PR is ready, sorry about the huge diff, its fixtures... |
| } | ||
| } | ||
| restore_error_handler(); | ||
| if ($i <$end &&0b10 === (\ord($prefix[$i]) >>6) &&preg_match('//u',$prefix.''.$anotherPrefix)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would add a comment here explaining what that snippet of code do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
comment added
nicolas-grekas commentedJun 10, 2018
Status: needs review |
fabpot commentedJun 10, 2018
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
teohhanhui commentedJun 19, 2018
Shouldn't we consider using a proper parser for the Routing component? Similar to:https://github.com/rcs/route-parser |
stof commentedJun 19, 2018
@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). |
teohhanhui commentedJun 19, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.