Skip to content
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

[12.x] Improve docblocks for file related methods of InteractsWithInput #55156

Merged

Conversation

SanderMuller
Copy link
Contributor

This PR improves the PHPDoc annotations for file-related methods and properties in the InteractsWithInput trait and the Request class.

  • Adds precise generic array types to allFiles() and convertUploadedFiles()
  • Enhances file() return type with a conditional PHPStan-style annotation
  • Updates the $convertedFiles property docblock accordingly

These changes improve static analysis, IDE autocomplete, and developer understanding of the expected data structures, particularly for nested or multiple file uploads.

No functional code changes were made.

@SanderMuller SanderMuller changed the title Improve docblocks for file related methods of InteractsWithInput [12.x] Improve docblocks for file related methods of InteractsWithInput Mar 24, 2025
Copy link

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@@ -240,7 +240,7 @@ protected function isValidFile($file)
*
* @param string|null $key
* @param mixed $default
* @return \Illuminate\Http\UploadedFile|\Illuminate\Http\UploadedFile[]|array|null
* @return ($key is null ? array<int, \Illuminate\Http\UploadedFile|array<int, \Illuminate\Http\UploadedFile>> : \Illuminate\Http\UploadedFile|\Illuminate\Http\UploadedFile[]|null)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when $key is null, the return type is always an array of files, otherwise it can be either an instance of a file, an array of files, or null

@@ -175,7 +175,7 @@ public function cookie($key = null, $default = null)
/**
* Get an array of all of the files on the request.
*
* @return array
* @return array<int, \Illuminate\Http\UploadedFile|\Illuminate\Http\UploadedFile[]>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think most codebases don't take an array containing another array of files into consideration, but it is possible according tot he code. convertUploadedFiles recursively maps the array to files or if the array contains an array, to an array of files (inside the array of files) resulting in nested arrays of files.

I guess not flat mapping this is on purpose (would be a breaking change now anyway), so the docblocks reflect the possibility of an array of items of which each item is either a file or an array of files.

@SanderMuller SanderMuller marked this pull request as ready for review March 24, 2025 14:41
@taylorotwell taylorotwell merged commit 4625a21 into laravel:12.x Mar 24, 2025
37 of 39 checks passed
@@ -175,7 +175,7 @@ public function cookie($key = null, $default = null)
/**
* Get an array of all of the files on the request.
*
* @return array
* @return array<int, \Illuminate\Http\UploadedFile|\Illuminate\Http\UploadedFile[]>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key here appears to be wrong, on FormRequest the key will be the name of the file input.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed here: #55287

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually looking in to adding types to this a few weeks ago, but backed off when I saw some of the nested logic so I'm glad you handled most of it :)

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.

None yet

3 participants