Skip to content

[Routing] Add support for 'priority' option for annotated Routes #11753

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 4 commits into from
Closed

[Routing] Add support for 'priority' option for annotated Routes #11753

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1ddfd1b
Implement priority routes for annotated controller actions
Aug 24, 2014
fe825f3
fix typo and coding standard error
Aug 24, 2014
3804ba9
Put routes with priority in the front
Aug 25, 2014
992e386
add support for routes with negative priority value
Aug 26, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/Symfony/Component/Routing/Annotation/Route.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,11 @@ public function getOptions()
return $this->options;
}

public function getOption($name)
{
return isset($this->options[$name]) ? $this->options[$name] : null;
}

public function setDefaults($defaults)
{
$this->defaults = $defaults;
Expand Down
29 changes: 28 additions & 1 deletion src/Symfony/Component/Routing/Loader/AnnotationClassLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ abstract class AnnotationClassLoader implements LoaderInterface
*/
protected $defaultRouteIndex = 0;

/**
* @var array
*/
protected $routesWithPriority = array();

/**
* Constructor.
*
Expand Down Expand Up @@ -122,11 +127,33 @@ public function load($class, $type = null)
$this->defaultRouteIndex = 0;
foreach ($this->reader->getMethodAnnotations($method) as $annot) {
if ($annot instanceof $this->routeAnnotationClass) {
$this->addRoute($collection, $annot, $globals, $class, $method);
$priority = (int) $annot->getOption('priority');
$this->routesWithPriority[$priority][] = array($annot, $globals, $class, $method);
}
}
}

$collection = $this->addPriorityRoutes($collection);

Copy link
Member

Choose a reason for hiding this comment

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

you must reset the state of the loader here

Copy link
Author

Choose a reason for hiding this comment

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

I looked for a way for that, but at the moment RouteCollection don't have such function, and if it's called in a directoryLoader, the named route will be removed and always appended to the end of the array, here in priority order. Or what do you mean by reseting?

Copy link
Member

Choose a reason for hiding this comment

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

$routesWithPriority is making the loader stateful. This means that if you import the routes from several classes explicitly, the second import will still see the priority routes of the first import, potentially creating weird issues

Copy link
Author

Choose a reason for hiding this comment

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

Oh, I see what you mean. The original goal was that if you import the routes for the Controller directory, you can set up priority between the files.

Copy link
Member

Choose a reason for hiding this comment

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

see my comment in the main discussion. This "feature" makes it very easy to break lots of things

return $collection;
}

/**
* Add the prioritised routes to the collection
*
* @param $collection
* @return RouteCollection
*/
public function addPriorityRoutes($collection)
{
krsort($this->routesWithPriority);

foreach ($this->routesWithPriority as $priorityGroup) {
foreach ($priorityGroup as $route) {
$this->addRoute($collection, $route[0], $route[1], $route[2], $route[3]);
Copy link
Member

Choose a reason for hiding this comment

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

Route already added at https://github.com/symfony/symfony/pull/11753/files#diff-90cbecfd4505e6e8af5842db11add3b5R139

Copy link
Author

Choose a reason for hiding this comment

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

Yes, here I had 2 option, one is when there is a priority key, I say continue, and I will not add the route initially there, and only here, but with this, if I add it again, routeCollection will remove the first addition and add the new one to the end of the list.

}
}

return $collection;
}

Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Component/Routing/Loader/AnnotationDirectoryLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ public function load($path, $type = null)
}
}

$collection = $this->loader->addPriorityRoutes($collection);

return $collection;
}

Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Component/Routing/Loader/AnnotationFileLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ public function load($file, $type = null)
$collection->addCollection($this->loader->load($class, $type));
}

$collection = $this->loader->addPriorityRoutes($collection);

return $collection;
}

Expand Down
7 changes: 6 additions & 1 deletion src/Symfony/Component/Routing/Tests/Loader/AnnotationClassLoaderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,11 @@ public function getLoadTests()
array('name' => 'route1', 'defaults' => array('arg2' => 'foo'), 'condition' => 'context.getMethod() == "GET"'),
array('arg2' => 'defaultValue2', 'arg3' =>'defaultValue3')
),
array(
'Symfony\Component\Routing\Tests\Fixtures\AnnotatedClasses\BarClass',
array('name' => 'route1', 'options' => array('priority' => 10)),
array('arg2' => 'defaultValue2', 'arg3' => 'defaultValue3'),
),
);
}

