Skip to content

Commit dc91f88

Browse files
arjenmnicolas-grekas
arjenm
authored and
nicolas-grekas
committed
Fix url matcher edge cases with trailing slash
1 parent 36ac088 commit dc91f88

File tree

3 files changed

+169
-19
lines changed

3 files changed

+169
-19
lines changed

‎Matcher/Dumper/PhpMatcherTrait.php

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,14 @@
1515
use Symfony\Component\Routing\Exception\NoConfigurationException;
1616
use Symfony\Component\Routing\Exception\ResourceNotFoundException;
1717
use Symfony\Component\Routing\Matcher\RedirectableUrlMatcherInterface;
18+
use Symfony\Component\Routing\RequestContext;
1819

1920
/**
2021
* @author Nicolas Grekas <[email protected]>
2122
*
2223
* @internal
24+
*
25+
* @property RequestContext $context
2326
*/
2427
trait PhpMatcherTrait
2528
{
@@ -89,13 +92,6 @@ private function doMatch(string $pathinfo, array &$allow = [], array &$allowSche
8992
continue;
9093
}
9194

92-
if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
93-
if ($supportsRedirections && (!$requiredMethods || isset($requiredMethods['GET']))) {
94-
return $allow = $allowSchemes = [];
95-
}
96-
continue;
97-
}
98-
9995
if ($requiredHost) {
10096
if ('#' !== $requiredHost[0] ? $requiredHost !== $host : !preg_match($requiredHost, $host, $hostMatches)) {
10197
continue;
@@ -106,13 +102,21 @@ private function doMatch(string $pathinfo, array &$allow = [], array &$allowSche
106102
}
107103
}
108104

105+
if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
106+
if ($supportsRedirections && (!$requiredMethods || isset($requiredMethods['GET']))) {
107+
return $allow = $allowSchemes = [];
108+
}
109+
continue;
110+
}
111+
109112
$hasRequiredScheme = !$requiredSchemes || isset($requiredSchemes[$context->getScheme()]);
110113
if ($requiredMethods && !isset($requiredMethods[$canonicalMethod]) && !isset($requiredMethods[$requestMethod])) {
111114
if ($hasRequiredScheme) {
112115
$allow += $requiredMethods;
113116
}
114117
continue;
115118
}
119+
116120
if (!$hasRequiredScheme) {
117121
$allowSchemes += $requiredSchemes;
118122
continue;

‎Matcher/UrlMatcher.php

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ class UrlMatcher implements UrlMatcherInterface, RequestMatcherInterface
3232
const REQUIREMENT_MISMATCH = 1;
3333
const ROUTE_MATCH = 2;
3434

35+
/** @var RequestContext */
3536
protected $context;
3637

3738
/**
@@ -166,14 +167,6 @@ protected function matchCollection($pathinfo, RouteCollection $routes)
166167
}
167168
}
168169

169-
if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
170-
if ($supportsTrailingSlash && (!$requiredMethods || \in_array('GET', $requiredMethods))) {
171-
return $this->allow = $this->allowSchemes = [];
172-
}
173-
174-
continue;
175-
}
176-
177170
$hostMatches = [];
178171
if ($compiledRoute->getHostRegex() && !preg_match($compiledRoute->getHostRegex(), $this->context->getHost(), $hostMatches)) {
179172
continue;
@@ -185,6 +178,14 @@ protected function matchCollection($pathinfo, RouteCollection $routes)
185178
continue;
186179
}
187180

181+
if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
182+
if ($supportsTrailingSlash && (!$requiredMethods || \in_array('GET', $requiredMethods))) {
183+
return $this->allow = $this->allowSchemes = [];
184+
}
185+
186+
continue;
187+
}
188+
188189
$hasRequiredScheme = !$route->getSchemes() || $route->hasScheme($this->context->getScheme());
189190
if ($requiredMethods) {
190191
if (!\in_array($method, $requiredMethods)) {

‎Tests/Matcher/UrlMatcherTest.php

Lines changed: 149 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -85,9 +85,8 @@ public function testMethodNotAllowedAggregatesAllowedMethods()
8585
}
8686
}
8787

88-
public function testMatch()
88+
public function testPatternMatchAndParameterReturn()
8989
{
90-
// test the patterns are matched and parameters are returned
9190
$collection = new RouteCollection();
9291
$collection->add('foo', new Route('/foo/{bar}'));
9392
$matcher = $this->getUrlMatcher($collection);
@@ -96,14 +95,21 @@ public function testMatch()
9695
$this->fail();
9796
} catch (ResourceNotFoundException $e) {
9897
}
98+
9999
$this->assertEquals(['_route' => 'foo', 'bar' => 'baz'], $matcher->match('/foo/baz'));
100+
}
100101

102+
public function testDefaultsAreMerged()
103+
{
101104
// test that defaults are merged
102105
$collection = new RouteCollection();
103106
$collection->add('foo', new Route('/foo/{bar}', ['def' => 'test']));
104107
$matcher = $this->getUrlMatcher($collection);
105108
$this->assertEquals(['_route' => 'foo', 'bar' => 'baz', 'def' => 'test'], $matcher->match('/foo/baz'));
109+
}
106110

111+
public function testMethodIsIgnoredIfNoMethodGiven()
112+
{
107113
// test that route "method" is ignored if no method is given in the context
108114
$collection = new RouteCollection();
109115
$collection->add('foo', new Route('/foo', [], [], [], '', [], ['get', 'head']));
@@ -123,8 +129,10 @@ public function testMatch()
123129
$this->assertInternalType('array', $matcher->match('/foo'));
124130
$matcher = $this->getUrlMatcher($collection, new RequestContext('', 'head'));
125131
$this->assertInternalType('array', $matcher->match('/foo'));
132+
}
126133

127-
// route with an optional variable as the first segment
134+
public function testRouteWithOptionalVariableAsFirstSegment()
135+
{
128136
$collection = new RouteCollection();
129137
$collection->add('bar', new Route('/{bar}/foo', ['bar' => 'bar'], ['bar' => 'foo|bar']));
130138
$matcher = $this->getUrlMatcher($collection);
@@ -136,8 +144,10 @@ public function testMatch()
136144
$matcher = $this->getUrlMatcher($collection);
137145
$this->assertEquals(['_route' => 'bar', 'bar' => 'foo'], $matcher->match('/foo'));
138146
$this->assertEquals(['_route' => 'bar', 'bar' => 'bar'], $matcher->match('/'));
147+
}
139148

140-
// route with only optional variables
149+
public function testRouteWithOnlyOptionalVariables()
150+
{
141151
$collection = new RouteCollection();
142152
$collection->add('bar', new Route('/{foo}/{bar}', ['foo' => 'foo', 'bar' => 'bar'], []));
143153
$matcher = $this->getUrlMatcher($collection);
@@ -512,6 +522,141 @@ public function testWithHostOnRouteCollection()
512522
$this->assertEquals(['foo' => 'bar', '_route' => 'bar', 'locale' => 'en'], $matcher->match('/bar/bar'));
513523
}
514524

525+
public function testVariationInTrailingSlashWithHosts()
526+
{
527+
$coll = new RouteCollection();
528+
$coll->add('foo', new Route('/foo/', [], [], [], 'foo.example.com'));
529+
$coll->add('bar', new Route('/foo', [], [], [], 'bar.example.com'));
530+
531+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'foo.example.com'));
532+
$this->assertEquals(['_route' => 'foo'], $matcher->match('/foo/'));
533+
534+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'bar.example.com'));
535+
$this->assertEquals(['_route' => 'bar'], $matcher->match('/foo'));
536+
}
537+
538+
public function testVariationInTrailingSlashWithHostsInReverse()
539+
{
540+
// The order should not matter
541+
$coll = new RouteCollection();
542+
$coll->add('bar', new Route('/foo', [], [], [], 'bar.example.com'));
543+
$coll->add('foo', new Route('/foo/', [], [], [], 'foo.example.com'));
544+
545+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'foo.example.com'));
546+
$this->assertEquals(['_route' => 'foo'], $matcher->match('/foo/'));
547+
548+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'bar.example.com'));
549+
$this->assertEquals(['_route' => 'bar'], $matcher->match('/foo'));
550+
}
551+
552+
public function testVariationInTrailingSlashWithHostsAndVariable()
553+
{
554+
$coll = new RouteCollection();
555+
$coll->add('foo', new Route('/{foo}/', [], [], [], 'foo.example.com'));
556+
$coll->add('bar', new Route('/{foo}', [], [], [], 'bar.example.com'));
557+
558+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'foo.example.com'));
559+
$this->assertEquals(['foo' => 'bar', '_route' => 'foo'], $matcher->match('/bar/'));
560+
561+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'bar.example.com'));
562+
$this->assertEquals(['foo' => 'bar', '_route' => 'bar'], $matcher->match('/bar'));
563+
}
564+
565+
public function testVariationInTrailingSlashWithHostsAndVariableInReverse()
566+
{
567+
// The order should not matter
568+
$coll = new RouteCollection();
569+
$coll->add('bar', new Route('/{foo}', [], [], [], 'bar.example.com'));
570+
$coll->add('foo', new Route('/{foo}/', [], [], [], 'foo.example.com'));
571+
572+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'foo.example.com'));
573+
$this->assertEquals(['foo' => 'bar', '_route' => 'foo'], $matcher->match('/bar/'));
574+
575+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'bar.example.com'));
576+
$this->assertEquals(['foo' => 'bar', '_route' => 'bar'], $matcher->match('/bar'));
577+
}
578+
579+
public function testVariationInTrailingSlashWithMethods()
580+
{
581+
$coll = new RouteCollection();
582+
$coll->add('foo', new Route('/foo/', [], [], [], '', [], ['POST']));
583+
$coll->add('bar', new Route('/foo', [], [], [], '', [], ['GET']));
584+
585+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'POST'));
586+
$this->assertEquals(['_route' => 'foo'], $matcher->match('/foo/'));
587+
588+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET'));
589+
$this->assertEquals(['_route' => 'bar'], $matcher->match('/foo'));
590+
}
591+
592+
public function testVariationInTrailingSlashWithMethodsInReverse()
593+
{
594+
// The order should not matter
595+
$coll = new RouteCollection();
596+
$coll->add('bar', new Route('/foo', [], [], [], '', [], ['GET']));
597+
$coll->add('foo', new Route('/foo/', [], [], [], '', [], ['POST']));
598+
599+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'POST'));
600+
$this->assertEquals(['_route' => 'foo'], $matcher->match('/foo/'));
601+
602+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET'));
603+
$this->assertEquals(['_route' => 'bar'], $matcher->match('/foo'));
604+
}
605+
606+
public function testVariableVariationInTrailingSlashWithMethods()
607+
{
608+
$coll = new RouteCollection();
609+
$coll->add('foo', new Route('/{foo}/', [], [], [], '', [], ['POST']));
610+
$coll->add('bar', new Route('/{foo}', [], [], [], '', [], ['GET']));
611+
612+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'POST'));
613+
$this->assertEquals(['foo' => 'bar', '_route' => 'foo'], $matcher->match('/bar/'));
614+
615+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET'));
616+
$this->assertEquals(['foo' => 'bar', '_route' => 'bar'], $matcher->match('/bar'));
617+
}
618+
619+
public function testVariableVariationInTrailingSlashWithMethodsInReverse()
620+
{
621+
// The order should not matter
622+
$coll = new RouteCollection();
623+
$coll->add('bar', new Route('/{foo}', [], [], [], '', [], ['GET']));
624+
$coll->add('foo', new Route('/{foo}/', [], [], [], '', [], ['POST']));
625+
626+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'POST'));
627+
$this->assertEquals(['foo' => 'bar', '_route' => 'foo'], $matcher->match('/bar/'));
628+
629+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET'));
630+
$this->assertEquals(['foo' => 'bar', '_route' => 'bar'], $matcher->match('/bar'));
631+
}
632+
633+
public function testMixOfStaticAndVariableVariationInTrailingSlashWithHosts()
634+
{
635+
$coll = new RouteCollection();
636+
$coll->add('foo', new Route('/foo/', [], [], [], 'foo.example.com'));
637+
$coll->add('bar', new Route('/{foo}', [], [], [], 'bar.example.com'));
638+
639+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'foo.example.com'));
640+
$this->assertEquals(['_route' => 'foo'], $matcher->match('/foo/'));
641+
642+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET', 'bar.example.com'));
643+
$this->assertEquals(['foo' => 'bar', '_route' => 'bar'], $matcher->match('/bar'));
644+
}
645+
646+
public function testMixOfStaticAndVariableVariationInTrailingSlashWithMethods()
647+
{
648+
$coll = new RouteCollection();
649+
$coll->add('foo', new Route('/foo/', [], [], [], '', [], ['POST']));
650+
$coll->add('bar', new Route('/{foo}', [], [], [], '', [], ['GET']));
651+
652+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'POST'));
653+
$this->assertEquals(['_route' => 'foo'], $matcher->match('/foo/'));
654+
655+
$matcher = $this->getUrlMatcher($coll, new RequestContext('', 'GET'));
656+
$this->assertEquals(['foo' => 'bar', '_route' => 'bar'], $matcher->match('/bar'));
657+
$this->assertEquals(['foo' => 'foo', '_route' => 'bar'], $matcher->match('/foo'));
658+
}
659+
515660
/**
516661
* @expectedException \Symfony\Component\Routing\Exception\ResourceNotFoundException
517662
*/

0 commit comments

Comments
 (0)