8000 bug #33244 [Router] Fix TraceableUrlMatcher behaviour with trailing s… · symfony/symfony@9672f4c · GitHub
[go: up one dir, main page]
Skip to content

Commit 9672f4c

Browse files
nicolas-grekasnicolas-grekas
committed
bug #33244 [Router] Fix TraceableUrlMatcher behaviour with trailing slash (Xavier Leune)
This PR was merged into the 3.4 branch. Discussion ---------- [Router] Fix TraceableUrlMatcher behaviour with trailing slash | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | #32149 | License | MIT | Doc PR | ¤ <!-- Replace this notice by a short README for your feature/bugfix. This will help people understand your PR and can be used as a start for the documentation. Additionally (see https://symfony.com/roadmap): - Bug fixes must be submitted against the lowest maintained branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against branch 4.4. - Legacy code removals go to the master branch. --> This pull requests fixes the bug #32149. This issue was about TraceableUrlMatcher having a wrong behaviour regarding trailing slashes (according to UrlMatcher and documentation). With this pull requests, the test class TraceableUrlMatcherTest now extends UrlMatcherTest, to prevent such behaviour digression. Thanks @nicolas-grekas for his feedback on the issue #32149 Commits ------- fd1cb44 [Router] Fix TraceableUrlMatcher behaviour with trailing slash
2 parents 727d431 + fd1cb44 commit 9672f4c

File tree

2 files changed

+64
-33
lines changed

2 files changed

+64
-33
lines changed

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

Lines changed: 58 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,41 @@ public function getTracesForRequest(Request $request)
5252

