Skip to content
Browse files

Refactored the RouterVoter to make it more flexible

The way to pass routes in the item extras has changed.

before:

    'routes' => array('foo', 'bar'),
    'routeParameters' => array('foo' => array('id' => 4))

after:

    'routes' => array(
        array('route' => 'foo', 'parameters' => array('id' => 4)),
	'bar',
    )

This new syntax allows using the same route with multiple parameter sets
for the same item, or matching the route for any parameter while using
some parameters to generate the url in the RouterAwareFactory by adding it
explicitly without parameters.

The old syntax is deprecated, and using it will trigger a
E_USER_DEPRECATED error.

Parameters are now checked against the _route_params attribute (introduced
in Symfony 2.1) instead of the attributes themselves, to avoid issues with
type casting when using param converters to replace the attributes by
objects. Closes #92 and replaces #93.
  • Loading branch information...
1 parent 323c0e6 commit cc20a1e67394a93d8559fc87af86db7eaf2fbf56 @stof stof committed Jun 23, 2013
View
2 phpunit.xml.dist
@@ -1,6 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>
-<phpunit colors="false" bootstrap="tests/bootstrap.php">
+<phpunit colors="true" bootstrap="tests/bootstrap.php">
<testsuites>
<testsuite name="KnpMenu Test Suite">
<directory suffix="Test.php">./tests/</directory>
View
8 src/Knp/Menu/Silex/RouterAwareFactory.php
@@ -25,10 +25,10 @@ protected function buildOptions(array $options = array())
$options['uri'] = $this->generator->generate($options['route'], $params, $absolute);
// adding the item route to the extras under the 'routes' key (for the Silex RouteVoter)
- $options = array_merge_recursive(array('extras' => array(
- 'routes' => array($options['route']),
- 'routesParameters' => array($options['route']=>$params),
- )), $options);
+ $options['extras']['routes'][] = array(
+ 'route' => $options['route'],
+ 'parameters' => $params,
+ );
}
return parent::buildOptions($options);
View
56 src/Knp/Menu/Silex/Voter/RouteVoter.php
@@ -34,22 +34,60 @@ public function matchItem(ItemInterface $item)
$routes = (array) $item->getExtra('routes', array());
$parameters = (array) $item->getExtra('routesParameters', array());
+
foreach ($routes as $testedRoute) {
- if ($route !== $testedRoute) {
- continue;
- }
+ if (is_string($testedRoute)) {
+ $testedRoute = array('route' => $testedRoute);
- if (isset($parameters[$route])) {
- foreach ($parameters[$route] as $name => $value) {
- if ($this->request->attributes->get($name) != $value) {
- return null;
- }
+ // BC layer for the configuration of route params
+ if (isset($parameters[$testedRoute['route']])) {
+ $testedRoute['parameters'] = $parameters[$testedRoute['route']];
+ trigger_error(
+ sprintf(
+ 'Using the routeParameters extra is deprecated. The parameters should be passed along the route in %s',
+ $item->getPathAsString()
+ ),
+ E_USER_DEPRECATED
+ );
}
}
- return true;
+ if (!is_array($testedRoute)) {
+ throw new \InvalidArgumentException('Routes extra items must be strings or arrays.');
+ }
+
+ if ($this->isMatchingRoute($testedRoute)) {
+ return true;
+ }
}
return null;
}
+
+ private function isMatchingRoute(array $testedRoute)
+ {
+ $route = $this->request->attributes->get('_route');
+
+ if (!isset($testedRoute['route'])) {
+ throw new \InvalidArgumentException('Routes extra items must have "route" key.');
+ }
+
+ if ($route !== $testedRoute['route']) {
+ return false;
+ }
+
+ if (!isset($testedRoute['parameters'])) {
+ return true;
+ }
+
+ $routeParameters = $this->request->attributes->get('_route_params', array());
+
+ foreach ($testedRoute['parameters'] as $name => $value) {
+ if (!isset($routeParameters[$name]) || $routeParameters[$name] !== (string) $value) {
+ return false;
+ }
+ }
+
+ return true;
+ }
}
View
4 tests/Knp/Menu/Tests/Silex/RouterAwareFactoryTest.php
@@ -58,9 +58,9 @@ public function testCreateItemAppendsRouteUnderExtras()
$factory = new RouterAwareFactory($generator);
$item = $factory->createItem('test_item', array('route' => 'homepage'));
- $this->assertEquals(array('homepage'), $item->getExtra('routes'));
+ $this->assertEquals(array(array('route' => 'homepage', 'parameters' => array())), $item->getExtra('routes'));
$item = $factory->createItem('test_item', array('route' => 'homepage', 'extras' => array('routes' => array('other_page'))));
- $this->assertContains('homepage', $item->getExtra('routes'));
+ $this->assertContains(array('route' => 'homepage', 'parameters' => array()), $item->getExtra('routes'));
}
}
View
115 tests/Knp/Menu/Tests/Silex/Voter/RouteVoterTest.php
@@ -26,15 +26,44 @@ public function testMatchingWithoutRequest()
}
/**
+ * @expectedException \InvalidArgumentException
+ */
+ public function testInvalidRouteConfig()
+ {
+ $item = $this->getMock('Knp\Menu\ItemInterface');
+ $item->expects($this->any())
+ ->method('getExtra')
+ ->with($this->logicalOr($this->equalTo('routes'), $this->equalTo('routesParameters')))
+ ->will($this->returnCallback(function ($parameter) {
+ switch ($parameter) {
+ case 'routes':
+ return array(array('invalid' => 'array'));
+ case 'routesParameters':
+ return array();
+ }
+ }));
+
+ $request = new Request();
+ $request->attributes->set('_route', 'foo');
+ $request->attributes->set('_route_params', array());
+
+ $voter = new RouteVoter();
+ $voter->setRequest($request);
+
+ $voter->matchItem($item);
+ }
+
+ /**
* @param string $route
* @param array $parameters
* @param string|array $itemRoutes
* @param array $itemsRoutesParameters
* @param boolean $expected
+ * @param boolean $deprecatedCall
*
* @dataProvider provideData
*/
- public function testMatching($route, array $parameters, $itemRoutes, array $itemsRoutesParameters, $expected)
+ public function testMatching($route, array $parameters, $itemRoutes, array $itemsRoutesParameters, $expected, $deprecatedCall = false)
{
$item = $this->getMock('Knp\Menu\ItemInterface');
$item->expects($this->any())
@@ -51,14 +80,30 @@ public function testMatching($route, array $parameters, $itemRoutes, array $item
$request = new Request();
$request->attributes->set('_route', $route);
- foreach ($parameters as $name => $value) {
- $request->attributes->set($name, $value);
- }
+ $request->attributes->set('_route_params', $parameters);
$voter = new RouteVoter();
$voter->setRequest($request);
- $this->assertSame($expected, $voter->matchItem($item));
+ $deprecatedErrorCatched = false;
+ set_error_handler(function ($errorNumber, $message, $file, $line) use (&$deprecatedErrorCatched) {
+ if ($errorNumber & E_USER_DEPRECATED) {
+ $deprecatedErrorCatched = true;
+ return true;
+ }
+
+ return \PHPUnit_Util_ErrorHandler::handleError($errorNumber, $message, $file, $line);
+ });
+
+ try {
+ $this->assertSame($expected, $voter->matchItem($item));
+ } catch (\Exception $e) {
+ restore_error_handler();
+ throw $e;
+ }
+
+ restore_error_handler();
+ $this->assertSame($deprecatedCall, $deprecatedErrorCatched);
}
public function provideData()
@@ -68,41 +113,75 @@ public function provideData()
'no item route' => array('foo', array(), null, array(), null),
'same single route' => array('foo', array(), 'foo', array(), true),
'different single route' => array('foo', array(), 'bar', array(), null),
- 'matching mutiple routes' => array('foo', array(), array('foo', 'baz'), array(), true),
- 'matching mutiple routes 2' => array('baz', array(), array('foo', 'baz'), array(), true),
+ 'matching multiple routes' => array('foo', array(), array('foo', 'baz'), array(), true),
+ 'matching multiple routes 2' => array('baz', array(), array('foo', 'baz'), array(), true),
'different multiple routes' => array('foo', array(), array('bar', 'baz'), array(), null),
'same single route with different parameters' => array(
'foo', array('1' => 'bar'),
- 'foo', array('foo' => array('1' => 'baz')),
+ array(array('route' => 'foo', 'parameters' => array('1' => 'baz'))), array(),
null
),
'same single route with same parameters' => array(
'foo', array('1' => 'bar'),
- 'foo', array('foo' => array('1' => 'bar')),
+ array(array('route' => 'foo', 'parameters' => array('1' => 'bar'))), array(),
true
),
'same single route with additional parameters' => array(
'foo', array('1' => 'bar'),
- 'foo', array('foo' => array('1' => 'bar', '2' => 'baz')),
+ array(array('route' => 'foo', 'parameters' => array('1' => 'bar', '2' >+ 'baz'))), array(),
null
),
'same single route with less parameters' => array(
'foo', array('1' => 'bar', '2' => 'baz'),
- 'foo', array('foo' => array('1' => 'bar')),
+ array(array('route' => 'foo', 'parameters' => array('1' => 'bar'))), array(),
true
),
- 'same single route with same type parameters' => array(
- 'foo', array('1' => 2),
- 'foo', array('foo' => array('1' => 2)),
+ 'same single route with different type parameters' => array(
+ 'foo', array('1' => '2'),
+ array(array('route' => 'foo', 'parameters' => array('1' => 2))), array(),
true
),
- 'same single route with different type parameters' => array(
- 'foo', array('1' => 2),
- 'foo', array('foo' => array('1' => '2')),
+ 'same route with multiple route params' => array(
+ 'foo', array('1' => 'bar'),
+ array(
+ array('route' => 'foo', 'parameters' => array('1' => 'baz')),
+ array('route' => 'foo', 'parameters' => array('1' => 'bar')),
+ ),
+ array(),
true
),
-
+ 'same route with and without route params' => array(
+ 'foo', array('1' => 'bar'),
+ array(
+ array('route' => 'foo', 'parameters' => array('1' => 'baz')),
+ array('route' => 'foo'),
+ ),
+ array(),
+ true
+ ),
+ 'same route with multiple different route params' => array(
+ 'foo', array('1' => 'bar'),
+ array(
+ array('route' => 'foo', 'parameters' => array('1' => 'baz')),
+ array('route' => 'foo', 'parameters' => array('1' => 'foo')),
+ ),
+ array(),
+ null
+ ),
+ // Test the BC layer for the old way to pass parameters
+ 'same single route with different parameters deprecated way' => array(
+ 'foo', array('1' => 'bar'),
+ 'foo', array('foo' => array('1' => 'baz')),
+ null,
+ true,
+ ),
+ 'same single route with same parameters deprecated way' => array(
+ 'foo', array('1' => 'bar'),
+ 'foo', array('foo' => array('1' => 'bar')),
+ true,
+ true,
+ ),
);
}
}

0 comments on commit cc20a1e

Please sign in to comment.
Something went wrong with that request. Please try again.