Skip to content

Commit a1b2414

Browse files
nicolas-grekasnicolas-grekas
committed
feature #26304 [Routing] support scheme requirement without redirectable dumped matcher (Tobion)
This PR was merged into the 4.1-dev branch. Discussion ---------- [Routing] support scheme requirement without redirectable dumped matcher | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no <!-- don't forget to update UPGRADE-*.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | | License | MIT | Doc PR | The scheme handling was just redirecting immediately without testing the other routes at all for potential matches. This can cause Problems when you try to have different routes/controllers for the same path but different schemes. See added test. ``` $coll = new RouteCollection(); $coll->add('https_route', new Route('/', array(), array(), array(), '', array('https'))); $coll->add('http_route', new Route('/', array(), array(), array(), '', array('http'))); $matcher = $this->getUrlMatcher($coll); $this->assertEquals(array('_route' => 'http_route'), $matcher->match('/')); ``` Instead of matching the right route, it would redirect immediatly as soon as it hits the first route. This does not make sense and is not consistent with the other logic. Redirection should only happen when nothing matches. While fixing this I could also remove the limitation > throw new \LogicException('The "schemes" requirement is only supported for URL matchers that implement RedirectableUrlMatcherInterface.'); If redirection is not possible, the wrong scheme will just not match the route. This is the same as already implemented by UrlMatcher (not RedirecableUrlMatcher). If redirection is supported, it will redirect to the first supported scheme if no other route matches. This makes the implementation similar to redirection for trailing slash and handling not allowed methods. Also previously, the scheme redirection was done for non-safe verbs which shouldn't happen as well, ref. #25962 Commits ------- f9b54c5 [Routing] support scheme requirement without redirectable dumped matcher
2 parents 308e12c + f9b54c5 commit a1b2414

21 files changed

+381
-202
lines changed

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

