8000 [Routing] fixed several bugs and applied improvements by Tobion · Pull Request #3754 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Routing] fixed several bugs and applied improvements #3754

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 7 commits into from
Closed

[Routing] fixed several bugs and applied improvements #3754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
30839fb
fix bugs and improvements in RouteCollection
Tobion Apr 1, 2012
55b4482
fixed bugs and added tests for Routing
Tobion Apr 4, 2012
d4a5f9f
fixed CS and minor tweaks
Tobion Apr 5, 2012
46434e0
fixed dumping of a root collection with prefix
Tobion Apr 10, 2012
a6b8d54
fixed CS and added notes in changelog
Tobion Apr 10, 2012
a4a3f44
fix that Route::compile needs to recompile once route is modified
Tobion Apr 10, 2012
d4f63d3
apply required changes caused by upstream
Tobion Apr 11, 2012
File filter

Filter by extension

Filter by e 8000 xtension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions CHANGELOG-2.1.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,9 +363,11 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c
* added a TraceableUrlMatcher
* added the possibility to define options, default values and requirements for placeholders in prefix, including imported routes
* added RouterInterface::getRouteCollection
* [BC BREAK] the UrlMatcher urldecodes the route parameters only once, they were
decoded twice before. Note that the `urldecode()` calls have been change for a
single `rawurldecode()` in order to support `+` for input paths.
* [BC BREAK] the UrlMatcher urldecodes the route parameters only once, they were decoded twice before.
Note that the `urldecode()` calls have been changed for a single `rawurldecode()` in order to support `+` for input paths.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed typo there: changed
@stof I rebased. Btw, how do I rebase and resolve conflict without using the --force option when pushing? Without it, it gets rejected becaues of non-fast-forward updates.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tobion when you rebase, you need to use --force when pushing as rebasing rewrites the history.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. The problem is github puts all commits at the end after that in Tobion added some commits section.
So conversation history is not that easy to follow.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference there is also another syntax git push tobion +branch_name which also forces the write.

* added RouteCollection::getRoot method to retrieve the root of a RouteCollection tree
* [BC BREAK] made RouteCollection::setParent private which could not have been used anyway without creating inconsistencies
* [BC BREAK] RouteCollection::remove also removes a route from parent collections (not only from its children)

### Security

Expand Down
214 changes: 101 additions & 113 deletions src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php
View file
Open in desktop
1E79
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
* PhpMatcherDumper creates a PHP class able to match URLs for a given set of routes.
*
* @author Fabien Potencier <fabien@symfony.com>
* @author Tobias Schultze <http://tobion.de>
*/
class PhpMatcherDumper extends MatcherDumper
{
Expand All @@ -44,21 +45,47 @@ public function dump(array $options = array())
$interfaces = class_implements($options['base_class']);
$supportsRedirections = isset($interfaces['Symfony\Component\Routing\Matcher\RedirectableUrlMatcherInterface']);

return
$this->startClass($options['class'], $options['base_class']).
$this->addConstructor().
$this->addMatcher($supportsRedirections).
$this->endClass()
;
return <<<EOF
<?php

use Symfony\Component\Routing\Exception\MethodNotAllowedException;
use Symfony\Component\Routing\Exception\ResourceNotFoundException;
use Symfony\Component\Routing\RequestContext;

/**
* {$options['class']}
*
* This class has been auto-generated
* by the Symfony Routing Component.
*/
class {$options['class']} extends {$options['base_class']}
{
/**
* Constructor.
*/
public function __construct(RequestContext \$context)
{
\$this->context = \$context;
}

{$this->generateMatchMethod($supportsRedirections)}
}

EOF;
}

private function addMatcher($supportsRedirections)
/**
* Generates the code for the match method implementing UrlMatcherInterface.
*
* @param Boolean $supportsRedirections Whether redirections are supported by the base class
*
* @return string Match method as PHP code
*/
private function generateMatchMethod($supportsRedirections)
{
// we need to deep clone the routes as we will modify the structure to optimize the dump
$code = rtrim($this->compileRoutes(clone $this->getRoutes(), $supportsRedirections), "\n");
$code = rtrim($this->compileRoutes($this->getRoutes(), $supportsRedirections), "\n");

return <<<EOF

public function match(\$pathinfo)
{
\$allow = array();
Expand All @@ -68,78 +95,82 @@ public function match(\$pathinfo)

throw 0 < count(\$allow) ? new MethodNotAllowedException(array_unique(\$allow)) : new ResourceNotFoundException();
}

EOF;
}

