8000 merged branch vicb/routing_improvements (PR #3876) · symfony/symfony@0c57db1 · GitHub
[go: up one dir, main page]
Skip to content

Commit 0c57db1

Browse files
fabpotfabpot
committed
merged branch vicb/routing_improvements (PR #3876)
Commits ------- 9307f5b [Routing] Implement bug fixes and enhancements Discussion ---------- [Routing] Implement bug fixes and enhancements (from @Tobion) This is mainly #3754 with some minor formatting changes. Original PR message from @Tobion: Here a list of what is fixed. Tests pass. 1. The `RouteCollection` states > it overrides existing routes with the same name defined in the instance or its children and parents. But this is not true for `->addCollection()` but only for `->add()`. addCollection does not remove routes with the same name in its parents (only in its children). I don't think this is on purpose. So I fixed it by making sure there can only be one route per name in all connected collections. This way we can also simplify `->get()` and `->remove()` and improve performance since we can stop iterating recursively when we found the first and only route with a given name. See `testUniqueRouteWithGivenName` that fails in the old code but works now. 2. There was an bug with `$collection->addPrefix('0');` that didn't apply the starting slash. Fixed and test case added. 3. There is an issue with `->get()` that I also think is not intended. Currently it allows to access a sub-RouteCollection by specifing $name as array integer index. But according to the PHPdoc you should only be allowed to receive a Route instance and not a RouteCollection. See `testGet` that has a failing test case. I fixed this behavior. 4. Then I recognized that `->addCollection` depended on the order of applying them. So $collection1->addCollection($collection2, '/b'); $collection2->addCollection($collection3, '/c'); $rootCollection->addCollection($collection1, '/a'); had a different pattern result from $collection2->addCollection($collection3, '/c'); $collection1->addCollection($collection2, '/b'); $rootCollection->addCollection($collection1, '/a'); Fixed and test case added. See `testPatternDoesNotChangeWhenDefinitionOrderChanges`. 5. PHP could have ended in an infinite loop when one tried to add an existing RouteCollection to the tree. Fixed by throwing an exception when this situation is detected. See tests `testCannotSelfJoinCollection` and `testCannotAddExistingCollectionToTree`. 6. I made `setParent()` private because its not useful outside the class itself. And `remove()` also removes the route from its parents. Added public `getRoot()` method. 7. The `Route` class throwed a PHP warning when trying to set an empty requirement. 8. Fixed issue #3777. See discussion there for more info. I fixed it by removing the over-optimization that was introduced in 91f4097 but didn't work properly. One cannot reorder the route definitions, as is was done, because then the wrong route might me matched before the correct one. If one really wanted to do that, it would require to calculate the intersection of two regular expressions to determine if they can be grouped together (a tool that would also be useful to check whether a route is unreachable, see discussion in #3678) We can only safely optimize routes with a static prefix within a RouteCollection, not across multiple RouteCollections with different parents. This is especially true when using variables and regular expressions requirements. I could however apply an optimization that was missing yet: Collections with a single route were missing the static prefix optimization with `0 === strpos()`. 9. Fixed an issue where the `PhpMatcherDumper` would not apply the optimization if the root collection to be dumped has a prefix itself. For this I had to rewrite `compileRoutes`. It is also much easier to understand now. Addionally I added many comments and PHPdoc because complex recursive methods like this are still hard to grasp. I added a test case for this (`url_matcher3.php`). 10. Fix that `Route::compile` needs to recompile a route once it is modified. Otherwise we have a wrong result. Test case added.
2 parents 3469713 + 9307f5b commit 0c57db1

File tree

11 files changed

+589
-223
lines changed

11 files changed

+589
-223
lines changed

‎CHANGELOG-2.1.md

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -363,9 +363,11 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c
363363
* added a TraceableUrlMatcher
364364
* added the possibility to define options, default values and requirements for placeholders in prefix, including imported routes
365365
* added RouterInterface::getRouteCollection
366-
* [BC BREAK] the UrlMatcher urldecodes the route parameters only once, they were
367-
decoded twice before. Note that the `urldecode()` calls have been change for a
368-
single `rawurldecode()` in order to support `+` for input paths.
366+
* [BC BREAK] the UrlMatcher urldecodes the route parameters only once, they were decoded twice before.
367+
Note that the `urldecode()` calls have been changed for a single `rawurldecode()` in order to support `+` for input paths.
368+
* added RouteCollection::getRoot method to retrieve the root of a RouteCollection tree
369+
* [BC BREAK] made RouteCollection::setParent private which could not have been used anyway without creating inconsistencies
370+
* [BC BREAK] RouteCollection::remove also removes a route from parent collections (not only from its children)
369371

370372
### Security
371373

‎src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php

Lines changed: 104 additions & 115 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
* PhpMatcherDumper creates a PHP class able to match URLs for a given set of routes.
1919
*
2020
* @author Fabien Potencier <fabien@symfony.com>
21+
* @a 9E88 uthor Tobias Schultze <http://tobion.de>
2122
*/
2223
class PhpMatcherDumper extends MatcherDumper
2324
{
@@ -42,23 +43,49 @@ public function dump(array $options = array())
4243

4344
// trailing slash support is only enabled if we know how to redirect the user
4445
$interfaces = class_implements($options['base_class']);
45-
$supportsRedirections = isset($interfaces['Symfony\Component\Routing\Matcher\RedirectableUrlMatcherInterface']);
46-
47-
return
48-
$this->startClass($options['class'], $options['base_class']).
49-
$this->addConstructor().
50-
$this->addMatcher($supportsRedirections).
51-
$this->endClass()
52-
;
46+
$supportsRedirections = isset($interfaces['Symfony\\Component\\Routing\\Matcher\\RedirectableUrlMatcherInterface']);
47+
48+
return <<<EOF
49+
<?php
50+
51+
use Symfony\Component\Routing\Exception\MethodNotAllowedException;
52+
use Symfony\Component\Routing\Exception\ResourceNotFoundException;
53+
use Symfony\Component\Routing\RequestContext;
54+
55+
/**
56+
* {$options['class']}
57+
*
58+
* This class has been auto-generated
59+
* by the Symfony Routing Component.
60+
*/
61+
class {$options['class']} extends {$options['base_class']}
62+
{
63+
/**
64+
* Constructor.
65+
*/
66+
public function __construct(RequestContext \$context)
67+
{
68+
\$this->context = \$context;
5369
}
5470
55-
private function addMatcher($supportsRedirections)
71+
{$this->generateMatchMethod($supportsRedirections)}
72+
}
73+
74+
EOF;
75+
}
76+
77+
/**
78+
* Generates the code for the match method implementing UrlMatcherInterface.
79+
*
80+
* @param Boolean $supportsRedirections Whether redirections are supported by the base class
81+
*
82+
* @return string Match method as PHP code
83+
*/
84+
private function generateMatchMethod($supportsRedirections)
5685
{
57-
// we need to deep clone the routes as we will modify the structure to optimize the dump
58-
$code = rtrim($this->compileRoutes(clone $this->getRoutes(), $supportsRedirections), "\n");
86+
$code = rtrim($this->compileRoutes($this->getRoutes(), $supportsRedirections), "\n");
5987

6088
return <<<EOF
61-
6289
public function match(\$pathinfo)
6390
{
6491
\$allow = array();
@@ -68,78 +95,83 @@ public function match(\$pathinfo)
6895
6996
throw 0 < count(\$allow) ? new MethodNotAllowedException(array_unique(\$allow)) : new ResourceNotFoundException();
7097
}
71-
7298
EOF;
7399
}
74100

101+
/**
102+
* Counts the number of routes as direct child of the RouteCollection.
103+
*
104+
* @param RouteCollection $routes A RouteCollection instance
105+
*
106+
* @return integer Number of Routes
107+
*/
108+
private function countDirectChildRoutes(RouteCollection $routes)
109+
{
110+
$count = 0;
111+
foreach ($routes as $route) {
112+
if ($route instanceof Route) {
113+
$count++;
114+
}
115+
}
116+
117+
return $count;
118+
}
119+
120+
/**
121+
* Generates PHP code recursively to match a RouteCollection with all child routes and child collections.
122+
*
123+
* @param RouteCollection $routes A RouteCollection instance
124+
* @param Boolean $supportsRedirections Whether redirections are supported by the base class
125+
* @param string|null $parentPrefix The prefix of the parent collection used to optimize the code
126+
*
127+
* @return string PHP code
128+
*/
75129
private function compileRoutes(RouteCollection $routes, $supportsRedirections, $parentPrefix = null)
76130
{
77131
$code = '';
78132

79-
$routeIterator = $routes->getIterator();
80-
$keys = array_keys($routeIterator->getArrayCopy());
81-
$keysCount = count($keys);
82-
83-
$i = 0;
84-
foreach ($routeIterator as $name => $route) {
85-
$i++;
86-
87-
if ($route instanceof RouteCollection) {
88-
$prefix = $route->getPrefix();
89-
$optimizable = $prefix && count($route->all()) > 1 && false === strpos($route->getPrefix(), '{');
90-
$optimized = false;
91-
92-
if ($optimizable) {
93-
for ($j = $i; $j < $keysCount; $j++) {
94-
if (null === $keys[$j]) {
95-
continue;
96-
}
97-
98-
$testRoute = $routeIterator->offsetGet($keys[$j]);
99-
$isCollection = ($testRoute instanceof RouteCollection);
100-
101-
$testPrefix = $isCollection ? $testRoute->getPrefix() : $testRoute->getPattern();
102-
103-
if (0 === strpos($testPrefix, $prefix)) {
104-
$routeIterator->offsetUnset($keys[$j]);
105-
106-
if ($isCollection) {
107-
$route->addCollection($testRoute);
108-
} else {
109-
$route->add($keys[$j], $testRoute);
110-
}
111-
112-
$i++;
113-
$keys[$j] = null;
114-
}
115-
}
116-
117-
if ($prefix !== $parentPrefix) {
118-
$code .= sprintf(" if (0 === strpos(\$pathinfo, %s)) {\n", var_export($prefix, true));
119-
$optimized = true;
120-
}
121-
}
122-
123-
if ($optimized) {
124-
foreach (explode("\n", $this->compileRoutes($route, $supportsRedirections, $prefix)) as $line) {
125-
if ('' !== $line) {
126-
$code .= ' '; // apply extra indention
127-
}
128-
$code .= $line."\n";
129-
}
130-
$code = substr($code, 0, -2); // remove redundant last two line breaks
131-
$code .= " }\n\n";
132-
} else {
133-
$code .= $this->compileRoutes($route, $supportsRedirections, $prefix);
134-
}
135-
} else {
136-
$code .= $this->compileRoute($route, $name, $supportsRedirections, $parentPrefix)."\n";
133+
$prefix = $routes->getPrefix();
134+
$countDirectChildRoutes = $this->countDirectChildRoutes($routes);
135+
$countAllChildRoutes = count($routes->all());
136+
// Can the matching be optimized by wrapping it with the prefix condition
137+
// - no need to optimize if current prefix is the same as the parent prefix
138+
// - if $countDirectChildRoutes === 0, the sub-collections can do their own optimizations (in case there are any)
139+
// - it's not worth wrapping a single child route
140+
// - prefixes with variables cannot be optimized because routes within the collection might have different requirements for the same variable
141+
$optimizable = '' !== $prefix && $prefix !== $parentPrefix && $countDirectChildRoutes > 0 && $countAllChildRoutes > 1 && false === strpos($prefix, '{');
142+
if ($optimizable) {
143+
$code .= sprintf(" if (0 === strpos(\$pathinfo, %s)) {\n", var_export($prefix, true));
144+
}
145+
146+
foreach ($routes as $name => $route) {
147+
if ($route instanceof Route) {
148+
// a single route in a sub-collection is not wrapped so it should do its own optimization in ->compileRoute with $parentPrefix = null
149+
$code .= $this->compileRoute($route, $name, $supportsRedirections, 1 === $countAllChildRoutes ? null : $prefix)."\n";
150+
} elseif ($countAllChildRoutes - $countDirectChildRoutes > 0) { // we can stop iterating recursively if we already know there are no more routes
151+
$code .= $this->compileRoutes($route, $supportsRedirections, $prefix);
137152
}
138153
}
139154

155+
if ($optimizable) {
156+
$code .= " }\n\n";
157+
// apply extra indention at each line (except empty ones)
158+
$code = preg_replace('/^.{2,}$/m', ' $0', $code);
159+
}
160+
140161
return $code;
141162
}
142163

164+
165+
/**
166+
* Compiles a single Route to PHP code used to match it against the path info.
167+
*
168+
* @param Route $routes A Route instance
169+
* @param string $name The name of the Route
170+
* @param Boolean $supportsRedirections Whether redirections are supported by the base class
171+
* @param string|null $parentPrefix The prefix of the parent collection used to optimize the code
172+
*
173+
* @return string PHP code
174+
*/
143175
private function compileRoute(Route $route, $name, $supportsRedirections, $parentPrefix = null)
144176
{
145177
$code = '';
@@ -167,7 +199,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
167199
$conditions[] = sprintf("\$pathinfo === %s", var_export(str_replace('\\', '', $m['url']), true));
168200
}
169201
} else {
170-
if ($compiledRoute->getStaticPrefix() && $compiledRoute->getStaticPrefix() != $parentPrefix) {
202+
if ($compiledRoute->getStaticPrefix() && $compiledRoute->getStaticPrefix() !== $parentPrefix) {
171203
$conditions[] = sprintf("0 === strpos(\$pathinfo, %s)", var_export($compiledRoute->getStaticPrefix(), true));
172204
}
173205

@@ -254,47 +286,4 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
254286

255287
return $code;
256288
}
257-
258-
private function startClass($class, $baseClass)
259-
{
260-
return <<<EOF
261-
<?php
262-
263-
use Symfony\Component\Routing\Exception\MethodNotAllowedException;
264-
use Symfony\Component\Routing\Exception\ResourceNotFoundException;
265-
use Symfony\Component\Routing\RequestContext;
266-
267-
/**
268-
* $class
269-
*
270-
* This class has been auto-generated
271-
* by the Symfony Routing Component.
272-
*/
273-
class $class extends $baseClass
274-
{
275-
276-
EOF;
277-
}
278-
279-
private function addConstructor()
280-
{
281-
return <<<EOF
282-
/**
283-
* Constructor.
284-
*/
285-
public function __construct(RequestContext \$context)
286-
{
287-
\$this->context = \$context;
288-
}
289-
290-
EOF;
291-
}
292-
293-
private function endClass()
294-
{
295-
return <<<EOF
296-
}
297-
298-
EOF;
299-
}
300289
}

‎src/Symfony/Component/Routing/Route.php

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -79,10 +79,12 @@ public function setPattern($pattern)
7979
$this->pattern = trim($pattern);
8080

8181
// a route must start with a slash
82-
if (empty($this->pattern) || '/' !== $this->pattern[0]) {
82+
if ('' === $this->pattern || '/' !== $this->pattern[0]) {
8383
$this->pattern = '/'.$this->pattern;
 10000
8484
}
8585

86+
$this->compiled = null;
87+
8688
return $this;
8789
}
8890

@@ -128,6 +130,7 @@ public function addOptions(array $options)
128130
foreach ($options as $name => $option) {
129131
$this->options[(string) $name] = $option;
130132
}
133+
$this->compiled = null;
131134

132135
return $this;
133136
}
@@ -147,6 +150,7 @@ public function addOptions(array $options)
147150
public function setOption($name, $value)
148151
{
149152
$this->options[$name] = $value;
153+
$this->compiled = null;
150154

151155
return $this;
152156
}
@@ -203,6 +207,7 @@ public function addDefaults(array $defaults)
203207
foreach ($defaults as $name => $default) {
204208
$this->defaults[(string) $name] = $default;
205209
}
210+
$this->compiled = null;
206211

207212
return $this;
208213
}
@@ -244,6 +249,7 @@ public function hasDefault($name)
244249
public function setDefault($name, $default)
245250
{
246251
$this->defaults[(string) $name] = $default;
252+
$this->compiled = null;
247253

248254
return $this;
249255
}
@@ -288,6 +294,7 @@ public function addRequirements(array $requirements)
288294
foreach ($requirements as $key => $regex) {
289295
$this->requirements[$key] = $this->sanitizeRequirement($key, $regex);
290296
}
297+
$this->compiled = null;
291298

292299
return $this;
293300
}
@@ -317,6 +324,7 @@ public function getRequirement($key)
317324
public function setRequirement($key, $regex)
318325
{
319326
$this->requirements[$key] = $this->sanitizeRequirement($key, $regex);
327+
$this->compiled = null;
320328

321329
return $this;
322330
}
@@ -343,15 +351,19 @@ public function compile()
343351

344352
private function sanitizeRequirement($key, $regex)
345353
{
346-
if (is_array($regex)) {
347-
throw new \InvalidArgumentException(sprintf('Routing requirements must be a string, array given for "%s"', $key));
354+
if (!is_string($regex)) {
355+
throw new \InvalidArgumentException(sprintf('Routing requirement for "%s" must be a string', $key));
356+
}
357+
358+
if ('' === $regex) {
359+
throw new \InvalidArgumentException(sprintf('Routing requirement for "%s" cannot be empty', $key));
348360
}
349361

350-
if ('^' == $regex[0]) {
362+
if ('^' === $regex[0]) {
351363
$regex = substr($regex, 1);
352364
}
353365

354-
if ('$' == substr($regex, -1)) {
366+
if ('$' === substr($regex, -1)) {
355367
$regex = substr($regex, 0, -1);
356368
}
357369

0 commit comments

Comments
 (0)
0