Permalink
Browse files

merged branch arnaud-lb/routing-php-dumper-simplification (PR #5734)

This PR was merged into the master branch.

Commits
-------

e54d749 [Routing] Simplified php matcher dumper (and optimized generated matcher)

Discussion
----------

[Routing] Simplified php matcher dumper (and optimized generated matcher)

Bug fix: no
Feature addition: no
Related: #3378
Backwards compatibility break: no
Symfony2 tests pass: yes

This simplifies the php matcher dumper by allowing the dumper to re-organize routes in the dumper's own structure.

As a result, dumping is made a little simpler. This is also helps much for my hostname-based routes PR #3378.

Reorganizing routes also allows to find more optimization opportunities:

Currently the dumper wraps some collections of routes in a `if (0 === strpos($pathinfo, '/someprefix')` if the collection has user-defined prefix, and if it contains more than one direct child Route. This can miss many optimization opportunities.

The PR changes this by building a prefix tree of routes based on the static prefix extracted from routes' patterns. Then every leave having a prefix and more than one child (route or collection) will be wrapped in a `if` statement.

Example:

```
// No explicit prefix is specified
@Route("/cafe")
@Route("/cacao")
@Route("/coca")
```

is compiled like this:

```
if (url starts with /c) {
    if (url starts with /ca) {
        // test route "/cafe"
        // test route "/cacao"
    }
    // test route "/coca"
}
```

Some tests have many white space changes, much more easier to review [here](https://github.com/symfony/symfony/pull/5734/files?w=1)

---------------------------------------------------------------------------

by Tobion at 2012-10-13T02:27:54Z

I'm not sure if adding these specific classes just for dumping is the best implementation because they duplicate some logic and this optimization should also work out-of-the-box with the standard RouteCollection etc.
What I have in mind is a new method in RouteCollection like `RouteCollection::optimizeTree` that returns a new RouteCollection with the Routes that includes these optimization you do here. So there would probably be no need for the new classes.

It think it requires some changes in RouteCollection like the handling of prefix that must start with a slash currently, which is too restrictive. But it should be possible.

---------------------------------------------------------------------------

by arnaud-lb at 2012-10-13T12:52:32Z

@Tobion

> I'm not sure if adding these specific classes just for dumping is the best implementation because they duplicate some logic and this optimization should also work out-of-the-box with the standard RouteCollection etc.

I think RouteCollection and DumperCollection do not share the same concerns; and RouteCollection does things that don't allow to reorganize routes freely. For instance when adding a collection to a RouteCollection this changes all the child routes' prefix, requirements, options, etc. When setting a collection's prefix, this prepends the prefix to every child route's pattern, etc.

---------------------------------------------------------------------------

by arnaud-lb at 2012-10-15T08:50:23Z

squashed the CS commits

---------------------------------------------------------------------------

by arnaud-lb at 2012-10-15T13:50:16Z

@fabpot @Tobion this PR is ready to be merged if everyone agrees

---------------------------------------------------------------------------

by Tobion at 2012-10-16T18:10:36Z

When the above is fixed, I think it's good to be merged.

---------------------------------------------------------------------------

by arnaud-lb at 2012-10-17T08:40:20Z

Fixed; thanks @Tobion @stof for your reviews

---------------------------------------------------------------------------

by Tobion at 2012-10-19T03:30:10Z

@arnaud-lb could you please test whether your PR fixes #5780 as a side-effect?
I can image that it's fixed because you use `$route->compile()->getStaticPrefix();` for prefix optimization.
If it's fixed please add a test case. If not, that's fine, and we need to fix it in another PR.
  • Loading branch information...
fabpot committed Oct 19, 2012
2 parents 88ea842 + e54d749 commit 388cbff022fb117880a1705f524174d943d0ccdf
@@ -0,0 +1,101 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Routing\Matcher\Dumper;
+
+/**
+ * Collection of routes.
+ *
+ * @author Arnaud Le Blanc <arnaud.lb@gmail.com>
+ */
+class DumperCollection implements \IteratorAggregate
+{
+ private $parent;
+ private $children = array();
+
+ /**
+ * Returns the children routes and collections.
+ *
+ * @return array Array of DumperCollection|DumperRoute
+ */
+ public function all()
+ {
+ return $this->children;
+ }
+
+ /**
+ * Adds a route or collection
+ *
+ * @param DumperRoute|DumperCollection The route or collection
+ */
+ public function add($child)
+ {
+ if ($child instanceof DumperCollection) {
+ $child->setParent($this);
+ }
+ $this->children[] = $child;
+ }
+
+ /**
+ * Sets children
+ *
+ * @param array $children The children
+ */
+ public function setAll(array $children)
+ {
+ foreach ($children as $child) {
+ if ($child instanceof DumperCollection) {
+ $child->setParent($this);
+ }
+ }
+ $this->children = $children;
+ }
+
+ /**
+ * Returns an iterator over the children.
+ *
+ * @return \Iterator The iterator
+ */
+ public function getIterator()
+ {
+ return new \ArrayIterator($this->children);
+ }
+
+ /**
+ * Returns the root of the collection.
+ *
+ * @return DumperCollection The root collection
+ */
+ public function getRoot()
+ {
+ return (null !== $this->parent) ? $this->parent->getRoot() : $this;
+ }
+
+ /**
+ * Returns the parent collection.
+ *
+ * @return DumperCollection|null The parent collection or null if the collection has no parent
+ */
+ protected function getParent()
+ {
+ return $this->parent;
+ }
+
+ /**
+ * Sets the parent collection.
+ *
+ * @param DumperCollection $parent The parent collection
+ */
+ protected function setParent(DumperCollection $parent)
+ {
+ $this->parent = $parent;
+ }
+}
@@ -0,0 +1,103 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Routing\Matcher\Dumper;
+
+/**
+ * Prefix tree of routes preserving routes order.
+ *
+ * @author Arnaud Le Blanc <arnaud.lb@gmail.com>
+ */
+class DumperPrefixCollection extends DumperCollection
+{
+ private $prefix = '';
+
+ /**
+ * Returns the prefix.
+ *
+ * @return string The prefix
+ */
+ public function getPrefix()
+ {
+ return $this->prefix;
+ }
+
+ /**
+ * Sets the prefix.
+ *
+ * @param string $prefix The prefix
+ */
+ public function setPrefix($prefix)
+ {
+ $this->prefix = $prefix;
+ }
+
+ /**
+ * Adds a route in the tree.
+ *
+ * @param DumperRoute $route The route
+ *
+ * @return DumperPrefixCollection The node the route was added to
+ */
+ public function addPrefixRoute(DumperRoute $route)
+ {
+ $prefix = $route->getRoute()->compile()->getStaticPrefix();
+
+ // Same prefix, add to current leave
+ if ($this->prefix === $prefix) {
+ $this->add($route);
+
+ return $this;
+ }
+
+ // Prefix starts with route's prefix
+ if ('' === $this->prefix || 0 === strpos($prefix, $this->prefix)) {
+ $collection = new DumperPrefixCollection();
+ $collection->setPrefix(substr($prefix, 0, strlen($this->prefix)+1));
+ $this->add($collection);
+
+ return $collection->addPrefixRoute($route);
+ }
+
+ // No match, fallback to parent (recursively)
+
+ if (null === $parent = $this->getParent()) {
+ throw new \LogicException("The collection root must not have a prefix");
+ }
+
+ return $parent->addPrefixRoute($route);
+ }
+
+ /**
+ * Merges nodes whose prefix ends with a slash
+ *
+ * Children of a node whose prefix ends with a slash are moved to the parent node
+ */
+ public function mergeSlashNodes()
+ {
+ $children = array();
+
+ foreach ($this as $child) {
+ if ($child instanceof self) {
+ $child->mergeSlashNodes();
+ if ('/' === substr($child->prefix, -1)) {
+ $children = array_merge($children, $child->all());
+ } else {
+ $children[] = $child;
+ }
+ } else {
+ $children[] = $child;
+ }
+ }
+
+ $this->setAll($children);
+ }
+}
@@ -0,0 +1,58 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Routing\Matcher\Dumper;
+
+use Symfony\Component\Routing\Route;
+use Symfony\Component\Routing\RouteCollection;
+
+/**
+ * Container for a Route.
+ *
+ * @author Arnaud Le Blanc <arnaud.lb@gmail.com>
+ */
+class DumperRoute
+{
+ private $name;
+ private $route;
+
+ /**
+ * Constructor.
+ *
+ * @param string $name The route name
+ * @param Route $route The route
+ */
+ public function __construct($name, Route $route)
+ {
+ $this->name = $name;
+ $this->route = $route;
+ }
+
+ /**
+ * Returns the route name.
+ *
+ * @return string The route name
+ */
+ public function getName()
+ {
+ return $this->name;
+ }
+
+ /**
+ * Returns the route.
+ *
+ * @return Route The route
+ */
+ public function getRoute()
+ {
+ return $this->route;
+ }
+}
Oops, something went wrong.

0 comments on commit 388cbff

Please sign in to comment.