Skip to content

Commit 05071a4

Browse files
nicolas-grekasnicolas-grekas
committed
bug #30013 [Routing] dont redirect routes with greedy trailing vars with no explicit slash (nicolas-grekas)
This PR was merged into the 4.1 branch. Discussion ---------- [Routing] dont redirect routes with greedy trailing vars with no explicit slash | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #29673 #29734 #29575 | License | MIT | Doc PR | - From the linked issue: > The current logic is the following: > - when a route is declared with a trailing slash, the trimmed-slash url is redirected to the one with the slash > - when a route is declared with *no* trailing slash, the slashed url is redirected to the trimmed-slash one > > That includes routes with slash-greedy requirements: when the same greedy requirement matches both the slashed and the trimmed-slash URLs, only one of them is considered the canonical one and a redirection happens. > > We could fine tune this logic and make an exception when a trailing slash-greedy requirement is declared with no explicit trailing slash after it. (ie disable any redirections for `/foo/{.*}` but keep it for `/foo/{.*}/`. That would mean `/foo/bar` and `/foo/bar/` wouldn't trigger the redirection for route `/foo/{.*}`, breaking the "not-semantics" property of trailing slashes for catch-all routes. Which might be legit afterall. This PR implements this fine tuning, as that's the most BC behavior (and thus the correct one). See #30012 for `testGreedyTrailingRequirement` in action on 3.4 as a proof. Commits ------- 2bb8890 [Routing] dont redirect routes with greedy trailing vars with no explicit slash
2 parents 78c23c7 + 2bb8890 commit 05071a4

13 files changed

+80
-213
lines changed

‎src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php

+8-36
Original file line numberDiff line numberDiff line change
@@ -558,34 +558,27 @@ private function compileSwitchDefault(bool $hasVars, bool $matchHost): string
558558
$code = '';
559559
}
560560

561-
$code .= $hasVars ? '
562-
if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
563-
break;
564-
}' : '
565-
break;';
566-
567561
$code = sprintf(<<<'EOF'
568-
if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {%s
562+
if ('/' !== $pathinfo && %s$hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {%s
563+
break;
569564
}
570565

571566
EOF
572567
,
568+
$hasVars ? '!$hasTrailingVar && ' : '',
573569
$code
574570
);
575571

576572
if ($hasVars) {
577573
$code = <<<'EOF'
578574
579-
if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
580-
// no-op
581-
} elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
582-
$matches = $n;
583-
} else {
584-
$hasTrailingSlash = true;
585-
}
575+
$hasTrailingVar = $trimmedPathinfo !== $pathinfo && $hasTrailingVar;
586576

587577
EOF
588578
.$code.<<<'EOF'
579+
if ($hasTrailingSlash && $hasTrailingVar && preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
580+
$matches = $n;
581+
}
589582
590583
foreach ($vars as $i => $v) {
591584
if (isset($matches[1 + $i])) {
@@ -665,31 +658,10 @@ private function compileRoute(Route $route, string $name, bool $checkHost, bool
665658
if ('/' !== $pathinfo && preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
666659
$matches = $n;
667660
}
668-
EOF;
669-
} elseif ($this->supportsRedirections && (!$methods || isset($methods['GET']))) {
670-
$code .= <<<'EOF'
671-
$hasTrailingSlash = false;
672-
if ($trimmedPathinfo === $pathinfo) {
673-
// no-op
674-
} elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
675-
$matches = $n;
676-
} else {
677-
$hasTrailingSlash = true;
678-
}
679-
680-
if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {%s
681-
if ($trimmedPathinfo === $pathinfo) {
682-
goto %s;
683-
}
684-
}
685661
EOF;
686662
} else {
687663
$code .= <<<'EOF'
688-
if ($trimmedPathinfo === $pathinfo) {
689-
// no-op
690-
} elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
691-
$matches = $n;
692-
} elseif ('/' !== $pathinfo) {
664+
if ($trimmedPathinfo !== $pathinfo) {
693665
goto %2$s;
694666
}
695667
EOF;

‎src/Symfony/Component/Routing/Matcher/UrlMatcher.php

+8-11
Original file line numberDiff line numberDiff line change
@@ -156,21 +156,18 @@ protected function matchCollection($pathinfo, RouteCollection $routes)
156156
continue;
157157
}
158158

159-
if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar = preg_match('#\{\w+\}/?$#', $route->getPath())) {
160-
// no-op
161-
} elseif (preg_match($regex, $trimmedPathinfo, $m)) {
162-
$matches = $m;
163-
} else {
164-
$hasTrailingSlash = true;
165-
}
159+
$hasTrailingVar = $trimmedPathinfo !== $pathinfo && preg_match('#\{\w+\}/?$#', $route->getPath());
166160

167-
if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
161+
if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
168162
if ($supportsTrailingSlash && (!$requiredMethods || \in_array('GET', $requiredMethods))) {
169163
return $this->allow = $this->allowSchemes = [];
170164
}
171-
if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
172-
continue;
173-
}
165+
166+
continue;
167+
}
168+
169+
if ($hasTrailingSlash && $hasTrailingVar && preg_match($regex, $trimmedPathinfo, $m)) {
170+
$matches = $m;
174171
}
175172