+37-46
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ class PhpMatcherDumper extends MatcherDumper
2929
{
3030
private $expressionLanguage;
3131
private $signalingException;
32-
private $supportsRedirections;
3332

3433
/**
3534
* @var ExpressionFunctionProviderInterface[]
@@ -57,7 +56,7 @@ public function dump(array $options = array())
5756

5857
// trailing slash support is only enabled if we know how to redirect the user
5958
$interfaces = class_implements($options['base_class']);
60-
$this->supportsRedirections = isset($interfaces[RedirectableUrlMatcherInterface::class]);
59+
$supportsRedirections = isset($interfaces[RedirectableUrlMatcherInterface::class]);
6160

6261
return <<<EOF
6362
<?php
@@ -77,7 +76,7 @@ public function __construct(RequestContext \$context)
7776
\$this->context = \$context;
7877
}
7978
80-
{$this->generateMatchMethod()}
79+
{$this->generateMatchMethod($supportsRedirections)}
8180
}
8281
8382
EOF;
@@ -91,7 +90,7 @@ public function addExpressionLanguageProvider(ExpressionFunctionProviderInterfac
9190
/**
9291
* Generates the code for the match method implementing UrlMatcherInterface.
9392
*/
94-
private function generateMatchMethod(): string
93+
private function generateMatchMethod(bool $supportsRedirections): string
9594
{
9695
// Group hosts by same-suffix, re-order when possible
9796
$matchHost = false;
@@ -111,7 +110,7 @@ private function generateMatchMethod(): string
111110

112111
$code = <<<EOF
113112
{
114-
\$allow = array();
113+
\$allow = \$allowSchemes = array();
115114
\$pathinfo = rawurldecode(\$rawPathinfo);
116115
\$context = \$this->context;
117116
\$requestMethod = \$canonicalMethod = \$context->getMethod();
@@ -124,25 +123,44 @@ private function generateMatchMethod(): string
124123
125124
EOF;
126125

127-
if ($this->supportsRedirections) {
126+
if ($supportsRedirections) {
128127
return <<<'EOF'
129128
public function match($pathinfo)
130129
{
131-
$allow = array();
132-
if ($ret = $this->doMatch($pathinfo, $allow)) {
130+
$allow = $allowSchemes = array();
131+
if ($ret = $this->doMatch($pathinfo, $allow, $allowSchemes)) {
133132
return $ret;
134133
}
135-
if ('/' !== $pathinfo && in_array($this->context->getMethod(), array('HEAD', 'GET'), true)) {
134+
if ($allow) {
135+
throw new MethodNotAllowedException(array_keys($allow));
136+
}
137+
if (!in_array($this->context->getMethod(), array('HEAD', 'GET'), true)) {
138+
// no-op
139+
} elseif ($allowSchemes) {
140+
redirect_scheme:
141+
$scheme = $this->context->getScheme();
142+
$this->context->setScheme(key($allowSchemes));
143+
try {
144+
if ($ret = $this->doMatch($pathinfo)) {
145+
return $this->redirect($pathinfo, $ret['_route'], $this->context->getScheme()) + $ret;
146+
}
147+
} finally {
148+
$this->context->setScheme($scheme);
149+
}
150+
} elseif ('/' !== $pathinfo) {
136151
$pathinfo = '/' !== $pathinfo[-1] ? $pathinfo.'/' : substr($pathinfo, 0, -1);
137-
if ($ret = $this->doMatch($pathinfo)) {
152+
if ($ret = $this->doMatch($pathinfo, $allow, $allowSchemes)) {
138153
return $this->redirect($pathinfo, $ret['_route']) + $ret;
139154
}
155+
if ($allowSchemes) {
156+
goto redirect_scheme;
157+
}
140158
}
141159
142-
throw $allow ? new MethodNotAllowedException(array_keys($allow)) : new ResourceNotFoundException();
160+
throw new ResourceNotFoundException();
143161
}
144162
145-
private function doMatch(string $rawPathinfo, array &$allow = array()): ?array
163+
private function doMatch(string $rawPathinfo, array &$allow = array(), array &$allowSchemes = array()): ?array
146164

147165
EOF
148166
.$code."\n return null;\n }";
@@ -238,9 +256,6 @@ private function compileStaticRoutes(array $staticRoutes, bool $matchHost): stri
238256
}
239257

240258
if (!$route->getCondition()) {
241-
if (!$this->supportsRedirections && $route->getSchemes()) {
242-
throw new \LogicException('The "schemes" requirement is only supported for URL matchers that implement RedirectableUrlMatcherInterface.');
243-
}
244259
$default .= sprintf(
245260
"%s => array(%s, %s, %s, %s),\n",
246261
self::export($url),
@@ -535,8 +550,8 @@ private function compileSwitchDefault(bool $hasVars, bool $matchHost): string
535550
} else {
536551
$code = '';
537552
}
538-
if ($this->supportsRedirections) {
539-
$code .= <<<EOF
553+
554+
$code .= <<<EOF
540555
541556
\$hasRequiredScheme = !\$requiredSchemes || isset(\$requiredSchemes[\$context->getScheme()]);
542557
if (\$requiredMethods && !isset(\$requiredMethods[\$canonicalMethod]) && !isset(\$requiredMethods[\$requestMethod])) {
@@ -546,28 +561,13 @@ private function compileSwitchDefault(bool $hasVars, bool $matchHost): string
546561
break;
547562
}
548563
if (!\$hasRequiredScheme) {
549-
if ('GET' !== \$canonicalMethod) {
550-
break;
551-
}
552-
553-
return \$this->redirect(\$rawPathinfo, \$ret['_route'], key(\$requiredSchemes)) + \$ret;
554-
}
555-
556-
return \$ret;
557-
558-
EOF;
559-
} else {
560-
$code .= <<<EOF
561-
562-
if (\$requiredMethods && !isset(\$requiredMethods[\$canonicalMethod]) && !isset(\$requiredMethods[\$requestMethod])) {
563-
\$allow += \$requiredMethods;
564+
\$allowSchemes += \$requiredSchemes;
564565
break;
565566
}
566567
567568
return \$ret;
568569
569570
EOF;
570-
}
571571

572572
return $code;
573573
}
@@ -647,9 +647,6 @@ private function compileRoute(Route $route, string $name, bool $checkHost): stri
647647
}
648648

649649
if ($schemes = $route->getSchemes()) {
650-
if (!$this->supportsRedirections) {
651-
throw new \LogicException('The "schemes" requirement is only supported for URL matchers that implement RedirectableUrlMatcherInterface.');
652-
}
653650
$schemes = self::export(array_flip($schemes));
654651
if ($methods) {
655652
$code .= <<<EOF
@@ -662,11 +659,8 @@ private function compileRoute(Route $route, string $name, bool $checkHost): stri
662659
goto $gotoname;
663660
}
664661
if (!\$hasRequiredScheme) {
665-
if ('GET' !== \$canonicalMethod) {
666-
goto $gotoname;
667-
}
668-
669-
return \$this->redirect(\$rawPathinfo, '$name', key(\$requiredSchemes)) + \$ret;
662+
\$allowSchemes += \$requiredSchemes;
663+
goto $gotoname;
670664
}
671665
672666
@@ -675,11 +669,8 @@ private function compileRoute(Route $route, string $name, bool $checkHost): stri
675669
$code .= <<<EOF
676670
\$requiredSchemes = $schemes;
677671
if (!isset(\$requiredSchemes[\$context->getScheme()])) {
678-
if ('GET' !== \$canonicalMethod) {
679-
goto $gotoname;
680-
}
681-
682-
return \$this->redirect(\$rawPathinfo, '$name', key(\$requiredSchemes)) + \$ret;
672+
\$allowSchemes += \$requiredSchemes;
673+
goto $gotoname;
683674
}
684675
685676

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

+27-27
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@
1111

1212
namespace Symfony\Component\Routing\Matcher;
1313

14+
use Symfony\Component\Routing\Exception\ExceptionInterface;
1415
use Symfony\Component\Routing\Exception\ResourceNotFoundException;
15-
use Symfony\Component\Routing\Route;
1616

1717
/**
1818
* @author Fabien Potencier <fabien@symfony.com>
@@ -27,38 +27,38 @@ public function match($pathinfo)
2727
try {
2828
return parent::match($pathinfo);
2929
} catch (ResourceNotFoundException $e) {
30-
if ('/' === $pathinfo || !\in_array($this->context->getMethod(), array('HEAD', 'GET'), true)) {
30+
if (!\in_array($this->context->getMethod(), array('HEAD', 'GET'), true)) {
3131
throw $e;
3232
}
3333

34-
try {
35-
$pathinfo = '/' !== $pathinfo[-1] ? $pathinfo.'/' : substr($pathinfo, 0, -1);
36-
$ret = parent::match($pathinfo);
34+
if ($this->allowSchemes) {
35+
redirect_scheme:
36+
$scheme = $this->context->getScheme();
37+
$this->context->setScheme(current($this->allowSchemes));
38+
try {
39+
$ret = parent::match($pathinfo);
3740

38-
return $this->redirect($pathinfo, $ret['_route'] ?? null) + $ret;
39-
} catch (ResourceNotFoundException $e2) {
41+
return $this->redirect($pathinfo, $ret['_route'] ?? null, $this->context->getScheme()) + $ret;
42+
} catch (ExceptionInterface $e2) {
43+
throw $e;
44+
} finally {
45+
$this->context->setScheme($scheme);
46+
}
47+
} elseif ('/' === $pathinfo) {
4048
throw $e;
41-
}
42-
}
43-
}
49+
} else {
50+
try {
51+
$pathinfo = '/' !== $pathinfo[-1] ? $pathinfo.'/' : substr($pathinfo, 0, -1);
52+
$ret = parent::match($pathinfo);
4453

45-
/**
46-
* {@inheritdoc}
47-
*/
48-
protected function handleRouteRequirements($pathinfo, $name, Route $route)
49-
{
50-
// expression condition
51-
if ($route->getCondition() && !$this->getExpressionLanguage()->evaluate($route->getCondition(), array('context' => $this->context, 'request' => $this->request ?: $this->createRequest($pathinfo)))) {
52-
return array(self::REQUIREMENT_MISMATCH, null);
53-
}
54-
55-
// check HTTP scheme requirement
56-
$scheme = $this->context->getScheme();
57-
$schemes = $route->getSchemes();
58-
if ($schemes && !$route->hasScheme($scheme)) {
59-
return array(self::ROUTE_MATCH, $this->redirect($pathinfo, $name, current($schemes)));
54+
return $this->redirect($pathinfo, $ret['_route'] ?? null) + $ret;
55+
} catch (ExceptionInterface $e2) {
56+
if ($this->allowSchemes) {
57+
goto redirect_scheme;
58+
}
59+
throw $e;
60+
}
61+
}
6062
}
61-
62-
return array(self::REQUIREMENT_MATCH, null);
6363
}
6464
}

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

+22-8
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,19 @@ class UrlMatcher implements UrlMatcherInterface, RequestMatcherInterface
3333
const ROUTE_MATCH = 2;
3434

3535
protected $context;
36+
37+
/**
38+
* Collects HTTP methods that would be allowed for the request.
39+
*/
3640
protected $allow = array();
41+
42+
/**
43+
* Collects URI schemes that would be allowed for the request.
44+
*
45+
* @internal
46+
*/
47+
protected $allowSchemes = array();
48+
3749
protected $routes;
3850
protected $request;
3951
protected $expressionLanguage;
@@ -70,7 +82,7 @@ public function getContext()
7082
*/
7183
public function match($pathinfo)
7284
{
73-
$this->allow = array();
85+
$this->allow = $this->allowSchemes = array();
7486

7587
if ($ret = $this->matchCollection(rawurldecode($pathinfo), $this->routes)) {
7688
return $ret;
@@ -141,22 +153,28 @@ protected function matchCollection($pathinfo, RouteCollection $routes)
141153
continue;
142154
}
143155

144-
// check HTTP method requirement
156+
$hasRequiredScheme = !$route->getSchemes() || $route->hasScheme($this->context->getScheme());
145157
if ($requiredMethods = $route->getMethods()) {
146158
// HEAD and GET are equivalent as per RFC
147159
if ('HEAD' === $method = $this->context->getMethod()) {
148160
$method = 'GET';
149161
}
150162

151163
if (!in_array($method, $requiredMethods)) {
152-
if (self::REQUIREMENT_MATCH === $status[0]) {
164+
if ($hasRequiredScheme) {
153165
$this->allow = array_merge($this->allow, $requiredMethods);
154166
}
155167

156168
continue;
157169
}
158170
}
159171

172+
if (!$hasRequiredScheme) {
173+
$this->allowSchemes = array_merge($this->allowSchemes, $route->getSchemes());
174+
175+
continue;
176+
}
177+
160178
return $this->getAttributes($route, $name, array_replace($matches, $hostMatches, isset($status[1]) ? $status[1] : array()));
161179
}
162180
}
@@ -197,11 +215,7 @@ protected function handleRouteRequirements($pathinfo, $name, Route $route)
197215
return array(self::REQUIREMENT_MISMATCH, null);
198216
}
199217

200-
// check HTTP scheme requirement
201-
$scheme = $this->context->getScheme();
202-
$status = $route->getSchemes() && !$route->hasScheme($scheme) ? self::REQUIREMENT_MISMATCH : self::REQUIREMENT_MATCH;
203-
204-
return array($status, null);
218+
return array(self::REQUIREMENT_MATCH, null);
205219
}
206220

207221
/**

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ public function __construct(RequestContext $context)
1717

1818
public function match($rawPathinfo)
1919
{
20-
$allow = array();
20+
$allow = $allowSchemes = array();
2121
$pathinfo = rawurldecode($rawPathinfo);
2222
$context = $this->context;
2323
$requestMethod = $canonicalMethod = $context->getMethod();

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

+17-3
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ public function __construct(RequestContext $context)
1717

1818
public function match($rawPathinfo)
1919
{
20-
$allow = array();
20+
$allow = $allowSchemes = array();
2121
$pathinfo = rawurldecode($rawPathinfo);
2222
$context = $this->context;
2323
$requestMethod = $canonicalMethod = $context->getMethod();
@@ -64,8 +64,15 @@ public function match($rawPathinfo)
6464
}
6565
}
6666

67+
$hasRequiredScheme = !$requiredSchemes || isset($requiredSchemes[$context->getScheme()]);
6768
if ($requiredMethods && !isset($requiredMethods[$canonicalMethod]) && !isset($requiredMethods[$requestMethod])) {
68-
$allow += $requiredMethods;
69+
if ($hasRequiredScheme) {
70+
$allow += $requiredMethods;
71+
}
72+
break;
73+
}
74+
if (!$hasRequiredScheme) {
75+
$allowSchemes += $requiredSchemes;
6976
break;
7077
}
7178

@@ -209,8 +216,15 @@ public function match($rawPathinfo)
209216
}
210217
}
211218

219+
$hasRequiredScheme = !$requiredSchemes || isset($requiredSchemes[$context->getScheme()]);
212220
if ($requiredMethods && !isset($requiredMethods[$canonicalMethod]) && !isset($requiredMethods[$requestMethod])) {
213-
$allow += $requiredMethods;
221+
if ($hasRequiredScheme) {
222+
$allow += $requiredMethods;
223+
}
224+
break;
225+
}
226+
if (!$hasRequiredScheme) {
227+
$allowSchemes += $requiredSchemes;
214228
break;
215229
}
216230

0 commit comments

Comments
 (0)