Skip to content

Conversation

@KnifeLemon
Copy link
Contributor

@KnifeLemon KnifeLemon commented Oct 16, 2025

Multiple file upload errors

[Error]
error

[Fix]
fixed

.\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]
param

[Error]
error

[Fix]
fix_1
fix2

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)

@n0nag0n n0nag0n requested a review from Copilot October 16, 2025 14:36
Copy link
Contributor

Copilot AI left a 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 assertUploadedFile is 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

@n0nag0n
Copy link
Collaborator

n0nag0n commented Oct 16, 2025

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.

@KnifeLemon
Copy link
Contributor Author

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.

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 $_FILES works. PHP itself behaves inconsistently depending on whether a single or multiple files are uploaded under the same field name. Even if we use [] in the field name, PHP will parse it as a single file structure when only one file is uploaded, and as an array structure when multiple files are uploaded.

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.

@KnifeLemon KnifeLemon requested a review from Copilot October 17, 2025 02:09
Copy link
Contributor

Copilot AI left a 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.

@KnifeLemon KnifeLemon requested a review from Copilot October 17, 2025 02:32
Copy link
Contributor

Copilot AI left a 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.

@KnifeLemon KnifeLemon requested a review from Copilot October 17, 2025 02:37
Copy link
Contributor

Copilot AI left a 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.

@KnifeLemon
Copy link
Contributor Author

dc56ca2
When I tried to access the link https://mydomain.com/panel/main?asdf=12, an error message was displayed.

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.

image

@n0nag0n
Copy link
Collaborator

n0nag0n commented Oct 21, 2025

Have a look over my changes and test it out on your end to make sure it does what you need it to do.

dc56ca2 When I tried to access the link https://mydomain.com/panel/main?asdf=12, an error message was displayed.

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.

image

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.

@KnifeLemon
Copy link
Contributor Author

Have a look over my changes and test it out on your end to make sure it does what you need it to do.

dc56ca2 When I tried to access the link https://mydomain.com/panel/main?asdf=12, an error message was displayed.
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.
image

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.
is_uploaded_file() only works with POST uploads, causing failures with other HTTP methods.

@n0nag0n
Copy link
Collaborator

n0nag0n commented Oct 27, 2025

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 true to the write() method?

@KnifeLemon
Copy link
Contributor Author

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 true to the write() method?

Strange thing is, though, now that I’ve added the string type back, it seems to work fine without any issues... 😅

I’m not entirely sure why the Engine was passing a boolean value to write()
When accessing URLs with query parameters like ?asdf=12, the error occurred consistently, but I couldn’t trace the exact cause inside the Engine layer.
My change simply makes write() more tolerant of non-string types to prevent this crash.

@n0nag0n
Copy link
Collaborator

n0nag0n commented Oct 28, 2025

Everything else worked as expected though?

@KnifeLemon
Copy link
Contributor Author

그래도 다른 모든 것은 예상대로 작동했습니까?

Yes

@n0nag0n n0nag0n merged commit 854f668 into flightphp:master Oct 29, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants