bug #29770 [Routing] Make important parameters required when matching…

… (vudaltsov) This PR was merged into the 4.3-dev branch. Discussion ---------- [Routing] Make important parameters required when matching | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #29763 | License | MIT | Doc PR | n/a 1. This PR improves "important" route parameters implementation. Instead of considering `!slug` to be a variable name (which is not correct from my POV and leads to a lot of `'!' === $varName[0]` and `substr($varName, 1)` snippets), I took advantage of the `$token` array and used offset `[5]` for keeping boolean importance flag. This approach improved and simplified code. 1. This PR makes important parameters required when matching according to #29763 Commits ------- 2c3ab22 Made important parameters required when matching
symfony · Jan 25, 2019 · a25daa8 · a25daa8
2 parents 9876e3a + 2c3ab22
commit a25daa8
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 14 deletions.
diff --git a/src/Symfony/Component/Routing/Generator/UrlGenerator.php b/src/Symfony/Component/Routing/Generator/UrlGenerator.php
@@ -157,9 +157,8 @@ protected function doGenerate($variables, $defaults, $requirements, $tokens, $pa
         foreach ($tokens as $token) {
             if ('variable' === $token[0]) {
                 $varName = $token[3];
-                if ($important = ('!' === $varName[0])) {
-                    $varName = substr($varName, 1);
-                }
+                // variable is not important by default
+                $important = $token[5] ?? false;
 
                 if (!$optional || $important || !array_key_exists($varName, $defaults) || (null !== $mergedParams[$varName] && (string) $mergedParams[$varName] !== (string) $defaults[$varName])) {
                     // check requirement

diff --git a/src/Symfony/Component/Routing/RouteCompiler.php b/src/Symfony/Component/Routing/RouteCompiler.php
@@ -111,9 +111,10 @@ private static function compilePattern(Route $route, $pattern, $isHost)
 
         // 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);
+        preg_match_all('#\{(!)?(\w+)\}#', $pattern, $matches, PREG_OFFSET_CAPTURE | PREG_SET_ORDER);
         foreach ($matches as $match) {
-            $varName = substr($match[0][0], 1, -1);
+            $important = $match[1][1] >= 0;
+            $varName = $match[2][0];
             // get all static text preceding the current variable
             $precedingText = substr($pattern, $pos, $match[0][1] - $pos);
             $pos = $match[0][1] + \strlen($match[0][0]);
@@ -183,10 +184,13 @@ private static function compilePattern(Route $route, $pattern, $isHost)
                 $regexp = self::transformCapturingGroupsToNonCapturings($regexp);
             }
 
-            $tokens[] = ['variable', $isSeparator ? $precedingChar : '', $regexp, $varName];
-            if ('!' === $varName[0]) {
-                $varName = substr($varName, 1);
+            if ($important) {
+                $token = ['variable', $isSeparator ? $precedingChar : '', $regexp, $varName, false, true];
+            } else {
+                $token = ['variable', $isSeparator ? $precedingChar : '', $regexp, $varName];
             }
+
+            $tokens[] = $token;
             $variables[] = $varName;
         }
 
@@ -199,7 +203,8 @@ private static function compilePattern(Route $route, $pattern, $isHost)
         if (!$isHost) {
             for ($i = \count($tokens) - 1; $i >= 0; --$i) {
                 $token = $tokens[$i];
-                if ('variable' === $token[0] && $route->hasDefault($token[3])) {
+                // variable is optional when it is not important and has a default value
+                if ('variable' === $token[0] && !($token[5] ?? false) && $route->hasDefault($token[3])) {
                     $firstOptional = $i;
                 } else {
                     break;
@@ -219,7 +224,7 @@ private static function compilePattern(Route $route, $pattern, $isHost)
             $regexp .= 'u';
             for ($i = 0, $nbToken = \count($tokens); $i < $nbToken; ++$i) {
                 if ('variable' === $tokens[$i][0]) {
-                    $tokens[$i][] = true;
+                    $tokens[$i][4] = true;
                 }
             }
         }
@@ -286,10 +291,6 @@ private static function computeRegexp(array $tokens, int $index, int $firstOptio
             // Text tokens
             return preg_quote($token[1], self::REGEX_DELIMITER);
         } else {
-            if ('variable' === $token[0] && '!' === $token[3][0]) {
-                $token[3] = substr($token[3], 1);
-            }
-
             // Variable tokens
             if (0 === $index && 0 === $firstOptional) {
                 // When the only token is an optional variable token, the separator is required

diff --git a/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php b/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php
@@ -186,6 +186,17 @@ public function testMatchImportantVariable()
         $this->assertEquals(['_route' => 'index', '_format' => 'xml'], $matcher->match('/index.xml'));
     }
 
+    /**
+     * @expectedException \Symfony\Component\Routing\Exception\ResourceNotFoundException
+     */
+    public function testShortPathDoesNotMatchImportantVariable()
+    {
+        $collection = new RouteCollection();
+        $collection->add('index', new Route('/index.{!_format}', ['_format' => 'xml']));
+
+        $this->getUrlMatcher($collection)->match('/index');
+    }
+
     /**
      * @expectedException \Symfony\Component\Routing\Exception\ResourceNotFoundException
      */