176173
$hostMatches = [];

‎src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php

+8-25
Original file line numberDiff line numberDiff line change
@@ -187,11 +187,7 @@ public function match($pathinfo)
187187
break;
188188
case 160:
189189
// foo1
190-
if ($trimmedPathinfo === $pathinfo) {
191-
// no-op
192-
} elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
193-
$matches = $n;
194-
} elseif ('/' !== $pathinfo) {
190+
if ($trimmedPathinfo !== $pathinfo) {
195191
goto not_foo1;
196192
}
197193

@@ -209,11 +205,7 @@ public function match($pathinfo)
209205
break;
210206
case 204:
211207
// foo2
212-
if ($trimmedPathinfo === $pathinfo) {
213-
// no-op
214-
} elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
215-
$matches = $n;
216-
} elseif ('/' !== $pathinfo) {
208+
if ($trimmedPathinfo !== $pathinfo) {
217209
goto not_foo2;
218210
}
219211

@@ -225,11 +217,7 @@ public function match($pathinfo)
225217
break;
226218
case 279:
227219
// foo3
228-
if ($trimmedPathinfo === $pathinfo) {
229-
// no-op
230-
} elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
231-
$matches = $n;
232-
} elseif ('/' !== $pathinfo) {
220+
if ($trimmedPathinfo !== $pathinfo) {
233221
goto not_foo3;
234222
}
235223

@@ -262,17 +250,12 @@ public function match($pathinfo)
262250

263251
list($ret, $vars, $requiredMethods, $requiredSchemes, $hasTrailingSlash, $hasTrailingVar) = $routes[$m];
264252

265-
if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
266-
// no-op
267-
} elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
268-
$matches = $n;
269-
} else {
270-
$hasTrailingSlash = true;
253+
$hasTrailingVar = $trimmedPathinfo !== $pathinfo && $hasTrailingVar;
254+
if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
255+
break;
271256
}
272-
if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
273-
if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
274-
break;
275-
}
257+
if ($hasTrailingSlash && $hasTrailingVar && preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
258+
$matches = $n;
276259
}
277260

278261
foreach ($vars as $i => $v) {

‎src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher10.php

+5-10
Original file line numberDiff line numberDiff line change
@@ -2794,17 +2794,12 @@ public function match($pathinfo)
27942794

27952795
list($ret, $vars, $requiredMethods, $requiredSchemes, $hasTrailingSlash, $hasTrailingVar) = $routes[$m];
27962796

2797-
if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
2798-
// no-op
2799-
} elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
2800-
$matches = $n;
2801-
} else {
2802-
$hasTrailingSlash = true;
2797+
$hasTrailingVar = $trimmedPathinfo !== $pathinfo && $hasTrailingVar;
2798+
if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
2799+
break;
28032800
}
2804-
if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
2805-
if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
2806-
break;
2807-
}
2801+
if ($hasTrailingSlash && $hasTrailingVar && preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
2802+
$matches = $n;
28082803
}
28092804

28102805
foreach ($vars as $i => $v) {

‎src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher11.php

+6-11
Original file line numberDiff line numberDiff line change
@@ -119,20 +119,15 @@ private function doMatch(string $pathinfo, array &$allow = [], array &$allowSche
119119

120120
list($ret, $vars, $requiredMethods, $requiredSchemes, $hasTrailingSlash, $hasTrailingVar) = $routes[$m];
121121

122-
if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
123-
// no-op
124-
} elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
125-
$matches = $n;
126-
} else {
127-
$hasTrailingSlash = true;
128-
}
129-
if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
122+
$hasTrailingVar = $trimmedPathinfo !== $pathinfo && $hasTrailingVar;
123+
if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
130124
if ('GET' === $canonicalMethod && (!$requiredMethods || isset($requiredMethods['GET']))) {
131125
return $allow = $allowSchemes = [];
132126
}
133-
if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
134-
break;
135-
}
127+
break;
128+
}
129+
if ($hasTrailingSlash && $hasTrailingVar && preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
130+
$matches = $n;
136131
}
137132

138133
foreach ($vars as $i => $v) {

‎src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher12.php

+5-10
Original file line numberDiff line numberDiff line change
@@ -64,17 +64,12 @@ public function match($pathinfo)
6464

6565
list($ret, $vars, $requiredMethods, $requiredSchemes, $hasTrailingSlash, $hasTrailingVar) = $routes[$m];
6666

67-
if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
68-
// no-op
69-
} elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
70-
$matches = $n;
71-
} else {
72-
$hasTrailingSlash = true;
67+
$hasTrailingVar = $trimmedPathinfo !== $pathinfo && $hasTrailingVar;
68+
if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
69+
break;
7370
}
74-
if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
75-
if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
76-
break;
77-
}
71+
if ($hasTrailingSlash && $hasTrailingVar && preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
72+
$matches = $n;
7873
}
7974

