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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Routing] fix matching host patterns, utf8 prefixes and non-capturing groups #27511

Merged
merged 1 commit into from Jun 10, 2018

Conversation

Projects
None yet
6 participants
@nicolas-grekas
Member

nicolas-grekas commented Jun 6, 2018

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 -
@rmsint

This comment has been minimized.

rmsint commented Jun 6, 2018

I ran into the preg_match(): Compilation failed: ... unmatched parentheses ... error and tried this fix. I could narrow it down to a utf8 route:

/**
 * @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 options = { "utf8": true } from the route my tests pass again and the route matcher does not throw the preg_match(): ... unmatched parentheses anymore.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 6, 2018

@rmsint see #27504, that's the issue you're experiencing. I'll add a fix for it here soon.

@rmsint

This comment has been minimized.

rmsint commented Jun 6, 2018

@nicolas-grekas yes exactly, I noticed you pointed there to this fix. Thanks!

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 6, 2018

PR is ready, sorry about the huge diff, its fixtures...

@@ -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)) {

This comment has been minimized.

@fabpot

fabpot Jun 8, 2018

Member

I would add a comment here explaining what that snippet of code do.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 8, 2018

Member

comment added

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 10, 2018

Status: needs review

@fabpot

fabpot approved these changes Jun 10, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Jun 10, 2018

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 465b15c into symfony:4.1 Jun 10, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jun 10, 2018

bug #27511 [Routing] fix matching host patterns, utf8 prefixes and no…
…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

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:route-fix-host branch Jun 10, 2018

@teohhanhui

This comment has been minimized.

Contributor

teohhanhui commented Jun 19, 2018

@stof

This comment has been minimized.

Member

stof commented Jun 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).
The route-parser you linked too will match a single route, not a full collection of routes to find the matching one.

@teohhanhui

This comment has been minimized.

Contributor

teohhanhui commented Jun 19, 2018

Sorry, I must have misunderstood. Because I cannot find any parser in the Symfony Routing component:
https://github.com/symfony/routing/search?q=parse&unscoped_q=parse

like I could in Twig:
https://github.com/twigphp/Twig/search?q=parse&unscoped_q=parse

or the Symfony ExpressionLanguage component:
https://github.com/symfony/expression-language/search?q=parse&unscoped_q=parse

@fabpot fabpot referenced this pull request Jun 25, 2018

Merged

Release v4.1.1 #27706

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment