-
Notifications
You must be signed in to change notification settings - Fork 418
fixed multiple file upload errors #664
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
Pull Request Overview
This PR fixes multiple issues with file upload handling in the Flight framework. The changes improve handling of array-format file uploads and add multipart form data parsing support for PUT, PATCH, and DELETE requests.
- Fixes array format detection for file uploads to properly maintain array structure for
file[]style inputs - Adds comprehensive multipart/form-data parsing for PUT, PATCH, and DELETE HTTP methods
- Improves test coverage with new test cases for multiple file upload scenarios
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/RequestTest.php | Enhanced test coverage for multiple file upload scenarios including single files, array format with single file, and multiple files |
| tests/RequestBodyParserTest.php | New test file covering multipart form data parsing for various HTTP methods and scenarios |
| flight/net/Request.php | Core implementation of multipart parsing logic and fixes to array format detection in file upload handling |
Comments suppressed due to low confidence (1)
tests/RequestBodyParserTest.php:1
- [nitpick] The helper method
assertUploadedFileis defined in RequestTest.php but could be extracted to a shared test trait or base class to avoid potential duplication if similar assertions are needed in other test files.
<?php
|
So I looked at the PR and fixed the reason why those 3 tests failed (because of the unset($_GET) $_POST, etc). I need to think more about the method you added to parse the body. While it works, and while it's unit tested.....man that is complex and I wonder how much of that is just rewriting how the $_FILES array internally handles things vs extra customizations that are needed for it to fully work correctly. |
Co-authored-by: Copilot <[email protected]>
Thanks a lot for taking a look at this. It’s actually a necessary customization to make multipart handling work properly for non-POST methods like PUT, DELETE, and PATCH. Additionally, I totally understand your concern about re-implementing how This logic ensures FlightPHP provides a consistent and predictable structure regardless of the HTTP method or file count. In that sense, this goes beyond a simple re-implementation — it’s filling a gap that PHP itself doesn’t cover. |
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.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Co-authored-by: Copilot <[email protected]>
This reverts commit 847a0d5.
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.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
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.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
|
dc56ca2 Fixed Response::write() TypeError when bool value passed from Engine. URL query parameters like ?asdf=12 were causing errors because write() method only accepted strings but Engine was passing boolean values. Removed strict string type to allow mixed parameter types.
|
|
Have a look over my changes and test it out on your end to make sure it does what you need it to do.
This may need some further investigation. I know it seems small, but I'm curious why it's trying to write a bool or an int to the body when the body is supposed to be a string. |
Fixed file upload for PATCH/PUT/DELETE methods. |
|
ok, I looked through this and added some more security checks. Go ahead and poke at this. I got rid of php-watcher as well. Totally spaced looking through the write() issue you were talking about. Can you tell me why it's trying to write |
Strange thing is, though, now that I’ve added the I’m not entirely sure why the Engine was passing a boolean value to |
|
Everything else worked as expected though? |
Yes |


Multiple file upload errors
[Error]

[Fix]

.\vendor\bin\phpunit.bat --filter testGetMultiFileUpload
PHPUnit 9.6.29 by Sebastian Bergmann and contributors.
Runtime: PHP 8.2.12
Configuration: D:\Pj\flightphp_core\core\phpunit.xml
Random Seed: 1760594422
. 1 / 1 (100%)
Time: 00:00.009, Memory: 8.00 MB
OK (1 test, 22 assertions)
Array format error in single file
Modified getUploadedFiles() to maintain array format for file[] input.
[Parameters]

[Error]

[Fix]


php vendor\phpunit\phpunit\phpunit --filter testGetMultiFileUpload
PHPUnit 9.6.29 by Sebastian Bergmann and contributors.
Runtime: PHP 8.2.12
Configuration: D:\Pj\flightphp_core\core\phpunit.xml
Random Seed: 1760603751
. 1 / 1 (100%)
Time: 00:00.031, Memory: 8.00 MB
OK (1 test, 23 assertions)
Add multipart parsing for PUT/PATCH/DELETE
php vendor\phpunit\phpunit\phpunit tests\RequestBodyParserTest.php
PHPUnit 9.6.29 by Sebastian Bergmann and contributors.
Runtime: PHP 8.2.12
Configuration: D:\Pj\flightphp_core\core\phpunit.xml
Random Seed: 1760606917
........... 11 / 11 (100%)
Time: 00:00.035, Memory: 6.00 MB
OK (11 tests, 20 assertions)