[Routing] Fix matching of utf8 params #42159

mazanax · 2021-07-16T14:34:43Z

Q	A
Branch?	5.3
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #41909
License	MIT
Doc PR

Change Regexp for routes with UTF-8 params.

carsonbot · 2021-07-16T14:34:47Z

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 5.4 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

Foxprodev · 2021-07-16T15:19:36Z

src/Symfony/Component/Routing/RouteCompiler.php

@@ -122,7 +122,8 @@ private static function compilePattern(Route $route, string $pattern, bool $isHo

        // Match all variables enclosed in "{}" and iterate over them. But we only want to match the innermost variable
        // in case of nested "{}", e.g. {foo{bar}}. This in ensured because \w does not match "{" or "}" itself.
-        preg_match_all('#\{(!)?(\w+)\}#', $pattern, $matches, \PREG_OFFSET_CAPTURE | \PREG_SET_ORDER);
+        $routeParamsPattern = $needsUtf8 ? '#\{(!)?([\p{L}_]+)\}#u' : '#\{(!)?(\w+)\}#';


Fixed, thanks

I am not sure 100% sure, but \w with u flag should be enough. Am I wrong?

\w doesn't support unicode characters.
Here is example:

\w - https://regex101.com/r/p9O5Qd/1

[\p{L}_] - https://regex101.com/r/QmU3Ru/1

There is no unicode flag in your snippet

OskarStark · 2021-07-16T18:50:30Z

src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php

+        } catch (ResourceNotFoundException $e) {
+        }
+
+        $this->assertEquals(['_route' => 'foo', 'bär' => 'baz'], $matcher->match('/foo/baz'));


assertSame whenever possible

I'm not sure that it's possible to use assertSame here. $matcher->match() doesn't guarantee the order of elements in the array. And assertEquals ignores it.

nicolas-grekas · 2021-07-19T07:56:58Z

src/Symfony/Component/Routing/RouteCompiler.php

@@ -122,7 +122,8 @@ private static function compilePattern(Route $route, string $pattern, bool $isHo

        // Match all variables enclosed in "{}" and iterate over them. But we only want to match the innermost variable
        // in case of nested "{}", e.g. {foo{bar}}. This in ensured because \w does not match "{" or "}" itself.
-        preg_match_all('#\{(!)?(\w+)\}#', $pattern, $matches, \PREG_OFFSET_CAPTURE | \PREG_SET_ORDER);
+        $routeParamsPattern = $needsUtf8 ? '#\{(!)?([\p{L}\d_]+)\}#u' : '#\{(!)?(\w+)\}#';


there is no need to check for unicode support: \pL always works
this means that the regexp can unconditionally be: '#\{(!)?([\w\pL]++)\}#'

note that there are other occurrences of \w in this very file

Yes, I agree. Anyway, I found that I also need to make some fixes to support utf-8 characters in regex of compiled routes. (for php7.2)

@nicolas-grekas Maybe it's not so bad to split the logic.

Matching characters by Unicode property is not fast, because PCRE has to do a multistage table lookup in order to find a character's property. That is why the traditional escape sequences such as \d and \w do not use Unicode properties in PCRE by default

In non-unicode mode, PCRE doesn't use unicode tables.

@nicolas-grekas in non-unicode mode \pL does not fully handle unicode characters too. https://www.phpliveregex.com/p/BbM

When the u modifier is set (aka when $needsUtf8 is true, aka when the utf8 option is set), PCRE will use Unicode tables. It will use ASCII tables otherwise.

So we still need to conditionally set u modifier, right?

That's already done:

symfony/src/Symfony/Component/Routing/RouteCompiler.php

Line 236 in 4dd13ab

$regexp .= 'u';

But it doesn't work. We have to use the modifier u in this regex as well. Because otherwise route params won't be parsed and route will be determined as static instead of dynamic

Oh, that's the point. But it doesn't work in my test cases and I am still sure that we need to add u on currently discussed string.
Anyway I will wait for full PR then. Thanks for the time!

nicolas-grekas

See grep '\\w' src/Symfony/Component/Routing/ -r

nicolas-grekas · 2022-01-18T10:11:51Z

Closing in favor of #45054
Could you please have a look @mazanax?
Thanks for pushing this forward!

carsonbot added Status: Needs Review Bug Routing labels Jul 16, 2021

carsonbot added this to the 5.3 milestone Jul 16, 2021

carsonbot changed the title ~~[Router] Fix matching of utf8 params~~ [Routing] Fix matching of utf8 params Jul 16, 2021

Foxprodev reviewed Jul 16, 2021

View reviewed changes

OskarStark reviewed Jul 16, 2021

View reviewed changes

nicolas-grekas reviewed Jul 19, 2021

View reviewed changes

nicolas-grekas requested changes Jul 19, 2021

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jul 19, 2021

mazanax added 2 commits July 19, 2021 13:50

[Routing] Fix matching of utf8 params

9f3da58

[Routing] Change route params regex pattern

89cc6e8

nicolas-grekas closed this Jan 18, 2022

Uh oh!

[Routing] Fix matching of utf8 params #42159

[Routing] Fix matching of utf8 params #42159

Uh oh!

Conversation

mazanax commented Jul 16, 2021

Uh oh!

carsonbot commented Jul 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Foxprodev Jul 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazanax Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazanax Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Foxprodev Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazanax Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Foxprodev Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Jan 18, 2022

Uh oh!

Uh oh!

Foxprodev Jul 16, 2021 •

edited

Loading

mazanax Jul 19, 2021 •

edited

Loading

nicolas-grekas Jul 19, 2021 •

edited

Loading

mazanax Jul 19, 2021 •

edited

Loading

Foxprodev Jul 19, 2021 •

edited

Loading

mazanax Jul 19, 2021 •

edited

Loading

Foxprodev Jul 19, 2021 •

edited

Loading