Skip to content

Commit 3180327

Browse files
fabpotfabpot
committed
bug #26253 [Routing] Fix suffix aggregation (nicolas-grekas)
This PR was merged into the 4.1-dev branch. Discussion ---------- [Routing] Fix suffix aggregation | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Commits ------- 1466597bed [Routing] Fix suffix aggregation
2 parents 7d29a4d + 7e196cb commit 3180327

File tree

3 files changed

+139
-58
lines changed

3 files changed

+139
-58
lines changed

‎Matcher/Dumper/StaticPrefixCollection.php‎

Lines changed: 38 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -65,31 +65,57 @@ public function getRoutes(): array
6565
*/
6666
public function addRoute(string $prefix, $route, string $staticPrefix = null)
6767
{
68-
$this->guardAgainstAddingNotAcceptedRoutes($prefix);
6968
if (null === $staticPrefix) {
7069
list($prefix, $staticPrefix) = $this->getCommonPrefix($prefix, $prefix);
7170
}
7271

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

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

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

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

86-
if ($this->staticPrefixes[$i] !== $this->prefixes[$i] && 0 === strpos($staticPrefix, $this->staticPrefixes[$i])) {
87-
break;
85+
if ($this->prefix === $staticPrefix && $this->prefix === $this->staticPrefixes[$i]) {
86+
// the new route and the previous one have no static prefix
87+
break;
88+
}
89+
90+
if ($this->prefixes[$i] !== $this->staticPrefixes[$i] && $this->prefix === $this->staticPrefixes[$i]) {
91+
// the previous route is non-static and has no static prefix
92+
break;
93+
}
94+
95+
if ($prefix !== $staticPrefix && $this->prefix === $staticPrefix) {
96+
// the new route is non-static and has no static prefix
97+
break;
98+
}
99+
100+
continue;
88101
}
89102

90-
if ($staticPrefix !== $prefix && 0 === strpos($this->staticPrefixes[$i], $staticPrefix)) {
91-
break;
103+
if ($item instanceof self && $this->prefixes[$i] === $commonPrefix) {
104+
// the new route is a child of a previous one, let's nest it
105+
$item->addRoute($prefix, $route, $staticPrefix);
106+
} else {
107+
// the new route and a previous one have a common prefix, let's merge them
108+
$child = new self($commonPrefix);
109+
list($child->prefixes[0], $child->staticPrefixes[0]) = $child->getCommonPrefix($this->prefixes[$i], $this->prefixes[$i]);
110+
list($child->prefixes[1], $child->staticPrefixes[1]) = $child->getCommonPrefix($prefix, $prefix);
111+
$child->items = array($this->items[$i], $route);
112+
113+
$this->staticPrefixes[$i] = $commonStaticPrefix;
114+
$this->prefixes[$i] = $commonPrefix;
115+
$this->items[$i] = $child;
92116
}
117+
118+
return;
93119
}
94120

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

118-
/**
119-
* Tries to combine a route with another route or group.
120-
*/
121-
private function groupWithItem(int $i, string $prefix, string $staticPrefix, $route): bool
122-
{
123-
list($commonPrefix, $commonStaticPrefix) = $this->getCommonPrefix($prefix, $this->prefixes[$i]);
124-
125-
if (\strlen($this->prefix) >= \strlen($commonPrefix)) {
126-
return false;
127-
}
128-
129-
$child = new self($commonPrefix);
130-
131-
$child->staticPrefixes = array($this->staticPrefixes[$i], $staticPrefix);
132-
$child->prefixes = array($this->prefixes[$i], $prefix);
133-
$child->items = array($this->items[$i], $route);
134-
135-
$this->staticPrefixes[$i] = $commonStaticPrefix;
136-
$this->prefixes[$i] = $commonPrefix;
137-
$this->items[$i] = $child;
138-
139-
return true;
140-
}
141-
142-
/**
143-
* Checks whether a prefix can be contained within the group.
144-
*/
145-
private function accepts(string $prefix): bool
146-
{
147-
return 0 === strpos($prefix, $this->prefix) && '?' !== ($prefix[\strlen($this->prefix)] ?? '');
148-
}
149-
150144
/**
151145
* Gets the full and static common prefixes between two route patterns.
152146
*
@@ -195,18 +189,4 @@ private function getCommonPrefix(string $prefix, string $anotherPrefix): array
195189

196190
return array(substr($prefix, 0, $i), substr($prefix, 0, $staticLength ?? $i));
197191
}
198-
199-
/**
200-
* Guards against adding incompatible prefixes in a group.
201-
*
202-
* @throws \LogicException when a prefix does not belong in a group
203-
*/
204-
private function guardAgainstAddingNotAcceptedRoutes(string $prefix): void
205-
{
206-
if (!$this->accepts($prefix)) {
207-
$message = sprintf('Could not add route with prefix %s to collection with prefix %s', $prefix, $this->prefix);
208-
209-
throw new \LogicException($message);
210-
}
211-
}
212192
}

