8000 [Routing] Fix suffix aggregation by nicolas-grekas · Pull Request #26253 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Routing] Fix suffix aggregation #26253

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 21, 2018
Merged

[Routing] Fix suffix aggregation #26253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 38 additions & 58 deletions src/Symfony/Component/Routing/Matcher/Dumper/StaticPrefixCollection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,31 +65,57 @@ public function getRoutes(): array
*/
public function addRoute(string $prefix, $route, string $staticPrefix = null)
{
$this->guardAgainstAddingNotAcceptedRoutes($prefix);
if (null === $staticPrefix) {
list($prefix, $staticPrefix) = $this->getCommonPrefix($prefix, $prefix);
}

for ($i = \count($this->items) - 1; 0 <= $i; --$i) {
$item = $this->items[$i];

if ($item instanceof self && $item->accepts($prefix)) {
$item->addRoute($prefix, $route, $staticPrefix);
list($commonPrefix, $commonStaticPrefix) = $this->getCommonPrefix($prefix, $this->prefixes[$i]);

return;
}
if ($this->prefix === $commonPrefix) {
// the new route and a previous one have no common prefix, let's see if they are exclusive to each others

if ($this->groupWithItem($i, $prefix, $staticPrefix, $route)) {
return;
}
if ($this->prefix !== $staticPrefix && $this->prefix !== $this->staticPrefixes[$i]) {
// the new route and the previous one have exclusive static prefixes
continue;
}

if ($this->staticPrefixes[$i] !== $this->prefixes[$i] && 0 === strpos($staticPrefix, $this->staticPrefixes[$i])) {
break;
if ($this->prefix === $staticPrefix && $this->prefix === $this->staticPrefixes[$i]) {
// the new route and the previous one have no static prefix
break;
}

if ($this->prefixes[$i] !== $this->staticPrefixes[$i] && $this->prefix === $this->staticPrefixes[$i]) {
// the previous route is non-static and has no static prefix
break;
}

if ($prefix !== $staticPrefix && $this->prefix === $staticPrefix) {
// the new route is non-static and has no static prefix
break;
}

continue;
}

if ($staticPrefix !== $prefix && 0 === strpos($this->staticPrefixes[$i], $staticPrefix)) {
break;
if ($item instanceof self && $this->prefixes[$i] === $commonPrefix) {
// the new route is a child of a previous one, let's nest it
$item->addRoute($prefix, $route, $staticPrefix);
} else {
// the new route and a previous one have a common prefix, let's merge them
$child = new self($commonPrefix);
list($child->prefixes[0], $child->staticPrefixes[0]) = $child->getCommonPrefix($this->prefixes[$i], $this->prefixes[$i]);
list($child->prefixes[1], $child->staticPrefixes[1]) = $child->getCommonPrefix($prefix, $prefix);
$child->items = array($this->items[$i], $route);

$this->staticPrefixes[$i] = $commonStaticPrefix;
$this->prefixes[$i] = $commonPrefix;
$this->items[$i] = $child;
}

return;
}

// No optimised case was found, in this case we simple add the route for possible
Expand All @@ -115,38 +141,6 @@ public function populateCollection(RouteCollection $routes): RouteCollection
return $routes;
}

/**
* Tries to combine a route with another route or group.
*/
private function groupWithItem(int $i, string $prefix, string $staticPrefix, $route): bool
{
list($commonPrefix, $commonStaticPrefix) = $this->getCommonPrefix($prefix, $this->prefixes[$i]);

if (\strlen($this->prefix) >= \strlen($commonPrefix)) {
return false;
}

$child = new self($commonPrefix);

$child->staticPrefixes = array($this->staticPrefixes[$i], $staticPrefix);
$child->prefixes = array($this->prefixes[$i], $prefix);
$child->items = array($this->items[$i], $route);

$this->staticPrefixes[$i] = $commonStaticPrefix;
$this->prefixes[$i] = $commonPrefix;
$this->items[$i] = $child;

return true;
}

/**
* Checks whether a prefix can be contained within the group.
*/
private function accepts(string $prefix): bool
{
return 0 === strpos($prefix, $this->prefix) && '?' !== ($prefix[\strlen($this->prefix)] ?? '');
}

/**
* Gets the full and static common prefixes between two route patterns.
*
Expand Down Expand Up @@ -195,18 +189,4 @@ private function getCommonPrefix(string $prefix, string $anotherPrefix): array

return array(substr($prefix, 0, $i), substr($prefix, 0, $staticLength ?? $i));
}

/**
* Guards against adding incompatible prefixes in a group.
*
* @throws \LogicException when a prefix does not belong in a group
*/
private function guardAgainstAddingNotAcceptedRoutes(string $prefix): void
{
if (!$this->accepts($prefix)) {
$message = sprintf('Could not add route with prefix %s to collection with prefix %s', $prefix, $this->prefix);

throw new \LogicException($message); 8000
}
}
}
91 changes: 91 additions & 0 deletions src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher12.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
<?php

use Symfony\Component\Routing\Exception\MethodNotAllowedException;
use Symfony\Component\Routing\Exception\ResourceNotFoundException;
use Symfony\Component\Routing\RequestContext;

/**
* This class has been auto-generated
* by the Symfony Routing Component.
*/
class ProjectUrlMatcher extends Symfony\Component\Routing\Matcher\UrlMatcher
{
public function __construct(RequestContext $context)
{
$this->context = $context;
}

public function match($rawPathinfo)
{
$allow = array();
$pathinfo = rawurldecode($rawPathinfo);
$trimmedPathinfo = rtrim($pathinfo, '/');
$context = $this->context;
$requestMethod = $canonicalMethod = $context->getMethod();

if ('HEAD' === $requestMethod) {
$canonicalMethod = 'GET';
}

$matchedPathinfo = $pathinfo;
$regexList = array(
0 => '{^(?'
.'|/abc([^/]++)(?'
.'|/1(?'
.'|(*:27)'
.'|0(?'
.'|(*:38)'
.'|0(*:46)'
.')'
.')'
.'|/2(?'
.'|(*:60)'
.'|0(?'
.'|(*:71)'
.'|0(*:79)'
.')'
.')'
.')'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without this change, a list is generated instead of a tree:

                        .'|/2(*:33)'
                        .'|/10(*:43)'
                        .'|/20(*:53)'
                        .'|/100(*:64)'
                        .'|/200(*:75)'

.')$}sD',
);

foreach ($regexList as $offset => $regex) {
while (preg_match($regex, $matchedPathinfo, $matches)) {
switch ($m = (int) $matches['MARK']) {
default:
$routes = array(
27 => array(array('_route' => 'r1'), array('foo'), null, null),
38 => array(array('_route' => 'r10'), array('foo'), null, null),
46 => array(array('_route' => 'r100'), array('foo'), null, null),
60 => array(array('_route' => 'r2'), array('foo'), null, null),
71 => array(array('_route' => 'r20'), array('foo'), null, null),
79 => array(array('_route' => 'r200'), array('foo'), null, null),
);

list($ret, $vars, $requiredMethods, $requiredSchemes) = $routes[$m];

foreach ($vars as $i => $v) {
if (isset($matches[1 + $i])) {
$ret[$v] = $matches[1 + $i];
}
}

if ($requiredMethods && !isset($requiredMethods[$canonicalMethod]) && !isset($requiredMethods[$requestMethod])) {
$allow += $requiredMethods;
break;
}

return $ret;
}

if (79 === $m) {
break;
}
$regex = substr_replace($regex, 'F', $m - $offset, 1 + strlen($m));
$offset += strlen($m);
}
}

throw $allow ? new MethodNotAllowedException(array_keys($allow)) : new ResourceNotFoundException();
}
}
10 changes: 10 additions & 0 deletions src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,15 @@ public function getRouteCollections()
$demoCollection->addRequirements(array('_locale' => 'en|fr'));
$demoCollection->addDefaults(array('_locale' => 'en'));

/* test case 12 */
$suffixCollection = new RouteCollection();
$suffixCollection->add('r1', new Route('abc{foo}/1'));
$suffixCollection->add('r2', new Route('abc{foo}/2'));
$suffixCollection->add('r10', new Route('abc{foo}/10'));
$suffixCollection->add('r20', new Route('abc{foo}/20'));
$suffixCollection->add('r100', new Route('abc{foo}/100'));
$suffixCollection->add('r200', new Route('abc{foo}/200'));

return array(
array(new RouteCollection(), 'url_matcher0.php', array()),
array($collection, 'url_matcher1.php', array()),
Expand All @@ -471,6 +480,7 @@ public function getRouteCollections()
array($hostTreeCollection, 'url_matcher9.php', array()),
array($chunkedCollection, 'url_matcher10.php', array()),
array($demoCollection, 'url_matcher11.php', array('base_class' => 'Symfony\Component\Routing\Tests\Fixtures\RedirectableUrlMatcher')),
array($suffixCollection, 'url_matcher12.php', array()),
);
}

Expand Down
0