8075
foreach ($vars as $i => $v) {

‎src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher13.php

+2-10
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,7 @@ public function match($pathinfo)
4444
switch ($m = (int) $matches['MARK']) {
4545
case 56:
4646
// r1
47-
if ($trimmedPathinfo === $pathinfo) {
48-
// no-op
49-
} elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
50-
$matches = $n;
51-
} elseif ('/' !== $pathinfo) {
47+
if ($trimmedPathinfo !== $pathinfo) {
5248
goto not_r1;
5349
}
5450

@@ -58,11 +54,7 @@ public function match($pathinfo)
5854
not_r1:
5955

6056
// r2
61-
if ($trimmedPathinfo === $pathinfo) {
62-
// no-op
63-
} elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
64-
$matches = $n;
65-
} elseif ('/' !== $pathinfo) {
57+
if ($trimmedPathinfo !== $pathinfo) {
6658
goto not_r2;
6759
}
6860

‎src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php

+11-48
Original file line numberDiff line numberDiff line change
@@ -230,11 +230,7 @@ private function doMatch(string $pathinfo, array &$allow = [], array &$allowSche
230230
break;
231231
case 160:
232232
// foo1
233-
if ($trimmedPathinfo === $pathinfo) {
234-
// no-op
235-
} elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
236-
$matches = $n;
237-
} elseif ('/' !== $pathinfo) {
233+
if ($trimmedPathinfo !== $pathinfo) {
238234
goto not_foo1;
239235
}
240236

@@ -252,22 +248,8 @@ private function doMatch(string $pathinfo, array &$allow = [], array &$allowSche
252248
break;
253249
case 204:
254250
// foo2
255-
$hasTrailingSlash = false;
256-
if ($trimmedPathinfo === $pathinfo) {
257-
// no-op
258-
} elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
259-
$matches = $n;
260-
} else {
261-
$hasTrailingSlash = true;
262-
}
263-
264-
if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
265-
if ('GET' === $canonicalMethod) {
266-
return $allow = $allowSchemes = [];
267-
}
268-
if ($trimmedPathinfo === $pathinfo) {
269-
goto not_foo2;
270-
}
251+
if ($trimmedPathinfo !== $pathinfo) {
252+
goto not_foo2;
271253
}
272254

273255
$matches = ['foo1' => $matches[1] ?? null];
@@ -278,22 +260,8 @@ private function doMatch(string $pathinfo, array &$allow = [], array &$allowSche
278260
break;
279261
case 279:
280262
// foo3
281-
$hasTrailingSlash = false;
282-
if ($trimmedPathinfo === $pathinfo) {
283-
// no-op
284-
} elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
285-
$matches = $n;
286-
} else {
287-
$hasTrailingSlash = true;
288-
}
289-
290-
if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
291-
if ('GET' === $canonicalMethod) {
292-
return $allow = $allowSchemes = [];
293-
}
294-
if ($trimmedPathinfo === $pathinfo) {
295-
goto not_foo3;
296-
}
263+
if ($trimmedPathinfo !== $pathinfo) {
264+
goto not_foo3;
297265
}
298266

299267
$matches = ['_locale' => $matches[1] ?? null, 'foo' => $matches[2] ?? null];
@@ -325,20 +293,15 @@ private function doMatch(string $pathinfo, array &$allow = [], array &$allowSche
325293

326294
list($ret, $vars, $requiredMethods, $requiredSchemes, $hasTrailingSlash, $hasTrailingVar) = $routes[$m];
327295

328-
if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
329-
// no-op
330-
} elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
331-
$matches = $n;
332-
} else {
333-
$hasTrailingSlash = true;
334-
}
335-
if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
296+
$hasTrailingVar = $trimmedPathinfo !== $pathinfo && $hasTrailingVar;
297+
if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
336298
if ('GET' === $canonicalMethod && (!$requiredMethods || isset($requiredMethods['GET']))) {
337299
return $allow = $allowSchemes = [];
338300
}
339-
if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
340-
break;
341-
}
301+
break;
302+
}
303+
if ($hasTrailingSlash && $hasTrailingVar && preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
304+
$matches = $n;
342305
}
343306

344307
foreach ($vars as $i => $v) {

‎src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php

+5-10
Original file line numberDiff line numberDiff line change
@@ -84,17 +84,12 @@ public function match($pathinfo)
8484

8585
list($ret, $vars, $requiredMethods, $requiredSchemes, $hasTrailingSlash, $hasTrailingVar) = $routes[$m];
8686

87-
if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
88-
// no-op
89-
} elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
90-
$matches = $n;
91-
} else {
92-
$hasTrailingSlash = true;
87+
$hasTrailingVar = $trimmedPathinfo !== $pathinfo && $hasTrailingVar;
88+
if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
89+
break;
9390
}
94-
if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
95-
if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
96-
break;
97-
}
91+
if ($hasTrailingSlash && $hasTrailingVar && preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
92+
$matches = $n;
9893
}
9994

10095
foreach ($vars as $i => $v) {

0 commit comments

Comments
 (0)