From 8b1ff2112cece57104383f00ec49b84ea4304e35 Mon Sep 17 00:00:00 2001 From: Xavier Leune Date: Mon, 19 Aug 2019 17:47:52 +0200 Subject: [PATCH] [Router] Fix TraceableUrlMatcher behaviour with trailing slash --- .../Routing/Matcher/TraceableUrlMatcher.php | 90 ++++++++++++------- .../Tests/Matcher/TraceableUrlMatcherTest.php | 8 +- 2 files changed, 64 insertions(+), 34 deletions(-) diff --git a/src/Symfony/Component/Routing/Matcher/TraceableUrlMatcher.php b/src/Symfony/Component/Routing/Matcher/TraceableUrlMatcher.php index 3c3c4bfcf919e..eb2bb3d696b10 100644 --- a/src/Symfony/Component/Routing/Matcher/TraceableUrlMatcher.php +++ b/src/Symfony/Component/Routing/Matcher/TraceableUrlMatcher.php @@ -52,10 +52,40 @@ public function getTracesForRequest(Request $request) protected function matchCollection($pathinfo, RouteCollection $routes) { + // HEAD and GET are equivalent as per RFC + if ('HEAD' === $method = $this->context->getMethod()) { + $method = 'GET'; + } + $supportsTrailingSlash = '/' !== $pathinfo && '' !== $pathinfo && $this instanceof RedirectableUrlMatcherInterface; + foreach ($routes as $name => $route) { $compiledRoute = $route->compile(); + $staticPrefix = $compiledRoute->getStaticPrefix(); + $requiredMethods = $route->getMethods(); + + // check the static prefix of the URL first. Only use the more expensive preg_match when it matches + if ('' === $staticPrefix || 0 === strpos($pathinfo, $staticPrefix)) { + // no-op + } elseif (!$supportsTrailingSlash || ($requiredMethods && !\in_array('GET', $requiredMethods)) || 'GET' !== $method) { + $this->addTrace(sprintf('Path "%s" does not match', $route->getPath()), self::ROUTE_DOES_NOT_MATCH, $name, $route); + continue; + } elseif ('/' === substr($staticPrefix, -1) && substr($staticPrefix, 0, -1) === $pathinfo) { + $this->addTrace('Route matches!', self::ROUTE_MATCHES, $name, $route); + return $this->allow = []; + } else { + $this->addTrace(sprintf('Path "%s" does not match', $route->getPath()), self::ROUTE_DOES_NOT_MATCH, $name, $route); + continue; + } + $regex = $compiledRoute->getRegex(); + + if ($supportsTrailingSlash && $pos = strpos($regex, '/$')) { + $regex = substr($regex, 0, $pos).'/?$'.substr($regex, $pos + 2); + $hasTrailingSlash = true; + } else { + $hasTrailingSlash = false; + } - if (!preg_match($compiledRoute->getRegex(), $pathinfo, $matches)) { + if (!preg_match($regex, $pathinfo, $matches)) { // does it match without any requirements? $r = new Route($route->getPath(), $route->getDefaults(), [], $route->getOptions()); $cr = $r->compile(); @@ -79,54 +109,50 @@ protected function matchCollection($pathinfo, RouteCollection $routes) continue; } - // check host requirement + if ($hasTrailingSlash && '/' !== substr($pathinfo, -1)) { + if ((!$requiredMethods || \in_array('GET', $requiredMethods)) && 'GET' === $method) { + $this->addTrace('Route matches!', self::ROUTE_MATCHES, $name, $route); + return $this->allow = []; + } + $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); + continue; + } + $hostMatches = []; if ($compiledRoute->getHostRegex() && !preg_match($compiledRoute->getHostRegex(), $this->context->getHost(), $hostMatches)) { $this->addTrace(sprintf('Host "%s" does not match the requirement ("%s")', $this->context->getHost(), $route->getHost()), self::ROUTE_ALMOST_MATCHES, $name, $route); - continue; } - // check HTTP method requirement - if ($requiredMethods = $route->getMethods()) { - // HEAD and GET are equivalent as per RFC - if ('HEAD' === $method = $this->context->getMethod()) { - $method = 'GET'; - } - - if (!\in_array($method, $requiredMethods)) { - $this->allow = array_merge($this->allow, $requiredMethods); - - $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); + $status = $this->handleRouteRequirements($pathinfo, $name, $route); - continue; + if (self::REQUIREMENT_MISMATCH === $status[0]) { + if ($route->getCondition()) { + $this->addTrace(sprintf('Condition "%s" does not evaluate to "true"', $route->getCondition()), self::ROUTE_ALMOST_MATCHES, $name, $route); + } else { + $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); } - } - // check condition - if ($condition = $route->getCondition()) { - if (!$this->getExpressionLanguage()->evaluate($condition, ['context' => $this->context, 'request' => $this->request ?: $this->createRequest($pathinfo)])) { - $this->addTrace(sprintf('Condition "%s" does not evaluate to "true"', $condition), self::ROUTE_ALMOST_MATCHES, $name, $route); - - continue; - } + continue; } - // check HTTP scheme requirement - if ($requiredSchemes = $route->getSchemes()) { - $scheme = $this->context->getScheme(); - - if (!$route->hasScheme($scheme)) { - $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); + // check HTTP method requirement + if ($requiredMethods) { + if (!\in_array($method, $requiredMethods)) { + if (self::REQUIREMENT_MATCH === $status[0]) { + $this->allow = array_merge($this->allow, $requiredMethods); + } + $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); - return true; + continue; } } $this->addTrace('Route matches!', self::ROUTE_MATCHES, $name, $route); - - return true; + return $this->getAttributes($route, $name, array_replace($matches, $hostMatches, isset($status[1]) ? $status[1] : [])); } + + return []; } private function addTrace($log, $level = self::ROUTE_DOES_NOT_MATCH, $name = null, $route = null) diff --git a/src/Symfony/Component/Routing/Tests/Matcher/TraceableUrlMatcherTest.php b/src/Symfony/Component/Routing/Tests/Matcher/TraceableUrlMatcherTest.php index 04ddf845f0d7e..b31f99e0c4964 100644 --- a/src/Symfony/Component/Routing/Tests/Matcher/TraceableUrlMatcherTest.php +++ b/src/Symfony/Component/Routing/Tests/Matcher/TraceableUrlMatcherTest.php @@ -11,14 +11,13 @@ namespace Symfony\Component\Routing\Tests\Matcher; -use PHPUnit\Framework\TestCase; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\Routing\Matcher\TraceableUrlMatcher; use Symfony\Component\Routing\RequestContext; use Symfony\Component\Routing\Route; use Symfony\Component\Routing\RouteCollection; -class TraceableUrlMatcherTest extends TestCase +class TraceableUrlMatcherTest extends UrlMatcherTest { public function test() { @@ -119,4 +118,9 @@ public function testRoutesWithConditions() $traces = $matcher->getTracesForRequest($matchingRequest); $this->assertEquals('Route matches!', $traces[0]['log']); } + + protected function getUrlMatcher(RouteCollection $routes, RequestContext $context = null) + { + return new TraceableUrlMatcher($routes, $context ?: new RequestContext()); + } }