‎Tests/Fixtures/dumper/url_matcher12.php‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
<?php
2+
3+
use Symfony\Component\Routing\Exception\MethodNotAllowedException;
4+
use Symfony\Component\Routing\Exception\ResourceNotFoundException;
5+
use Symfony\Component\Routing\RequestContext;
6+
7+
/**
8+
* This class has been auto-generated
9+
* by the Symfony Routing Component.
10+
*/
11+
class ProjectUrlMatcher extends Symfony\Component\Routing\Matcher\UrlMatcher
12+
{
13+
public function __construct(RequestContext $context)
14+
{
15+
$this->context = $context;
16+
}
17+
18+
public function match($rawPathinfo)
19+
{
20+
$allow = array();
21+
$pathinfo = rawurldecode($rawPathinfo);
22+
$trimmedPathinfo = rtrim($pathinfo, '/');
23+
$context = $this->context;
24+
$requestMethod = $canonicalMethod = $context->getMethod();
25+
26+
if ('HEAD' === $requestMethod) {
27+
$canonicalMethod = 'GET';
28+
}
29+
30+
$matchedPathinfo = $pathinfo;
31+
$regexList = array(
32+
0 => '{^(?'
33+
.'|/abc([^/]++)(?'
34+
.'|/1(?'
35+
.'|(*:27)'
36+
.'|0(?'
37+
.'|(*:38)'
38+
.'|0(*:46)'
39+
.')'
40+
.')'
41+
.'|/2(?'
42+
.'|(*:60)'
43+
.'|0(?'
44+
.'|(*:71)'
45+
.'|0(*:79)'
46+
.')'
47+
.')'
48+
.')'
49+
.')$}sD',
50+
);
51+
52+
foreach ($regexList as $offset => $regex) {
53+
while (preg_match($regex, $matchedPathinfo, $matches)) {
54+
switch ($m = (int) $matches['MARK']) {
55+
default:
56+
$routes = array(
57+
27 => array(array('_route' => 'r1'), array('foo'), null, null),
58+
38 => array(array('_route' => 'r10'), array('foo'), null, null),
59+
46 => array(array('_route' => 'r100'), array('foo'), null, null),
60+
60 => array(array('_route' => 'r2'), array('foo'), null, null),
61+
71 => array(array('_route' => 'r20'), array('foo'), null, null),
62+
79 => array(array('_route' => 'r200'), array('foo'), null, null),
63+
);
64+
65+
list($ret, $vars, $requiredMethods, $requiredSchemes) = $routes[$m];
66+
67+
foreach ($vars as $i => $v) {
68+
if (isset($matches[1 + $i])) {
69+
$ret[$v] = $matches[1 + $i];
70+
}
71+
}
72+
73+
if ($requiredMethods && !isset($requiredMethods[$canonicalMethod]) && !isset($requiredMethods[$requestMethod])) {
74+
$allow += $requiredMethods;
75+
break;
76+
}
77+
78+
return $ret;
79+
}
80+
81+
if (79 === $m) {
82+
break;
83+
}
84+
$regex = substr_replace($regex, 'F', $m - $offset, 1 + strlen($m));
85+
$offset += strlen($m);
86+
}
87+
}
88+
89+
throw $allow ? new MethodNotAllowedException(array_keys($allow)) : new ResourceNotFoundException();
90+
}
91+
}

‎Tests/Matcher/Dumper/PhpMatcherDumperTest.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -458,6 +458,15 @@ public function getRouteCollections()
458458
$demoCollection->addRequirements(array('_locale' => 'en|fr'));
459459
$demoCollection->addDefaults(array('_locale' => 'en'));
460460

461+
/* test case 12 */
462+
$suffixCollection = new RouteCollection();
463+
$suffixCollection->add('r1', new Route('abc{foo}/1'));
464+
$suffixCollection->add('r2', new Route('abc{foo}/2'));
465+
$suffixCollection->add('r10', new Route('abc{foo}/10'));
466+
$suffixCollection->add('r20', new Route('abc{foo}/20'));
467+
$suffixCollection->add('r100', new Route('abc{foo}/100'));
468+
$suffixCollection->add('r200', new Route('abc{foo}/200'));
469+
461470
return array(
462471
array(new RouteCollection(), 'url_matcher0.php', array()),
463472
array($collection, 'url_matcher1.php', array()),
@@ -471,6 +480,7 @@ public function getRouteCollections()
471480
array($hostTreeCollection, 'url_matcher9.php', array()),
472481
array($chunkedCollection, 'url_matcher10.php', array()),
473482
array($demoCollection, 'url_matcher11.php', array('base_class' => 'Symfony\Component\Routing\Tests\Fixtures\RedirectableUrlMatcher')),
483+
array($suffixCollection, 'url_matcher12.php', array()),
474484
);
475485
}
476486

0 commit comments

Comments
 (0)