Skip to content

Commit

Permalink
merged branch Tobion/strictrequirements2 (PR #5445)
Browse files Browse the repository at this point in the history 
This PR was merged into the master branch.

Commits
-------

e22823b [Routing] context params should have higher priority than defaults
16c1b01 [Routing] fixed 4 bugs in the UrlGenerator

Discussion
----------

[Routing] UrlGenerator: fixed missing query param and some ignored requirements

reopened version of #5181 (cherry-picked)

On top of that I fixed #5437 in my code and added test case.

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

by Tobion at 2012-10-03T18:41:45Z

@fabpot ping

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

by fabpot at 2012-10-03T18:43:43Z

IIUC, #5437 is a regression in 2.1 and should be done in 2.1, no?

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

by Tobion at 2012-10-03T18:46:42Z

It's not in 2.1 anymore as you reverted the PR.  #5437 is not valid currently and can be closed.
So this can either be merged in master or 2.1 whatever you prefer.
  • Loading branch information
@fabpot
fabpot committed Oct 3, 2012
2 parents c77d326 + e22823b commit 7276b2d
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 27 deletions.
42 changes: 18 additions & 24 deletions src/Symfony/Component/Routing/Generator/UrlGenerator.php
View file
Expand Up @@ -23,6 +23,7 @@
* UrlGenerator generates a URL based on a set of routes.
*
* @author Fabien Potencier <fabien@symfony.com>
* @author Tobias Schultze <http://tobion.de>
*
* @api
*/
Expand Down Expand Up @@ -132,44 +133,37 @@ public function generate($name, $parameters = array(), $absolute = false)
protected function doGenerate($variables, $defaults, $requirements, $tokens, $parameters, $name, $absolute)
{
$variables = array_flip($variables);

$originParameters = $parameters;
$parameters = array_replace($this->context->getParameters(), $parameters);
$tparams = array_replace($defaults, $parameters);
$mergedParams = array_replace($defaults, $this->context->getParameters(), $parameters);

// all params must be given
if ($diff = array_diff_key($variables, $tparams)) {
if ($diff = array_diff_key($variables, $mergedParams)) {
throw new MissingMandatoryParametersException(sprintf('The "%s" route has some missing mandatory parameters ("%s").', $name, implode('", "', array_keys($diff))));
}

$url = '';
$optional = true;
foreach ($tokens as $token) {
if ('variable' === $token[0]) {
if (false === $optional || !array_key_exists($token[3], $defaults) || (isset($parameters[$token[3]]) && (string) $parameters[$token[3]] != (string) $defaults[$token[3]])) {
if (!$isEmpty = in_array($tparams[$token[3]], array(null, '', false), true)) {
// check requirement
if ($tparams[$token[3]] && !preg_match('#^'.$token[2].'$#', $tparams[$token[3]])) {
$message = sprintf('Parameter "%s" for route "%s" must match "%s" ("%s" given).', $token[3], $name, $token[2], $tparams[$token[3]]);
if ($this->strictRequirements) {
throw new InvalidParameterException($message);
}

if ($this->logger) {
$this->logger->err($message);
}

return null;
if (!$optional || !array_key_exists($token[3], $defaults) || (string) $mergedParams[$token[3]] !== (string) $defaults[$token[3]]) {
// check requirement
if (!preg_match('#^'.$token[2].'$#', $mergedParams[$token[3]])) {
$message = sprintf('Parameter "%s" for route "%s" must match "%s" ("%s" given).', $token[3], $name, $token[2], $mergedParams[$token[3]]);
if ($this->strictRequirements) {
throw new InvalidParameterException($message);
}

if ($this->logger) {
$this->logger->err($message);
}
}

if (!$isEmpty || !$optional) {
$url = $token[1].$tparams[$token[3]].$url;
return null;
}

$url = $token[1].$mergedParams[$token[3]].$url;
$optional = false;
}
} elseif ('text' === $token[0]) {
} else {
// static text
$url = $token[1].$url;
$optional = false;
}
Expand All @@ -193,7 +187,7 @@ protected function doGenerate($variables, $defaults, $requirements, $tokens, $pa
}

// add a query string if needed
$extra = array_diff_key($originParameters, $variables, $defaults);
$extra = array_diff_key($parameters, $variables);
if ($extra && $query = http_build_query($extra, '', '&')) {
$url .= '?'.$query;
}
Expand Down
55 changes: 52 additions & 3 deletions src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php
View file
Expand Up @@ -74,12 +74,15 @@ public function testRelativeUrlWithNullParameter()
$this->assertEquals('/app.php/testing', $url);
}

/**
* @expectedException Symfony\Component\Routing\Exception\InvalidParameterException
*/
public function testRelativeUrlWithNullParameterButNotOptional()
{
$routes = $this->getRoutes('test', new Route('/testing/{foo}/bar', array('foo' => null)));
$url = $this->getGenerator($routes)->generate('test', array(), false);

$this->assertEquals('/app.php/testing//bar', $url);
// This must raise an exception because the default requirement for "foo" is "[^/]+" which is not met with these params.
// Generating path "/testing//bar" would be wrong as matching this route would fail.
$this->getGenerator($routes)->generate('test', array(), false);
}

public function testRelativeUrlWithOptionalZeroParameter()
Expand All @@ -90,6 +93,13 @@ public function testRelativeUrlWithOptionalZeroParameter()
$this->assertEquals('/app.php/testing/0', $url);
}

public function testNotPassedOptionalParameterInBetween()
{
$routes = $this->getRoutes('test', new Route('/{slug}/{page}', array('slug' => 'index', 'page' => 0)));
$this->assertSame('/app.php/index/1', $this->getGenerator($routes)->generate('test', array('page' => 1)));
$this->assertSame('/app.php/', $this->getGenerator($routes)->generate('test'));
}

public function testRelativeUrlWithExtraParameters()
{
$routes = $this->getRoutes('test', new Route('/testing'));
Expand Down Expand Up @@ -138,6 +148,18 @@ public function testUrlWithGlobalParameter()
$this->assertEquals('/app.php/testing/bar', $url);
}

public function testGlobalParameterHasHigherPriorityThanDefault()
{
$routes = $this->getRoutes('test', new Route('/{_locale}', array('_locale' => 'en')));
$generator = $this->getGenerator($routes);
$context = new RequestContext('/app.php');
$context->setParameter('_locale', 'de');
$generator->setContext($context);
$url = $generator->generate('test', array());

$this->assertSame('/app.php/de', $url);
}

/**
* @expectedException Symfony\Component\Routing\Exception\RouteNotFoundException
*/
Expand Down Expand Up @@ -165,6 +187,15 @@ public function testGenerateForRouteWithInvalidOptionalParameter()
$this->getGenerator($routes)->generate('test', array('foo' => 'bar'), true);
}

/**
* @expectedException Symfony\Component\Routing\Exception\InvalidParameterException
*/
public function testGenerateForRouteWithInvalidParameter()
{
$routes = $this->getRoutes('test', new Route('/testing/{foo}', array(), array('foo' => '1|2')));
$this->getGenerator($routes)->generate('test', array('foo' => '0'), true);
}

public function testGenerateForRouteWithInvalidOptionalParameterNonStrict()
{
$routes = $this->getRoutes('test', new Route('/testing/{foo}', array('foo' => '1'), array('foo' => 'd+')));
Expand Down Expand Up @@ -196,6 +227,15 @@ public function testGenerateForRouteWithInvalidMandatoryParameter()
$routes = $this->getRoutes('test', new Route('/testing/{foo}', array(), array('foo' => 'd+')));
$this->getGenerator($routes)->generate('test', array('foo' => 'bar'), true);
}

/**
* @expectedException Symfony\Component\Routing\Exception\InvalidParameterException
*/
public function testRequiredParamAndEmptyPassed()
{
$routes = $this->getRoutes('test', new Route('/{slug}', array(), array('slug' => '.+')));
$this->getGenerator($routes)->generate('test', array('slug' => ''));
}

public function testSchemeRequirementDoesNothingIfSameCurrentScheme()
{
Expand Down Expand Up @@ -229,6 +269,15 @@ public function testWithAnIntegerAsADefaultValue()
$this->assertEquals('/app.php/foo', $this->getGenerator($routes)->generate('test', array('default' => 'foo')));
}

public function testQueryParamSameAsDefault()
{
$routes = $this->getRoutes('test', new Route('/test', array('default' => 'value')));

$this->assertSame('/app.php/test?default=foo', $this->getGenerator($routes)->generate('test', array('default' => 'foo')));
$this->assertSame('/app.php/test?default=value', $this->getGenerator($routes)->generate('test', array('default' => 'value')));
$this->assertSame('/app.php/test', $this->getGenerator($routes)->generate('test'));
}

public function testUrlEncoding()
{
// This tests the encoding of reserved characters that are used for delimiting of URI components (defined in RFC 3986)
Expand Down

0 comments on commit 7276b2d

Please sign in to comment.