[HttpKernel] Fix ArgumentValueResolver for arguments default null #21333

Merged
merged 1 commit into from Jan 19, 2017

Projects

None yet

6 participants

@chalasr
Contributor
chalasr commented Jan 18, 2017 edited
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:


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

@chalasr
Contributor
chalasr commented Jan 18, 2017 edited

There is no BC break nor deprecation here, please remove the corresponding labels, my bad sorry.

@@ -58,7 +58,7 @@ public function createArgumentMetadata($controller)
}
foreach ($reflection->getParameters() as $param) {
- $arguments[] = new ArgumentMetadata($param->getName(), $this->getType($param), $this->isVariadic($param), $this->hasDefaultValue($param), $this->getDefaultValue($param), $param->allowsNull());
+ $arguments[] = new ArgumentMetadata($param->getName(), $this->getType($param), $this->isVariadic($param), $this->hasDefaultValue($param), $this->getDefaultValue($param), $this->isDefaultNull($param));
@chalasr
chalasr Jan 18, 2017 edited Contributor

ReflectionParameter::allowsNull() returns true for mandatory arguments which don't have any type hint i.e: function ($foo); // allowsNull(): true
Here we only want to know if the argument is default null, or has a typehint which allows null i.e. ?type

@iltar
Contributor
iltar commented Jan 18, 2017

How come it injects null instead of the Request? In theory the Request resolver should inject it as the default value one is triggered way later

@chalasr
Contributor
chalasr commented Jan 18, 2017 edited

Not sure but I believe the current implementation makes the Request resolver is just skipped so the default value wins. It can be reproduced with a fresh SE, changing the provided DefaultController::indexAction(Request $request) to DefaultController::indexAction(Request $request = null);

@iltar
Contributor
iltar commented Jan 18, 2017

But that would mean that this code is not hit properly: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestValueResolver.php#L30-L38

Can you confirm this? Because this code is responsible for setting the request if it's supported and should work with the original setup (in theory).

@chalasr
Contributor
chalasr commented Jan 18, 2017 edited

@iltar I can confirm this since using indexAction(Request $request = null) and adding a dump($request);die; immediately before this line https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestValueResolver.php#L30, the dump is never reached.

But right now I'm not able to say what exactly is wrong with the current implem, the patch made here avoids the only doubts I have about the origin of the issue (one being continue 2;) while covering all other cases that are working well originally, I was hoping you can say me more :/

@iltar
Contributor
iltar commented Jan 18, 2017

I'll try to reproduce it tomorrow. Maybe it's a sequence issue, because in theory this is only not called when something else already supports it and the continue 2; is hit. So if something else hits this first, there is probably another issue at hand.

@chalasr
Contributor
chalasr commented Jan 18, 2017 edited

Maybe it's a sequence issue

Right! I looked deeper into and fact is that the RequestAttributeValueResolver is the culprit.
I updated this PR to give it a lower priority than the RequestValueResolver/SecurityUserValueResolver, but maybe there's something wrong in the request argument one that should be fixed instead.
Waiting for your feedback.

@chalasr chalasr changed the title from [HttpKernel] Fix ArgumentResolver for argument with default null to [HttpKernel] Fix ArgumentValueResolver registration order Jan 18, 2017
@iltar
Contributor
iltar commented Jan 18, 2017 edited

@chalasr is there a specific reason that this line is returning true? https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestAttributeValueResolver.php#L30

Perhaps the 'request' key is created for some reason? Not sure why a null 'request' key would return true in a has here. Regarding your fix, if having the request in the attribute bag but as null is correct (which I kinda doubt), make sure the request attribute resolver is still called before the RequestValueResolver, due to the parameter converters

@chalasr
Contributor
chalasr commented Jan 18, 2017 edited

make sure the request attribute resolver is still called before the RequestValueResolver, due to the parameter converters

Did not considered that :/ then the current fix is not good, it just gives a lower priority to the request attribute resolver so it is called after the request/user resolver.

Perhaps the "request" key is created for some reason?

Right again! A request (variable name) key is put in the attribute bag with null as value. Same problem for the SecurityUserValueResolver given a user attribute with null as value exists, the RequestAttributeValueResolver takes precedence of course...

Not sure what's the proper way to fix this :/ Any idea @iltar?

@iltar
Contributor
iltar commented Jan 18, 2017

I think we have to check why the attributes are created but empty, perhaps this is done for the parameter converters? An additional check could be to check if it has() the key and it's not empty

@chalasr
Contributor
chalasr commented Jan 18, 2017 edited

perhaps this is done for the parameter converters?

Not sure but there're good chances. Note that these attributes stay null at the end (from the controller itself) since I do not have any param converter explicitly configured

make sure the request attribute resolver is still called before the RequestValueResolver, due to the parameter converters

Could you please develop?
How calling the request/user resolvers before the request attribute one would be a problem for param converters? I mean if one sets up an argument value resolver for a given argument, then he should not rely on a param converter to populate it, should he?

An additional check could be to check if it has() the key and it's not empty

If you mean adding an additional check inside RequestAttributeValueResolver::supports() I think it could help indeed, I'd say a strict null !== $attribute though

@iltar
Contributor
iltar commented Jan 18, 2017 edited

The PC attribute resolver should be called first because that's how the behavior was before the argument value resolvers were introduced. if attributes.foo existed, 'foo' should be inserted first rather than trigger the PCs (when passing in templates for example).

edit: Not PC but attribute resolver should be called first

@chalasr
Contributor
chalasr commented Jan 18, 2017 edited

I updated this for reverting the previous changes and only adding the additional null !== $attribute check instead, it fixes the issue.
It would be great if you could have a look at it and confirm it won't cause any issue with PCs.

@chalasr chalasr changed the title from [HttpKernel] Fix ArgumentValueResolver registration order to [HttpKernel] Fix ArgumentValueResolver for arguments default null Jan 18, 2017
+
+ $argumentName = $argument->getName();
+
+ return $request->attributes->has($argumentName) && null !== $request->attributes->get($argumentName);
@iltar
iltar Jan 18, 2017 edited Contributor

Hmm I'm curious what happens if you have this:

$request->attributes->set('required', null);
public function fooAction(string $required);

edit: probably already broken/proper error in an old version?

@chalasr
chalasr Jan 18, 2017 edited Contributor

Using:

/**
 * @Route("/", name="homepage", defaults={"required": null})
 */
public function indexAction(string $required)
{
}

Controller "AppBundle\Controller\DefaultController::indexAction()" requires that you provide a value for the "$required" argument [...]

Changing the signature to indexAction(string $required = null); or indexAction(?string $required); works of course.

Same behavior on 3.0, run into the same exception if the attribute is null.

@xabbuh
xabbuh Jan 18, 2017 Member

But this will now break the case where an attribute was set to null deliberately. Isn't that a BC break?

@chalasr
chalasr Jan 18, 2017 Contributor

Indeed, it is...

@iltar
iltar Jan 19, 2017 Contributor

With this signature it would fail already, you just simply can't set it to null anymore and it will trigger the resolver.

// call this with null, it will still trigger the resolver
public function fooAction(UserInterface $user = null);

But I'm still curious as of why the request values are null in the attribute bag.

@xabbuh regarding the above example, that would fail already. Also I don't think that values not mapped in the route should be affecting the parameter resolvers:

  • /{required} with (string $required) = allowed to be set to null but will throw an error as its required
  • / with (string $required) != allowed to set to null, as it was not added in the route definition
@iltar
iltar Jan 19, 2017 edited Contributor

After a bit of debugging, I found this in my url matcher cache:

if (rtrim($pathinfo, '/') === '') {
    if (substr($pathinfo, -1) !== '/') {
        return $this->redirect($pathinfo.'/', 'app.dashboard');
    }

   return array (  'user' => NULL,  '_controller' => 'app.controller.dashboard:dashboardAction',  '_route' => 'app.dashboard',);
}
dashboardAction(Client $client, Request $request, UserInterface $user = null)

It seems like the warmup of the cache adds a null value here, which is definitely not what should happen if it's not defined in the route configuration.

@chalasr
chalasr Jan 19, 2017 Contributor

@iltar it happens only when the argument has a default, I'll look if avoiding this breaks something but I think you're right. If we can remove this magic then the resolver stays as it is and continue to work as expected in case the attribute is set explicitly

@iltar
iltar Jan 19, 2017 Contributor

If you ask me, routing information should not be affected by arguments in the method that are not mapped in the route, but that's exactly what happens right now 😢

@xabbuh
xabbuh Jan 19, 2017 Member

@iltar No, I mean a parameter without a type hint and without a default value.

@chalasr
chalasr Jan 19, 2017 edited Contributor

@xabbuh it could even break for ones that are type-hinted as objects with null as default value I think, which is what we are trying to fix. The current approach is definitely not the good one.

routing information should not be affected by arguments in the method that are not mapped in the route

I agree, but I don't know if it is expected somewhere internally (if someone has an hint?), nor where it happens

@iltar
iltar Jan 19, 2017 Contributor

@xabbuh I agree on that point. If the compiled route would be without the default value, everything will work as expected

@iltar
iltar approved these changes Jan 18, 2017 View changes

Looks good to me!

@chalasr
Contributor
chalasr commented Jan 18, 2017

Added a test for.
@iltar Thank you for the time spent

@chalasr
Contributor
chalasr commented Jan 19, 2017

@iltar @xabbuh In fact the problem occurs only when using the @Route annotation, the routing AnnotationClassLoader merges the signature arguments (the ones with default null) to the route defaults. Since it is the only one to do so and I can't find a good reason for that, I removed this behavior and all works as expected.

@iltar
Contributor
iltar commented Jan 19, 2017

Are the other changes still required to fix the bug? I'm afraid they might cause a behavioral change as there's been done some tweaking regarding nullables

@chalasr
Contributor
chalasr commented Jan 19, 2017

@iltar My bad, I forgot to revert it. The diff is clean now

@iltar
Contributor
iltar commented Jan 19, 2017

Nice! 👍

So just for the confirmation, this behavior is only present with annotations?

@chalasr
Contributor
chalasr commented Jan 19, 2017

Yes, it is only present for annotations, which make this bug exists only when using them.

@iltar
iltar approved these changes Jan 19, 2017 View changes

Seems like a double fix to me

@fabpot
Member
fabpot commented Jan 19, 2017 edited

The code you removed was introduced in 84adcb1 by @lyrixx. See #5904 for the associated PR.

@chalasr
Contributor
chalasr commented Jan 19, 2017 edited

@fabpot I found it and was about to add a comment for clarifying this patch after discussed it with @iltar and @xabbuh on slack.

The code removed here seems unnecessary, arguments with default values properly keep their default value even after removing it, same on 3.0.
Actually, it seems that this code sets attributes that are not needed, and it prevents the ArgumentResolver from resolving these arguments (since they have an equivalent request attribute which takes precedence), and this only occurs when using annotations.

@lyrixx Could you please confirm that this code is no more needed and that removing it in 2.7 would not break anything? The bug fixed here exists on 2.7 as well, before that argument value resolvers were introduced.

@fabpot
Member
fabpot commented Jan 19, 2017

If the tests added back then by @lyrixx still work after the removal, I would say that this is safe to do on 2.7 instead of 3.1. Any drawbacks?

@chalasr chalasr changed the base branch to symfony:2.7 from symfony:3.1 Jan 19, 2017
@chalasr chalasr Avoid setting request attributes from signature arguments in Annotati…
…onClassLoader
d3fa8a1
@chalasr
Contributor
chalasr commented Jan 19, 2017

Changed the target to 2.7. Tests seem green, let's see the build.

@lyrixx
Member
lyrixx commented Jan 19, 2017

Could you please confirm that this code is no more needed and that removing it in 2.7 would not break anything? The bug fixed here exists on 2.7 as well, before that argument value resolvers were introduced.

Actually I don't know. The best way to know it is to test in a real project I guess.

@chalasr
Contributor
chalasr commented Jan 19, 2017

Ok thanks, tried out on 2.7 and it works as expected (for the record: screenshot on a fresh 2.7).

@fabpot
Member
fabpot commented Jan 19, 2017

Thank you @chalasr.

@fabpot fabpot merged commit d3fa8a1 into symfony:2.7 Jan 19, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
@fabpot fabpot added a commit that referenced this pull request Jan 19, 2017
@fabpot fabpot bug #21333 [HttpKernel] Fix ArgumentValueResolver for arguments defau…
…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
77289b9
@chalasr chalasr deleted the chalasr:kernel/resolve-nullable-arguments branch Jan 19, 2017
@fabpot fabpot added a commit that referenced this pull request Jan 24, 2017
@fabpot fabpot minor #21379 [Routing] Fix BC break in AnnotationClassLoader defaults…
… attributes handling (chalasr)

This PR was merged into the 2.7 branch.

Discussion
----------

[Routing] Fix BC break in AnnotationClassLoader defaults attributes handling

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

This fixes a BC break introduced in #21333. Instead of removing the automatic request attributes creation, we keep it but only for attributes that are mandatory (i.e. present in the route path).

Thanks to @iltar for the idea.

Commits
-------

1d298f0 [Routing] Fix BC break in AnnotationClassLoader defaults attributes handling
dd4e78c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment