Skip to content

Commit

Permalink
bug #21333 [HttpKernel] Fix ArgumentValueResolver for arguments defau…
Browse files Browse the repository at this point in the history
…lt null (chalasr)

This PR was merged into the 2.7 branch.

Discussion
----------

[HttpKernel] Fix ArgumentValueResolver for arguments default null

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

If an argument has `null` as default value __and annotations are used for routing__, then it is not resolved by the ArgumentResolver e.g:

```php

public function showAction(Request $request) {
   dump($request); // Request object
}

public function showAction(?Request $request) {
   dump($request); // Request object
}

public function showAction(Request $request = null) {
   dump($request); // null
}
```
To me, the last example should have been a Request object too.

Note that it is the same behavior when using `UserInterface` or whatever, I ran into this issue while trying to dump a user for a route accepting both anonymous and authenticated requests, the argument was always null while `$this->getUser()` returned the expected value. According to http://symfony.com/blog/new-in-symfony-3-2-user-value-resolver-for-controllers it should have worked through the argument.

I propose to make it works as a bugfix.
Any suggestion for improving the patch will be really welcomed. ping @iltar

Commits
-------

d3fa8a1 Avoid setting request attributes from signature arguments in AnnotationClassLoader
  • Loading branch information
fabpot committed Jan 19, 2017
2 parents 5ba84ca + d3fa8a1 commit 77289b9
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 9 deletions.
Expand Up @@ -138,11 +138,6 @@ protected function addRoute(RouteCollection $collection, $annot, $globals, \Refl
}

$defaults = array_replace($globals['defaults'], $annot->getDefaults());
foreach ($method->getParameters() as $param) {
if (!isset($defaults[$param->getName()]) && $param->isDefaultValueAvailable()) {
$defaults[$param->getName()] = $param->getDefaultValue();
}
}
$requirements = array_replace($globals['requirements'], $annot->getRequirements());
$options = array_replace($globals['options'], $annot->getOptions());
$schemes = array_merge($globals['schemes'], $annot->getSchemes());
Expand Down
Expand Up @@ -136,11 +136,10 @@ public function testLoad($className, $routeData = array(), $methodArgs = array()
array_intersect_assoc($routeData['options'], $route->getOptions()),
'->load preserves options annotation'
);
$defaults = array_replace($methodArgs, $routeData['defaults']);
$this->assertCount(
count($defaults),
array_intersect_assoc($defaults, $route->getDefaults()),
'->load preserves defaults annotation and merges them with default arguments in method signature'
count($routeData['defaults']),
$route->getDefaults(),
'->load preserves defaults annotation'
);
$this->assertEquals($routeData['schemes'], $route->getSchemes(), '->load preserves schemes annotation');
$this->assertEquals($routeData['methods'], $route->getMethods(), '->load preserves methods annotation');
Expand Down

5 comments on commit 77289b9

@stasicki
Copy link

Choose a reason for hiding this comment

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

This break compatibility with php default values.
http://symfony.com/doc/current/routing.html#giving-placeholders-a-default-value
For example:

    /**
     * @param string $type
     * @return Response
     * @Route("/index/{type}", name="orders.index")
     */
    public function indexAction($type = null)
    {
        ...
    }

then $this->get('router')->generate('orders.index');
will throw Symfony\Component\Routing\Exception\MissingMandatoryParametersException
in Symfony/Component/Routing/Generator/UrlGenerator.php#151

@linaori
Copy link
Contributor

@linaori linaori commented on 77289b9 Jan 23, 2017

Choose a reason for hiding this comment

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

That's because the defaults is missing from your route definition (you have to add this to any other config as well afaik).

Edit: interestingly enough, this "feature" was relying on a buggy behavior

@stasicki
Copy link

Choose a reason for hiding this comment

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

Look at the documentation link that I gave you. There was no need to set _defautls if was defined in function definition.

@linaori
Copy link
Contributor

Choose a reason for hiding this comment

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

The buggy behavior was that for (only) annotations, empty keys would be added in the request attributes. For any other configuration method, you still have to add the defaults option.

@chalasr
Copy link
Member

Choose a reason for hiding this comment

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

Indeed, this was supposed to work but was inconsistent with other configuration formats, and was causing a very blocking bug. I'll send a PR to mention the BC break in UPGRADe files and another one to update the doc. Thanks for reporting!

Please sign in to comment.