Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit a25daa8

Browse filesBrowse files
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
2 parents 9876e3a + 2c3ab22 commit a25daa8
Copy full SHA for a25daa8

File tree

3 files changed

+25
-14
lines changed
Filter options

3 files changed

+25
-14
lines changed

‎src/Symfony/Component/Routing/Generator/UrlGenerator.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/Routing/Generator/UrlGenerator.php
+2-3Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -157,9 +157,8 @@ protected function doGenerate($variables, $defaults, $requirements, $tokens, $pa
157157
foreach ($tokens as $token) {
158158
if ('variable' === $token[0]) {
159159
$varName = $token[3];
160-
if ($important = ('!' === $varName[0])) {
161-
$varName = substr($varName, 1);
162-
}
160+
// variable is not important by default
161+
$important = $token[5] ?? false;
163162

164163
if (!$optional || $important || !array_key_exists($varName, $defaults) || (null !== $mergedParams[$varName] && (string) $mergedParams[$varName] !== (string) $defaults[$varName])) {
165164
// check requirement

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/Routing/RouteCompiler.php
+12-11Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -111,9 +111,10 @@ private static function compilePattern(Route $route, $pattern, $isHost)
111111

112112
// Match all variables enclosed in "{}" and iterate over them. But we only want to match the innermost variable
113113
// in case of nested "{}", e.g. {foo{bar}}. This in ensured because \w does not match "{" or "}" itself.
114-
preg_match_all('#\{!?\w+\}#', $pattern, $matches, PREG_OFFSET_CAPTURE | PREG_SET_ORDER);
114+
preg_match_all('#\{(!)?(\w+)\}#', $pattern, $matches, PREG_OFFSET_CAPTURE | PREG_SET_ORDER);
115115
foreach ($matches as $match) {
116-
$varName = substr($match[0][0], 1, -1);
116+
$important = $match[1][1] >= 0;
117+
$varName = $match[2][0];
117118
// get all static text preceding the current variable
118119
$precedingText = substr($pattern, $pos, $match[0][1] - $pos);
119120
$pos = $match[0][1] + \strlen($match[0][0]);
@@ -183,10 +184,13 @@ private static function compilePattern(Route $route, $pattern, $isHost)
183184
$regexp = self::transformCapturingGroupsToNonCapturings($regexp);
184185
}
185186

186-
$tokens[] = ['variable', $isSeparator ? $precedingChar : '', $regexp, $varName];
187-
if ('!' === $varName[0]) {
188-
$varName = substr($varName, 1);
187+
if ($important) {
188+
$token = ['variable', $isSeparator ? $precedingChar : '', $regexp, $varName, false, true];
189+
} else {
190+
$token = ['variable', $isSeparator ? $precedingChar : '', $regexp, $varName];
189191
}
192+
193+
$tokens[] = $token;
190194
$variables[] = $varName;
191195
}
192196

@@ -199,7 +203,8 @@ private static function compilePattern(Route $route, $pattern, $isHost)
199203
if (!$isHost) {
200204
for ($i = \count($tokens) - 1; $i >= 0; --$i) {
201205
$token = $tokens[$i];
202-
if ('variable' === $token[0] && $route->hasDefault($token[3])) {
206+
// variable is optional when it is not important and has a default value
207+
if ('variable' === $token[0] && !($token[5] ?? false) && $route->hasDefault($token[3])) {
203208
$firstOptional = $i;
204209
} else {
205210
break;
@@ -219,7 +224,7 @@ private static function compilePattern(Route $route, $pattern, $isHost)
219224
$regexp .= 'u';
220225
for ($i = 0, $nbToken = \count($tokens); $i < $nbToken; ++$i) {
221226
if ('variable' === $tokens[$i][0]) {
222-
$tokens[$i][] = true;
227+
$tokens[$i][4] = true;
223228
}
224229
}
225230
}
@@ -286,10 +291,6 @@ private static function computeRegexp(array $tokens, int $index, int $firstOptio
286291
// Text tokens
287292
return preg_quote($token[1], self::REGEX_DELIMITER);
288293
} else {
289-
if ('variable' === $token[0] && '!' === $token[3][0]) {
290-
$token[3] = substr($token[3], 1);
291-
}
292-
293294
// Variable tokens
294295
if (0 === $index && 0 === $firstOptional) {
295296
// When the only token is an optional variable token, the separator is required

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php
+11Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,17 @@ public function testMatchImportantVariable()
186186
$this->assertEquals(['_route' => 'index', '_format' => 'xml'], $matcher->match('/index.xml'));
187187
}
188188

189+
/**
190+
* @expectedException \Symfony\Component\Routing\Exception\ResourceNotFoundException
191+
*/
192+
public function testShortPathDoesNotMatchImportantVariable()
193+
{
194+
$collection = new RouteCollection();
195+
$collection->add('index', new Route('/index.{!_format}', ['_format' => 'xml']));
196+
197+
$this->getUrlMatcher($collection)->match('/index');
198+
}
199+
189200
/**
190201
* @expectedException \Symfony\Component\Routing\Exception\ResourceNotFoundException
191202
*/

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.