bug #26538 [Routing] remove capturing groups from requirements, they break the merged regex (nicolas-grekas)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[Routing] remove capturing groups from requirements, they break the merged regex

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Group positions are now used to extract variables. Capturing groups in requirements break them for now.

Commits
-------

8444022 [Routing] remove capturing groups from requirements, they break the merged regex

Author: nicolas-grekas

RouteCompiler.php

@@ -180,6 +180,7 @@ private static function compilePattern(Route $route, $pattern, $isHost)
                 if (!$useUtf8 && $needsUtf8) {
                     throw new \LogicException(sprintf('Cannot mix UTF-8 requirement with non-UTF-8 charset for variable "%s" in pattern "%s".', $varName, $pattern));
                 }
+                $regexp = self::transformCapturingGroupsToNonCapturings($regexp);
             }
 
             $tokens[] = array('variable', $isSeparator ? $precedingChar : '', $regexp, $varName);
@@ -247,7 +248,7 @@ private static function determineStaticPrefix(Route $route, array $tokens): stri
     }
 
     /**
-     * Returns the next static character in the Route pattern that will serve as a separator (or the empty string when none available)
+     * Returns the next static character in the Route pattern that will serve as a separator (or the empty string when none available).
      */
     private static function findNextSeparator(string $pattern, bool $useUtf8): string
     {
@@ -304,4 +305,25 @@ private static function computeRegexp(array $tokens, int $index, int $firstOptio
             }
         }
     }
+
+    private static function transformCapturingGroupsToNonCapturings(string $regexp): string
+    {
+        for ($i = 0; $i < \strlen($regexp); ++$i) {
+            if ('\\' === $regexp[$i]) {
+                ++$i;
+                continue;
+            }
+            if ('(' !== $regexp[$i] || !isset($regexp[$i + 2])) {
+                continue;
+            }
+            if ('*' === $regexp[++$i] || '?' === $regexp[$i]) {
+                ++$i;
+                continue;
+            }
+            $regexp = substr_replace($regexp, '?:', $i, 0);
+            $i += 2;
+        }
+
+        return $regexp;
+    }
 }

Tests/Matcher/UrlMatcherTest.php

@@ -587,6 +587,15 @@ public function testUnicodeRoute()
         $this->assertEquals(array('_route' => 'b', 'a' => 'é'), $matcher->match('/é'));
     }
 
+    public function testRequirementWithCapturingGroup()
+    {
+        $coll = new RouteCollection();
+        $coll->add('a', new Route('/{a}/{b}', array(), array('a' => '(a|b)')));
+
+        $matcher = $this->getUrlMatcher($coll);
+        $this->assertEquals(array('_route' => 'a', 'a' => 'a', 'b' => 'b'), $matcher->match('/a/b'));
+    }
+
     protected function getUrlMatcher(RouteCollection $routes, RequestContext $context = null)
     {
         return new UrlMatcher($routes, $context ?: new RequestContext());

Tests/RouteCompilerTest.php

@@ -110,8 +110,8 @@ public function provideCompileData()
             array(
                 'Route with an optional variable as the first segment with requirements',
                 array('/{bar}', array('bar' => 'bar'), array('bar' => '(foo|bar)')),
-                '', '#^/(?P<bar>(foo|bar))?$#sD', array('bar'), array(
-                    array('variable', '/', '(foo|bar)', 'bar'),
+                '', '#^/(?P<bar>(?:foo|bar))?$#sD', array('bar'), array(
+                    array('variable', '/', '(?:foo|bar)', 'bar'),
                 ),
             ),
 
@@ -146,10 +146,10 @@ public function provideCompileData()
             array(
                 'Route without separator between variables',
                 array('/{w}{x}{y}{z}.{_format}', array('z' => 'default-z', '_format' => 'html'), array('y' => '(y|Y)')),
-                '', '#^/(?P<w>[^/\.]+)(?P<x>[^/\.]+)(?P<y>(y|Y))(?:(?P<z>[^/\.]++)(?:\.(?P<_format>[^/]++))?)?$#sD', array('w', 'x', 'y', 'z', '_format'), array(
+                '', '#^/(?P<w>[^/\.]+)(?P<x>[^/\.]+)(?P<y>(?:y|Y))(?:(?P<z>[^/\.]++)(?:\.(?P<_format>[^/]++))?)?$#sD', array('w', 'x', 'y', 'z', '_format'), array(
                     array('variable', '.', '[^/]++', '_format'),
                     array('variable', '', '[^/\.]++', 'z'),
-                    array('variable', '', '(y|Y)', 'y'),
+                    array('variable', '', '(?:y|Y)', 'y'),
                     array('variable', '', '[^/\.]+', 'x'),
                     array('variable', '/', '[^/\.]+', 'w'),
                 ),
@@ -380,6 +380,25 @@ public function testRouteWithTooLongVariableName()
         $route = new Route(sprintf('/{%s}', str_repeat('a', RouteCompiler::VARIABLE_MAXIMUM_LENGTH + 1)));
         $route->compile();
     }
+
+    /**
+     * @dataProvider provideRemoveCapturingGroup
+     */
+    public function testRemoveCapturingGroup($regex, $requirement)
+    {
+        $route = new Route('/{foo}', array(), array('foo' => $requirement));
+
+        $this->assertSame($regex, $route->compile()->getRegex());
+    }
+
+    public function provideRemoveCapturingGroup()
+    {
+        yield array('#^/(?P<foo>a(?:b|c)(?:d|e)f)$#sD', 'a(b|c)(d|e)f');
+        yield array('#^/(?P<foo>a\(b\)c)$#sD', 'a\(b\)c');
+        yield array('#^/(?P<foo>(?:b))$#sD', '(?:b)');
+        yield array('#^/(?P<foo>(?(b)b))$#sD', '(?(b)b)');
+        yield array('#^/(?P<foo>(*F))$#sD', '(*F)');
+    }
 }
 
 class Utf8RouteCompiler extends RouteCompiler