8000 merged branch Tobion/collection-flat (PR #6120) · symfony/symfony@344496f · GitHub
[go: up one dir, main page]
Skip to content

Commit 344496f

Browse files
fabpotfabpot
committed
merged branch Tobion/collection-flat (PR #6120)
This PR was merged into the master branch. Commits ------- 51223c0 added upgrade instructions 50e6259 adjusted tests 98f3ca8 [Routing] removed tree structure from RouteCollection Discussion ---------- [Routing] removed tree structure from RouteCollection BC break: yes (see below) Deprecations: RouteCollection::getParent(); RouteCollection::getRoot() tests pass: yes The reason for this is so quite simple. The RouteCollection has been designed as a tree structure, but it cannot at all be used as one. There is no getter for a sub-collection at all. So you cannot access a sub-collection after you added it to the tree with `addCollection(new RouteCollection())`. In contrast to the form component, e.g. `$form->get('child')->get('grandchild')`. So you can see the RouteCollection cannot be used as a tree and it should not, as the same can be achieved with a flat array! Using a flat array removes all the need for recursive traversal and makes the code much faster, much lighter, less memory (big problem in CMS with many routes) and less error-prone. BC break: there is only a BC break if somebody used the PHP API for defining RouteCollection and also added a Route to a collection after it has been added to another collection. So ``` $rootCollection = new RouteCollection(); $subCollection = new RouteCollection(); $rootCollection->addCollection($subCollection); $subCollection->add('foo', new Route('/foo')); ``` must be updated to the following (otherwise the 'foo' Route is not imported to the rootCollection) ``` $rootCollection = new RouteCollection(); $subCollection = new RouteCollection(); $subCollection->add('foo', new Route('/foo')); $rootCollection->addCollection($subCollection); ``` Also one must call addCollection from the bottom to the top. So the correct sequence is the following (and not the reverse) ``` $childCollection->->addCollection($grandchildCollection); $rootCollection->addCollection($childCollection); ``` Remeber, this is only needed when using PHP for defining routes and calling methods in a special order. There is no change required when using XML or YAML for definitions. Also, I'm pretty sure that neither the CMF, nor Drupal routing, nor Silex is relying on the tree stuff. So they should also still work. cc @fabpot @Crell @dbu One more thing: RouteCollection wasn't an appropriate name for a tree anyway as a collection of routes (that it now is) is definitely not a tree. Yet another point: The XML declaration of routes uses the `<import>` element, which is excatly what the new implementation of addCollection without the need of a tree does. So this is now also more analogous. --------------------------------------------------------------------------- by Koc at 2012-11-26T17:34:15Z What benefit of this? --------------------------------------------------------------------------- by Tobion at 2012-11-26T17:56:53Z @Koc Why did you not simply wait for the description? ^^ --------------------------------------------------------------------------- by dbu at 2012-11-26T18:33:09Z i love PR that remove more code than they add whithout removing functionality. --------------------------------------------------------------------------- by Crell at 2012-11-26T18:49:52Z There's an issue somewhere in Drupal where we're trying to use addCollection() as a shorthand for iterating over one collection and calling add() on the other for each item. We can't do that, however, because the subcollections are not flattened properly when reading back and our current dumper can't cope with that. So this change would not harm Drupal at all, and would mean I don't have fix a bug in our dumper. :-) I cannot speak for any other projects, of course. --------------------------------------------------------------------------- by Tobion at 2012-11-27T19:06:34Z Ok, this is ready.
2 parents e925978 + 51223c0 commit 344496f

File tree

9 files changed

+142
-266
lines changed

9 files changed

+142
-266
lines changed

‎UPGRADE-2.2.md

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,42 @@
3939

4040
* The PasswordType is now not trimmed by default.
4141

42+
### Routing
43+
44+
* RouteCollection does not behave like a tree structure anymore but as a flat
45+
array of Routes. So when using PHP to build the RouteCollection, you must
46+
make sure to add routes to the sub-collection before adding it to the parent
47+
collection (this is not relevant when using YAML or XML for Route definitions).
48+
49+
Before:
50+
51+
```
52+
$rootCollection = new RouteCollection();
53+
$subCollection = new RouteCollection();
54+
$rootCollection->addCollection($subCollection);
55+
$subCollection->add('foo', new Route('/foo'));
56+
```
57+
58+
After:
59+
60+
```
61+
$rootCollection = new RouteCollection();
62+
$subCollection = new RouteCollection();
63+
$subCollection->add('foo', new Route('/foo'));
64+
$rootCollection->addCollection($subCollection);
65+
```
66+
67+
Also one must call `addCollection` from the bottom to the top hierarchy.
68+
So the correct sequence is the following (and not the reverse):
69+
70+
```
71+
$childCollection->->addCollection($grandchildCollection);
72+
$rootCollection->addCollection($childCollection);
73+
```
74+
75+
* The methods `RouteCollection::getParent()` and `RouteCollection::getRoot()`
76+
have been deprecated and will be removed in Symfony 2.3.
77+
4278
### Validator
4379

4480
* Interfaces were created for the classes `ConstraintViolation`,

‎src/Symfony/Bundle/FrameworkBundle/Routing/Router.php

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -85,20 +85,16 @@ public function warmUp($cacheDir)
8585
private function resolveParameters(RouteCollection $collection)
8686
{
8787
foreach ($collection as $route) {
88-
if ($route instanceof RouteCollection) {
89-
$this->resolveParameters($route);
90-
} else {
91-
foreach ($route->getDefaults() as $name => $value) {
92-
$route->setDefault($name, $this->resolve($value));
93-
}
94-
95-
foreach ($route->getRequirements() as $name => $value) {
96-
$route->setRequirement($name, $this->resolve($value));
97-
}
98-
99-
$route->setPattern($this->resolve($route->getPattern()));
100-
$route->setHostnamePattern($this->resolve($route->getHostnamePattern()));
88+
foreach ($route->getDefaults() as $name => $value) {
89+
$route->setDefault($name, $this->resolve($value));
10190
}
91+
92+
foreach ($route->getRequirements() as $name => $value) {
93+
$route->setRequirement($name, $this->resolve($value));
94+
}
95+
96+
$route->setPattern($this->resolve($route->getPattern()));
97+
$route->setHostnamePattern($this->resolve($route->getHostnamePattern()));
10298
}
10399
}
104100

‎src/Symfony/Component/Routing/CHANGELOG.md

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,39 @@ CHANGELOG
44
2.2.0
55
-----
66

7+
* [BC BREAK] RouteCollection does not behave like a tree structure anymore but as
8+
a flat array of Routes. So when using PHP to build the RouteCollection, you must
9+
make sure to add routes to the sub-collection before adding it to the parent
10+
collection (this is not relevant when using YAML or XML for Route definitions).
11+
12+
Before:
13+
14+
```
15+
$rootCollection = new RouteCollection();
16+
$subCollection = new RouteCollection();
17+
$rootCollection->addCollection($subCollection);
18+
$subCollection->add('foo', new Route('/foo'));
19+
```
20+
21+
After:
22+
23+
```
24+
$rootCollection = new RouteCollection();
25+
$subCollection = new RouteCollection();
26+
$subCollection->add('foo', new Route('/foo'));
27+
$rootCollection->addCollection($subCollection);
28+
```
29+
30+
Also one must call `addCollection` from the bottom to the top hierarchy.
31+
So the correct sequence is the following (and not the reverse):
32+
33+
```
34+
$childCollection->->addCollection($grandchildCollection);
35+
$rootCollection->addCollection($childCollection);
36+
```
37+
38+
* The methods `RouteCollection::getParent()` and `RouteCollection::getRoot()`
39+
have been deprecated and will be removed in Symfony 2.3.
740
* added support for the method default argument values when defining a @Route
841
* Adjacent placeholders without separator work now, e.g. `/{x}{y}{z}.{_format}`.
942
* Characters that function as separator between placeholders are now whitelisted

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

Lines changed: 4 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,6 @@ private function compileRoutes(RouteCollection $routes, $supportsRedirections)
111111
{
112112
$fetchedHostname = false;
113113

114-
$routes = $this->flattenRouteCollection($routes);
115114
$groups = $this->groupRoutesByHostnameRegex($routes);
116115
$code = '';
117116

@@ -321,31 +320,6 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
321320
return $code;
322321
}
323322

324-
/**
325-
* Flattens a tree of routes to a single collection.
326-
*
327-
* @param RouteCollection $routes Collection of routes
328-
* @param DumperCollection|null $to A DumperCollection to add routes to
329-
*
330-
* @return DumperCollection
331-
*/
332-
private function flattenRouteCollection(RouteCollection $routes, DumperCollection $to = null)
333-
{
334-
if (null === $to) {
335-
$to = new DumperCollection();
336-
}
337-
338-
foreach ($routes as $name => $route) {
339-
if ($route instanceof RouteCollection) {
340-
$this->flattenRouteCollection($route, $to);
341-
} else {
342-
$to->add(new DumperRoute($name, $route));
343-
}
344-
}
345-
346-
return $to;
347-
}
348-
349323
/**
350324
* Groups consecutive routes having the same hostname regex.
351325
*
@@ -355,22 +329,22 @@ private function flattenRouteCollection(RouteCollection $routes, DumperCollectio
355329
*
356330
* @return DumperCollection A collection with routes grouped by hostname regex in sub-collections
357331
*/
358-
private function groupRoutesByHostnameRegex(DumperCollection $routes)
332+
private function groupRoutesByHostnameRegex(RouteCollection $routes)
359333
{
360334
$groups = new DumperCollection();
361335

362336
$currentGroup = new DumperCollection();
363337
$currentGroup->setAttribute('hostname_regex', null);
364338
$groups->add($currentGroup);
365339

366-
foreach ($routes as $route) {
367-
$hostnameRegex = $route->getRoute()->compile()->getHostnameRegex();
340+
foreach ($routes as $name => $route) {
341+
$hostnameRegex = $route->compile()->getHostnameRegex();
368342
if ($currentGroup->getAttribute('hostname_regex') !== $hostnameRegex) {
369343
$currentGroup = new DumperCollection();
370344
$currentGroup->setAttribute('hostname_regex', $hostnameRegex);
371345
$groups->add($currentGroup);
372346
}
373-
$currentGroup->add($route);
347+
$currentGroup->add(new DumperRoute($name, $route));
374348
}
375349

376350
return $groups;

‎src/Symfony/Component/Routing/Matcher/TraceableUrlMatcher.php

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -44,14 +44,6 @@ public function getTraces($pathinfo)
4444
protected function matchCollection($pathinfo, RouteCollection $routes)
4545
{
4646
foreach ($routes as $name => $route) {
47-
if ($route instanceof RouteCollection) {
48-
if (!$ret = $this->matchCollection($pathinfo, $route)) {
49-
continue;
50-
}
51-
52-
return true;
53-
}
54-
5547
$compiledRoute = $route->compile();
5648

5749
if (!preg_match($compiledRoute->getRegex(), $pathinfo, $matches)) {

‎src/Symfony/Component/Routing/Matcher/UrlMatcher.php

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -105,18 +105,6 @@ public function match($pathinfo)
105105
protected function matchCollection($pathinfo, RouteCollection $routes)
106106
{
107107
foreach ($routes as $name => $route) {
108-
if ($route instanceof RouteCollection) {
109-
if (false === strpos($route->getPrefix(), '{') && $route->getPrefix() !== substr($pathinfo, 0, strlen($route->getPrefix()))) {
110-
continue;
111-
}
112-
113-
if (!$ret = $this->matchCollection($pathinfo, $route)) {
114-
continue;
115-
}
116-
117-
return $ret;
118-
}
119-
120108
$compiledRoute = $route->compile();
121109

122110
// check the static prefix of the URL first. Only use the more expensive preg_match when it matches

0 commit comments

Comments
 (0)
0