[HttpKernel] #[MapUploadedFile] throws http exception on empty files array if argument not nullable nor has default value #61381
Conversation
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestPayloadValueResolver.php
Outdated
Show resolved
Hide resolved
|
Indeed. Previously it returned an empty array or |
| } | ||
|
|
||
| return $files; | ||
| return $request->files->get($attribute->name ?? $argument->getName()); |
There was a problem hiding this comment.
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())) { | |
| return null; | |
| } | |
| return $files ?? ('array' === $argument->getType() ? [] : null); |
There was a problem hiding this comment.
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.
I understand returning an empty array for the sake of BC, but IMO an absence of value should be denoted by null 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 an HttpException should be thrown.
There was a problem hiding this comment.
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.
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
bc04da3 to
67eaf1f
Compare
|
Thank you @hwawshy. |
Fixes #61376