Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel] #[MapUploadedFile] throws http exception on empty files array if argument not nullable nor has default value#61381
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestPayloadValueResolver.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I understand that this is a bug fix, but also a behavior change, so I'm not sure we should do it in 7.3.
@symfony/mergers Thoughts?
Indeed. Previously it returned an empty array or |
| } | ||
| return$files; | ||
| return$request->files->get($attribute->name ??$argument->getName()); |
nicolas-grekasAug 19, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What looks strange to me in the previous code is defaulting to an array without caring about the accepted type. To reduce the behavioral diff, we should continue to default to an array when arrays are accepted - and when not, then we should fix the behavior.
This would need a new test case for the array-case.
| return$request->files->get($attribute->name ??$argument->getName()); | |
| if (!($files =$request->files->get($attribute->name ??$argument->getName())) && ($argument->isNullable() ||$argument->hasDefaultValue())) { | |
| returnnull; | |
| } | |
| return$files ?? ('array' ===$argument->getType() ? [] :null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I see at least 4 test cases that should be verified when no file is provided:
#[MapUploadedFile] array $files=> should return an empty array#[MapUploadedFile] ?array $files=> should returnnull#[MapUploadedFile] UploadedFile $file=> should returnnulland an exception will be thrown#[MapUploadedFile] UploadedFile ?$file=> should returnnull
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I understand returning an empty array for the sake of BC, but IMO an absence of value should be denoted bynull and not an empty array. The user indicates the value is optional by making the argument nullable (that's also what the docs currently say), otherwise anHttpException should be thrown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's not how this works for arrays for the other value resolvers. We have to be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Alright, I applied your suggestion and added the missing test cases. I am not sure if the failed pipelines are related to my changes.
…array if argument not nullable nor has default value
Thank you@hwawshy. |
4102229 intosymfony:7.3Uh oh!
There was an error while loading.Please reload this page.
Fixes#61376