8000 [Routing] Implement bug fixes and enhancements · symfony/symfony@9307f5b · GitHub
[go: up one dir, main page]
Skip to content

Commit 9307f5b

Browse files
Tobionvicb
Tobion
authored and
vicb
committed
[Routing] Implement bug fixes and enhancements
1 parent 3c3ec5c commit 9307f5b

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+
* @author 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;
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