Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[HttpKernel] Fatal Error when using#[MapUploadedFile] with non-array/non-variadic argument#57824
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
base:7.1
Are you sure you want to change the base?
Conversation
@renedelima Thanks for this feature 💪 . |
MapUploadedFile for non-array/non-variadic argumentsMapUploadedFile for non-array/non-variadic arguments#[MapUploadedFile] for non-array/non-variadic arguments#[MapUploadedFile] for non-array/non-variadic arguments#[MapUploadedFile] as non-array/non-variadic argument#[MapUploadedFile] as non-array/non-variadic argument#[MapUploadedFile] with non-array/non-variadic argumentThere 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.
LGTM, I just have minor comments.
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestPayloadValueResolver.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestPayloadValueResolver.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| if (!(is_array($payload) && array_is_list($payload))) { | ||
| throw HttpException::fromStatusCode(422); | ||
| } | ||
| array_splice($arguments, $i, count($payload), $payload); |
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.
wouldn't array_merge be more appropriate?
| array_splice($arguments,$i,count($payload),$payload); | |
| array_splice($arguments,$i,\count($payload),$payload); |
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 wish it work, here I put more details#54901 (comment)
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestPayloadValueResolver.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| @@ -68,7 +68,7 @@ public function testEmpty(RequestPayloadValueResolver $resolver, Request $reques | |||
| $attribute = new MapUploadedFile(); | |||
| $argument = new ArgumentMetadata( | |||
| 'qux', | |||
| UploadedFile::class, | |||
| "array", | |||
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.
| "array", | |
| 'array', |
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.
Shouldn't this be spotted by fabbot? 🤔
| } | ||
| array_splice($arguments, $i, count($payload), $payload); | ||
| } else { | ||
| $arguments[$i] = $payload; |
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 about the situation when the argument isUploadedFile and the$payload is an array of files? I think this would still result in a500?
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.
Interesting 🤔
Is it possible to haveUploadedFile as part of#[MapRequestPayload] $payload? (#[MapQuery] we can exclude, I guess)
Could you please share an example of what you mean, so I can elaborate more on this ?
| @@ -170,7 +170,14 @@ public function onKernelControllerArguments(ControllerArgumentsEvent $event): vo | |||
| }; | |||
| } | |||
| $arguments[$i] = $payload; | |||
| if ($argument->metadata->isVariadic()) { | |||
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.
Should we check if it is variadic as well as if the type is an array here, similar to the check in themapUploadedFile method?
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.
Basically$arguments here are original arguments of an action.
In example below$arguments = [$request, $payload, $files];
publicfunctionaction(Request$request, #[MapRequestPayload]Payload$payload, #[MapUploadedFile]UploadedFile ...$files]) {...}
And the magic here is to expand variadic$files into several items of$arguments.
I hope PHP won't change mechanics of how variadic works and it will be always only 1 possible variadic argument in function signature 🤞
Also, there is already an ability to use#[MapUploadedFile] as bellow. And it works and covered byUploadedFileValueResolverTest::testMultipleFilesArray test case.
publicfunctionaction(Request$request, #[MapRequestPayload]Payload$payload,/** @var UploadedFile[] */ #[MapUploadedFile]array$files]) {...}
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 have tested variant
finalclass TestPayloadRequest{publicUploadedFile$file;}finalclass IndexControllerextends AbstractController{ #[Route(path:"/test-upload", methods:"POST")]publicfunctiontestUpload( #[MapRequestPayload]TestPayloadRequest$file, #[MapUploadedFile]UploadedFile ...$files, ):Response {returnnewResponse(); }}
with request like
POST /test-upload HTTP/1.1Cookie: XDEBUG_SESSION=PHPSTORM;Content-Type: multipart/form-data; charset=utf-8; boundary=__X_PAW_BOUNDARY__--__X_PAW_BOUNDARY__Content-Disposition: form-data; name="files[]"; filename="test.yaml"Content-Type: application/x-yamlinfo: test--__X_PAW_BOUNDARY__Content-Disposition: form-data; name="file"; filename="test.yaml"Content-Type: application/x-yamlinfo: test--__X_PAW_BOUNDARY__Content-Disposition: form-data; name="files[]"; filename="test.yaml"Content-Type: application/x-yamlinfo: testShortly: current implementation is not designed to work like this, because data for#[MapRequestPayload]
is fetched like this :
Seems like failed tests are unrelated to my changes |
@nicolas-grekas@stetodd friendly reminder ;) |
faa10b6 to93d4190Compareany updates? |
Uh oh!
There was an error while loading.Please reload this page.
Having a simple controller with
MapUploadedFileas non-array.When the
fileis omitted in request, symfony throwsValueErrorbecause resolver returns an empty array and it doesn't match withUploadedFile $file.In my opinion, the HttpException should be thrown in this case.
For array argument the behaviour stays the same.
Example with omitted
file-ValueErroroccurs hereError example