merged branch lyrixx/routing-annot-default-values (PR #5904)
This PR was squashed before being merged into the master branch (closes #5904).
Commits
-------
84adcb1
[2.2][Routing] Added support for default attributes with default values of method params
Discussion
----------
[2.2][Routing] Added support for default attributes with default values of method params
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
With this patch, you can configure your default values likes this:
``` php
/**
* @Route("/hi/{name}", name="hi")
*/
public function hiAction($name = "Bob")
{
return new Response($name);
}
```
---------------------------------------------------------------------------
by Tobion at 2012-11-03T23:15:32Z
I'm unsure. How does one know if that param defines a default value or a requirement? It's too vague.
---------------------------------------------------------------------------
by lyrixx at 2012-11-03T23:35:27Z
It's only a default value, not a requirement.
It's just a shortcut to avoid `defaults={"name"="bob"}`
---------------------------------------------------------------------------
by Tobion at 2012-11-03T23:43:51Z
Yes, but its not clear. It could also be a shortcut to `requirements={"name"="bob"}`, which has totally different meaning. So it's not self-explanatory.
-1 for me.
---------------------------------------------------------------------------
by lyrixx at 2012-11-03T23:48:21Z
it is the default php behavior. It's a default value for a variable...
---------------------------------------------------------------------------
by stof at 2012-11-04T00:22:58Z
@Tobion using the default value of the method to set a requirement does not make any sense. I don't see why someone would expect this behavior
---------------------------------------------------------------------------
by fabpot at 2012-11-06T10:12:05Z
@lyrixx Can you add some unit tests?
---------------------------------------------------------------------------
by Tobion at 2012-11-06T10:28:42Z
Oh I misunderstood the PR. I thought this makes the `name` param default to `hi`. `@Route("/hi/{name}", name="hi")`. But it's just the name of the route. Your example was easy to misinterpret as you used `name` everywhere.
---------------------------------------------------------------------------
by fabpot at 2012-11-10T08:33:13Z
@lyrixx Can you finish this PR?
---------------------------------------------------------------------------
by lyrixx at 2012-11-10T13:16:34Z
@fabpot Yes i will as soon as possible.
---------------------------------------------------------------------------
by lyrixx at 2012-11-10T18:34:07Z
I rebase and amend my commit. (I changed doc in commit message to be less confusing)
I will try to add tests.
But for now, `AnnotationClassLoader::load` is not really tested, and `AnnotationClassLoader::addRoute` is absolutely not tested. So I think I should add tests for these methods ? And then add tests for my patch.
I will try tomorrow.
---------------------------------------------------------------------------
by lyrixx at 2012-11-11T18:23:41Z
@fabpot I added new tests. I tried to made very atomic commits.
This commit is contained in:
commit
b0994603bb
@ -149,6 +149,11 @@ abstract class AnnotationClassLoader implements LoaderInterface
|
|||||||
}
|
}
|
||||||
|
|
||||||
$defaults = array_merge($globals['defaults'], $annot->getDefaults());
|
$defaults = array_merge($globals['defaults'], $annot->getDefaults());
|
||||||
|
foreach ($method->getParameters() as $param) {
|
||||||
|
if ($param->isOptional()) {
|
||||||
|
$defaults[$param->getName()] = $param->getDefaultValue();
|
||||||
|
}
|
||||||
|
}
|
||||||
$requirements = array_merge($globals['requirements'], $annot->getRequirements());
|
$requirements = array_merge($globals['requirements'], $annot->getRequirements());
|
||||||
$options = array_merge($globals['options'], $annot->getOptions());
|
$options = array_merge($globals['options'], $annot->getOptions());
|
||||||
|
|
||||||
|
@ -0,0 +1,19 @@
|
|||||||
|
<?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\Tests\Fixtures\AnnotatedClasses;
|
||||||
|
|
||||||
|
class BarClass
|
||||||
|
{
|
||||||
|
public function routeAction($arg1, $arg2 = 'defaultValue2', $arg3 = 'defaultValue3')
|
||||||
|
{
|
||||||
|
}
|
||||||
|
}
|
@ -21,7 +21,8 @@ class AnnotationClassLoaderTest extends AbstractAnnotationLoaderTest
|
|||||||
{
|
{
|
||||||
parent::setUp();
|
parent::setUp();
|
||||||
|
|
||||||
$this->loader = $this->getClassLoader($this->getReader());
|
$this->reader = $this->getReader();
|
||||||
|
$this->loader = $this->getClassLoader($this->reader);
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@ -71,4 +72,52 @@ class AnnotationClassLoaderTest extends AbstractAnnotationLoaderTest
|
|||||||
$this->assertFalse($this->loader->supports('class', 'foo'), '->supports() checks the resource type if specified');
|
$this->assertFalse($this->loader->supports('class', 'foo'), '->supports() checks the resource type if specified');
|
||||||
}
|
}
|
||||||
|
|
||||||
|
public function getLoadTests()
|
||||||
|
{
|
||||||
|
return array(
|
||||||
|
array(
|
||||||
|
'Symfony\Component\Routing\Tests\Fixtures\AnnotatedClasses\BarClass',
|
||||||
|
array('name'=>'route1'),
|
||||||
|
array('arg2' => 'defaultValue2', 'arg3' =>'defaultValue3')
|
||||||
|
),
|
||||||
|
array(
|
||||||
|
'Symfony\Component\Routing\Tests\Fixtures\AnnotatedClasses\BarClass',
|
||||||
|
array('name'=>'route1', 'defaults' => array('arg2' => 'foo')),
|
||||||
|
array('arg2' => 'defaultValue2', 'arg3' =>'defaultValue3')
|
||||||
|
),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* @dataProvider getLoadTests
|
||||||
|
*/
|
||||||
|
public function testLoad($className, $routeDatas = array(), $methodArgs = array())
|
||||||
|
{
|
||||||
|
$routeDatas = array_replace(array(
|
||||||
|
'name' => 'route',
|
||||||
|
'pattern' => '/',
|
||||||
|
'requirements' => array(),
|
||||||
|
'options' => array(),
|
||||||
|
'defaults' => array(),
|
||||||
|
), $routeDatas);
|
||||||
|
|
||||||
|
$this->reader
|
||||||
|
->expects($this->once())
|
||||||
|
->method('getMethodAnnotations')
|
||||||
|
->will($this->returnValue(array($this->getAnnotedRoute($routeDatas))))
|
||||||
|
;
|
||||||
|
$routeCollection = $this->loader->load($className);
|
||||||
|
$route = $routeCollection->get($routeDatas['name']);
|
||||||
|
|
||||||
|
$this->assertSame($routeDatas['pattern'], $route->getPattern(), '->load preserves pattern 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(array_replace($routeDatas['defaults'], $methodArgs), $route->getDefaults(), '->load preserves defaults annotation');
|
||||||
|
}
|
||||||
|
|
||||||
|
private function getAnnotedRoute($datas)
|
||||||
|
{
|
||||||
|
return new \Symfony\Component\Routing\Annotation\Route($datas);
|
||||||
|
}
|
||||||
|
|
||||||
}
|
}
|
||||||
|
@ -29,7 +29,13 @@ class AnnotationDirectoryLoaderTest extends AbstractAnnotationLoaderTest
|
|||||||
|
|
||||||
public function testLoad()
|
public function testLoad()
|
||||||
{
|
{
|
||||||
$this->reader->expects($this->once())->method('getClassAnnotation');
|
$this->reader->expects($this->exactly(2))->method('getClassAnnotation');
|
||||||
|
|
||||||
|
$this->reader
|
||||||
|
->expects($this->any())
|
||||||
|
->method('getMethodAnnotations')
|
||||||
|
->will($this->returnValue(array()))
|
||||||
|
;
|
||||||
|
|
||||||
$this->loader->load(__DIR__.'/../Fixtures/AnnotatedClasses');
|
$this->loader->load(__DIR__.'/../Fixtures/AnnotatedClasses');
|
||||||
}
|
}
|
||||||
|
Reference in New Issue
Block a user