/**
* Counts the number of routes as direct child of the RouteCollection.
*
* @param RouteCollection $routes A RouteCollection instance
*
* @return integer Number of Routes
*/
private function countDirectChildRoutes(RouteCollection $routes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really need an arg here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean? how else should it work?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake (think we were in RouteCollection here)

{
$count = 0;
foreach ($routes as $route) {
if ($route instanceof Route) {
$count++;
}
}

return $count;
}

/**
* Generates PHP code recursively to match a RouteCollection with all child routes and child collections.
*
* @param RouteCollection $routes A RouteCollection instance
* @param Boolean $supportsRedirections Whether redirections are supported by the base class
* @param string|null $parentPrefix The prefix of the parent collection used to optimize the code
*
* @return string PHP code
*/
private function compileRoutes(RouteCollection $routes, $supportsRedirections, $parentPrefix = null)
{
$code = '';

$routeIterator = $routes->getIterator();
$keys = array_keys($routeIterator->getArrayCopy());
$keysCount = count($keys);

$i = 0;
foreach ($routeIterator as $name => $route) {
$i++;

if ($route instanceof RouteCollection) {
$prefix = $route->getPrefix();
$optimizable = $prefix && count($route->all()) > 1 && false === strpos($route->getPrefix(), '{');
$optimized = false;

if ($optimizable) {
for ($j = $i; $j < $keysCount; $j++) {
if (null === $keys[$j]) {
continue;
}

$testRoute = $routeIterator->offsetGet($keys[$j]);
$isCollection = ($testRoute instanceof RouteCollection);

$testPrefix = $isCollection ? $testRoute->getPrefix() : $testRoute->getPattern();

if (0 === strpos($testPrefix, $prefix)) {
$routeIterator->offsetUnset($keys[$j]);

if ($isCollection) {
$route->addCollection($testRoute);
} else {
$route->add($keys[$j], $testRoute);
}

$i++;
$keys[$j] = null;
}
}

if ($prefix !== $parentPrefix) {
$code .= sprintf(" if (0 === strpos(\$pathinfo, %s)) {\n", var_export($prefix, true));
$optimized = true;
}
}

if ($optimized) {
foreach (explode("\n", $this->compileRoutes($route, $supportsRedirections, $prefix)) as $line) {
if ('' !== $line) {
$code .= ' '; // apply extra indention
}
$code .= $line."\n";
}
$code = substr($code, 0, -2); // remove redundant last two line breaks
$code .= " }\n\n";
} else {
$code .= $this->compileRoutes($route, $supportsRedirections, $prefix);
}
} else {
$code .= $this->compileRoute($route, $name, $supportsRedirections, $parentPrefix)."\n";
$prefix = $routes->getPrefix();
$countDirectChildRoutes = $this->countDirectChildRoutes($routes);
$countAllChildRoutes = count($routes->all());
$optimizable = '' !== $prefix && $prefix !== $parentPrefix && $countDirectChildRoutes > 0 && $countAllChildRoutes > 1 && false === strpos($prefix, '{');
// whether the matching of routes within the collection can be optimized by wrapping it with the prefix condition
// - no need to optimize if current prefix is the same as the parent prefix
// - if $countDirectChildRoutes === 0, the sub-collections can do their own optimizations (in case there are any)
// - it's not worth wrapping a single child route
// - prefixes with variables cannot be optimized because routes within the collection might have different requirements for the same variable
if ($optimizable) {
$code .= sprintf(" if (0 === strpos(\$pathinfo, %s)) {\n", var_export($prefix, true));
}

foreach ($routes as $name => $route) {
if ($route instanceof Route) {
$code .= $this->compileRoute($route, $name, $supportsRedirections, 1 === $countAllChildRoutes ? null : $prefix)."\n";
// a single route in a sub-collection is not wrapped so it should do its own optimization in ->compileRoute with $parentPrefix = null
} elseif ($countAllChildRoutes - $countDirectChildRoutes > 0) { // we can stop iterating recursively if we already know there are no more routes
$code .= $this->compileRoutes($route, $supportsRedirections, $prefix);
}
}

if ($optimizable) {
$code .= " }\n\n";
$code = preg_replace('/^.{2,}$/m', ' $0', $code); // apply extra indention at each line (except empty ones)
}

return $code;
}


/**
* Compiles a single Route to PHP code used to match it against the path info.
*
* @param Route $routes A Route instance
* @param string $name The name of the Route
* @param Boolean $supportsRedirections Whether redirections are supported by the base class
* @param string|null $parentPrefix The prefix of the parent collection used to optimize the code
*
* @return string PHP code
*/
private function compileRoute(Route $route, $name, $supportsRedirections, $parentPrefix = null)
{
$code = '';
Expand Down Expand Up @@ -167,7 +198,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
$conditions[] = sprintf("\$pathinfo === %s", var_export(str_replace('\\', '', $m['url']), true));
}
} else {
if ($compiledRoute->getStaticPrefix() && $compiledRoute->getStaticPrefix() != $parentPrefix) {
if ($compiledRoute->getStaticPrefix() && $compiledRoute->getStaticPrefix() !== $parentPrefix) {
$conditions[] = sprintf("0 === strpos(\$pathinfo, %s)", var_export($compiledRoute->getStaticPrefix(), true));
}

Expand Down Expand Up @@ -254,47 +285,4 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren

return $code;
}

private function startClass($class, $baseClass)
{
return <<<EOF
<?php

use Symfony\Component\Routing\Exception\MethodNotAllowedException;
use Symfony\Component\Routing\Exception\ResourceNotFoundException;
use Symfony\Component\Routing\RequestContext;

/**
* $class
*
* This class has been auto-generated
* by the Symfony Routing Component.
*/
class $class extends $baseClass
{

EOF;
}

private function addConstructor()
{
return <<<EOF
/**
* Constructor.
*/
public function __construct(RequestContext \$context)
{
\$this->context = \$context;
}

EOF;
}

private function endClass()
{
return <<<EOF
}

EOF;
}
}
22 changes: 17 additions & 5 deletions src/Symfony/Component/Routing/Route.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,12 @@ public function setPattern($pattern)
$this->pattern = trim($pattern);

// a route must start with a slash
if (empty($this->pattern) || '/' !== $this->pattern[0]) {
if ('' === $this->pattern || '/' !== $this->pattern[0]) {
$this->pattern = '/'.$this->pattern;
}

$this->compiled = null;

return $this;
}

Expand Down Expand Up @@ -128,6 +130,7 @@ public function addOptions(array $options)
foreach ($options as $name => $option) {
$this->options[(string) $name] = $option;
}
$this->compiled = null;

return $this;
}
Expand All @@ -147,6 +150,7 @@ public function addOptions(array $options)
public function setOption($name, $value)
{
$this->options[$name] = $value;
$this->compiled = null;

return $this;
}
Expand Down Expand Up @@ -203,6 +207,7 @@ public function addDefaults(array $defaults)
foreach ($defaults as $name => $default) {
$this->defaults[(string) $name] = $default;
}
$this->compiled = null;

return $this;
}
Expand Down Expand Up @@ -244,6 +249,7 @@ public function hasDefault($name)
public function setDefault($name, $default)
{
$this->defaults[(string) $name] = $default;
$this->compiled = null;

return $this;
}
Expand Down Expand Up @@ -288,6 +294,7 @@ public function addRequirements(array $requirements)
foreach ($requirements as $key => $regex) {
$this->requirements[$key] = $this->sanitizeRequirement($key, $regex);
}
$this->compiled = null;

return $this;
}
Expand Down Expand Up @@ -317,6 +324,7 @@ public function getRequirement($key)
public function setRequirement($key, $regex)
{
$this->requirements[$key] = $this->sanitizeRequirement($key, $regex);
$this->compiled = null;

return $this;
}
Expand All @@ -343,15 +351,19 @@ public function compile()

private function sanitizeRequirement($key, $regex)
{
if (is_array($regex)) {
throw new \InvalidArgumentException(sprintf('Routing requirements must be a string, array given for "%s"', $key));
if (!is_string($regex)) {
throw new \InvalidArgumentException(sprintf('Routing requirement for "%s" must be a string', $key));
}

if ('' === $regex) {
throw new \InvalidArgumentException(sprintf('Routing requirement for "%s" cannot be empty', $key));
}

if ('^' == $regex[0]) {
if ('^' === $regex[0]) {
$regex = substr($regex, 1);
}

if ('$' == substr($regex, -1)) {
if ('$' === substr($regex, -1)) {
$regex = substr($regex, 0, -1);
}

Expand Down
Loading
0