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

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

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 providedDefaultController::indexAction(Request $request) toDefaultController::indexAction(Request $request = null);

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).

@iltar I can confirm this since usingindexAction(Request $request = null) and adding adump($request);die; immediately before this linehttps://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 beingcontinue 2;) while covering all other cases that are working well originally, I was hoping you can say me more :/

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 thecontinue 2; is hit. So if something else hits this first, there is probably another issue at hand.

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 theRequestValueResolver/SecurityUserValueResolver, but maybe there's something wrong in the request argument one that should be fixed instead.
Waiting for your feedback.

@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 theRequestValueResolver, due to the parameter converters

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! Arequest (variable name) key is put in the attribute bag withnull as value. Same problem for theSecurityUserValueResolver given auser attribute withnull as value exists, theRequestAttributeValueResolver takes precedence of course...

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

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 ithas() the key and it's not empty

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 insideRequestAttributeValueResolver::supports() I think it could help indeed, I'd say a strictnull !== $attribute though

ThePC 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

I updated this for reverting the previous changes and only adding the additionalnull !== $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.

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

Nice! 👍

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

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

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

@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.

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?

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

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.

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

Thank you@chalasr.