Expand Down Expand Up @@ -120,7 +125,7 @@ public function testLoad($className, $routeDatas = array(), $methodArgs = array(

$this->assertSame($routeDatas['path'], $route->getPath(), '->load preserves path annotation');
$this->assertSame($routeDatas['requirements'],$route->getRequirements(), '->load preserves requirements annotation');
$this->assertCount(0, array_intersect($route->getOptions(), $routeDatas['options']), '->load preserves options annotation');
$this->assertSame(count($routeDatas['options']), count(array_intersect($route->getOptions(), $routeDatas['options'])), '->load preserves options annotation');
$this->assertSame(array_replace($methodArgs, $routeDatas['defaults']), $route->getDefaults(), '->load preserves defaults annotation');
$this->assertEquals($routeDatas['condition'], $route->getCondition(), '->load preserves condition annotation');
}
Expand Down
24 changes: 24 additions & 0 deletions src/Symfony/Component/Routing/Tests/Loader/AnnotationDirectoryLoaderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use Symfony\Component\Routing\Loader\AnnotationDirectoryLoader;
use Symfony\Component\Config\FileLocator;
use Symfony\Component\Routing\Annotation\Route;

class AnnotationDirectoryLoaderTest extends AbstractAnnotationLoaderTest
{
Expand Down Expand Up @@ -50,4 +51,27 @@ public function testSupports()
$this->assertTrue($this->loader->supports($fixturesDir, 'annotation'), '->supports() checks the resource type if specified');
$this->assertFalse($this->loader->supports($fixturesDir, 'foo'), '->supports() checks the resource type if specified');
}

public function testRoutesWithPriority()
{
$routeDatas = array(
'foo' => new Route(array('name' => 'foo', 'path' => '/foo')),
'unimportant' => new Route(array('name' => 'unimportant', 'path' => '/unimportant', 'options' => array('priority' => -10))),
'bar' => new Route(array('name' => 'bar', 'path' => '/bar')),
'static_id' => new Route(array('name' => 'static_id', 'path' => '/static/{id}', 'options' => array('priority' => 1))),
'home' => new Route(array('name' => 'home', 'path' => '/home')),
'static_contact' => new Route(array('name' => 'static_contact', 'path' => '/static/contact', 'options' => array('priority' => 5))),
'login' => new Route(array('name' => 'login', 'path' => '/login')),
'static_location' => new Route(array('name' => 'static_location', 'path' => '/static/location', 'options' => array('priority' => 5))),
);

$this->reader
->expects($this->once())
->method('getMethodAnnotations')
->will($this->returnValue($routeDatas));

$routeCollection = $this->loader->load(__DIR__.'/../Fixtures/AnnotatedClasses');
$expectedOrder = array( 'static_contact', 'static_location', 'static_id', 'foo', 'bar', 'home', 'login', 'unimportant');
$this->assertEquals($expectedOrder, array_keys($routeCollection->all()));
}
}
23 changes: 23 additions & 0 deletions src/Symfony/Component/Routing/Tests/Loader/AnnotationFileLoaderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Component\Routing\Tests\Loader;

use Symfony\Component\Routing\Annotation\Route;
use Symfony\Component\Routing\Loader\AnnotationFileLoader;
use Symfony\Component\Config\FileLocator;

Expand Down Expand Up @@ -44,4 +45,26 @@ public function testSupports()
$this->assertTrue($this->loader->supports($fixture, 'annotation'), '->supports() checks the resource type if specified');
$this->assertFalse($this->loader->supports($fixture, 'foo'), '->supports() checks the resource type if specified');
}

public function testRoutesWithPriority()
{
$routeDatas = array(
'foo' => new Route(array('name' => 'foo', 'path' => '/foo')),
'bar' => new Route(array('name' => 'bar', 'path' => '/bar')),
'static_id' => new Route(array('name' => 'static_id', 'path' => '/static/{id}', 'options' => array('priority' => 1))),
'home' => new Route(array('name' => 'home', 'path' => '/home')),
'static_contact' => new Route(array('name' => 'static_contact', 'path' => '/static/contact', 'options' => array('priority' => 5))),
'login' => new Route(array('name' => 'login', 'path' => '/login')),
'static_location' => new Route(array('name' => 'static_location', 'path' => '/static/location', 'options' => array('priority' => 5))),
);

$this->reader
->expects($this->once())
->method('getMethodAnnotations')
->will($this->returnValue($routeDatas));

$routeCollection = $this->loader->load(__DIR__.'/../Fixtures/AnnotatedClasses/BarClass.php');
$expectedOrder = array( 'static_contact', 'static_location', 'static_id', 'foo', 'bar', 'home', 'login');
$this->assertEquals($expectedOrder, array_keys($routeCollection->all()));
}
}