5353
protected function matchCollection($pathinfo, RouteCollection $routes)
5454
{
55+
// HEAD and GET are equivalent as per RFC
56+
if ('HEAD' === $method = $this->context->getMethod()) {
57+
$method = 'GET';
58+
}
59+
$supportsTrailingSlash = '/' !== $pathinfo && '' !== $pathinfo && $this instanceof RedirectableUrlMatcherInterface;
60+
5561
foreach ($routes as $name => $route) {
5662
$compiledRoute = $route->compile();
63+
$staticPrefix = $compiledRoute->getStaticPrefix();
64+
$requiredMethods = $route->getMethods();
65+
66+
// check the static prefix of the URL first. Only use the more expensive preg_match when it matches
67+
if ('' === $staticPrefix || 0 === strpos($pathinfo, $staticPrefix)) {
68+
// no-op
69+
} elseif (!$supportsTrailingSlash || ($requiredMethods && !\in_array('GET', $requiredMethods)) || 'GET' !== $method) {
70+
$this->addTrace(sprintf('Path "%s" does not match', $route->getPath()), self::ROUTE_DOES_NOT_MATCH, $name, $route);
71+
continue;
72+
} elseif ('/' === substr($staticPrefix, -1) && substr($staticPrefix, 0, -1) === $pathinfo) {
73+
$this->addTrace('Route matches!', self::ROUTE_MATCHES, $name, $route);
74+
75+
return $this->allow = [];
76+
} else {
77+
$this->addTrace(sprintf('Path "%s" does not match', $route->getPath()), self::ROUTE_DOES_NOT_MATCH, $name, $route);
78+
continue;
79+
}
80+
$regex = $compiledRoute->getRegex();
81+
82+
if ($supportsTrailingSlash && $pos = strpos($regex, '/$')) {
83+
$regex = substr($regex, 0, $pos).'/?$'.substr($regex, $pos + 2);
84+
$hasTrailingSlash = true;
85+
} else {
86+
$hasTrailingSlash = false;
87+
}
5788

58-
if (!preg_match($compiledRoute->getRegex(), $pathinfo, $matches)) {
89+
if (!preg_match($regex, $pathinfo, $matches)) {
5990
// does it match without any requirements?
6091
$r = new Route($route->getPath(), $route->getDefaults(), [], $route->getOptions());
6192
$cr = $r->compile();
@@ -79,53 +110,49 @@ protected function matchCollection($pathinfo, RouteCollection $routes)
79110
continue;
80111
}
81112

82-
// check host requirement
113+
if ($hasTrailingSlash && '/' !== substr($pathinfo, -1)) {
114+
if ((!$requiredMethods || \in_array('GET', $requiredMethods)) && 'GET' === $method) {
115+
$this->addTrace('Route matches!', self::ROUTE_MATCHES, $name, $route);
116+
117+
return $this->allow = [];
118+
}
119+
$this->addTrace(sprintf('Method "%s" does not match any of the required methods (%s)', $this->context->getMethod(), implode(', ', $requiredMethods)), self::ROUTE_ALMOST_MATCHES, $name, $route);
120+
continue;
121+
}
122+
83123
$hostMatches = [];
84124
if ($compiledRoute->getHostRegex() && !preg_match($compiledRoute->getHostRegex(), $this->context->getHost(), $hostMatches)) {
85125
$this->addTrace(sprintf('Host "%s" does not match the requirement ("%s")', $this->context->getHost(), $route->getHost()), self::ROUTE_ALMOST_MATCHES, $name, $route);
86-
87126
continue;
88127
}
89128

90-
// check HTTP method requirement
91-
if ($requiredMethods = $route->getMethods()) {
92-
// HEAD and GET are equivalent as per RFC
93-
if ('HEAD' === $method = $this->context->getMethod()) {
94-
$method = 'GET';
95-
}
96-
97-
if (!\in_array($method, $requiredMethods)) {
98-
$this->allow = array_merge($this->allow, $requiredMethods);
99-
100-
$this->addTrace(sprintf('Method "%s" does not match any of the required methods (%s)', $this->context->getMethod(), implode(', ', $requiredMethods)), self::ROUTE_ALMOST_MATCHES, $name, $route);
129+
$status = $this->handleRouteRequirements($pathinfo, $name, $route);
101130

102-
continue;
131+
if (self::REQUIREMENT_MISMATCH === $status[0]) {
132+
if ($route->getCondition()) {
133+
$this->addTrace(sprintf('Condition "%s" does not evaluate to "true"', $route->getCondition()), self::ROUTE_ALMOST_MATCHES, $name, $route);
134+
} else {
135+
$this->addTrace(sprintf('Scheme "%s" does not match any of the required schemes (%s); the user will be redirected to first required scheme', $this->getContext()->getScheme(), implode(', ', $route->getSchemes())), self::ROUTE_ALMOST_MATCHES, $name, $route);
103136
}
104-
}
105137

106-
// check condition
107-
if ($condition = $route->getCondition()) {
108-
if (!$this->getExpressionLanguage()->evaluate($condition, ['context' => $this->context, 'request' => $this->request ?: $this->createRequest($pathinfo)])) {
109-
$this->addTrace(sprintf('Condition "%s" does not evaluate to "true"', $condition), self::ROUTE_ALMOST_MATCHES, $name, $route);
110-
111-
continue;
112-
}
138+
continue;
113139
}
114140

115-
// check HTTP scheme requirement
116-
if ($requiredSchemes = $route->getSchemes()) {
117-
$scheme = $this->context->getScheme();
118-
119-
if (!$route->hasScheme($scheme)) {
120-
$this->addTrace(sprintf('Scheme "%s" does not match any of the required schemes (%s); the user will be redirected to first required scheme', $scheme, implode(', ', $requiredSchemes)), self::ROUTE_ALMOST_MATCHES, $name, $route);
141+
// check HTTP method requirement
142+
if ($requiredMethods) {
143+
if (!\in_array($method, $requiredMethods)) {
144+
if (self::REQUIREMENT_MATCH === $status[0]) {
145+
$this->allow = array_merge($this->allow, $requiredMethods);
146+
}
147+
$this->addTrace(sprintf('Method "%s" does not match any of the required methods (%s)', $this->context->getMethod(), implode(', ', $requiredMethods)), self::ROUTE_ALMOST_MATCHES, $name, $route);
121148

122-
return true;
149+
continue;
123150
}
124151
}
125152

126153
$this->addTrace('Route matches!', self::ROUTE_MATCHES, $name, $route);
127154

128-
return true;
155+
return $this->getAttributes($route, $name, array_replace($matches, $hostMatches, isset($status[1]) ? $status[1] : []));
129156
}
130157

131158
return [];

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,13 @@
1111

1212
namespace Symfony\Component\Routing\Tests\Matcher;
1313

14-
use PHPUnit\Framework\TestCase;
1514
use Symfony\Component\HttpFoundation\Request;
1615
use Symfony\Component\Routing\Matcher\TraceableUrlMatcher;
1716
use Symfony\Component\Routing\RequestContext;
1817
use Symfony\Component\Routing\Route;
1918
use Symfony\Component\Routing\RouteCollection;
2019

21-
class TraceableUrlMatcherTest extends TestCase
20+
class TraceableUrlMatcherTest extends UrlMatcherTest
2221
{
2322
public function test()
2423
{
@@ -119,4 +118,9 @@ public function testRoutesWithConditions()
119118
$traces = $matcher->getTracesForRequest($matchingRequest);
120119
$this->assertEquals('Route matches!', $traces[0]['log']);
121120
}
121+
122+
protected function getUrlMatcher(RouteCollection $routes, RequestContext $context = null)
123+
{
124+
return new TraceableUrlMatcher($routes, $context ?: new RequestContext());
125+
}
122126
}

0 commit comments

Comments
 (0)
0