Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpFoundation] fix FileBag under PHP 8.1#42112
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
alexpott commentedJul 14, 2021
I don't think is the correct fix. I think we might want to get this new information into \Symfony\Component\HttpFoundation\File\UploadedFile - but the list of hardcoded keys is not that fun. |
nicolas-grekas commentedJul 15, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Maybe this is good enough for 4.4, and we need to improve |
alexpott commentedJul 15, 2021
@nicolas-grekas taking this approach in 4.4 seems reasonable. I've not got time atm for PR for 5.4. Working through well over 1000 fails for the Drupal test suite on PHP 8.1 :D |
426c0c8 todc35049Comparederrabus commentedJul 15, 2021
Thank you@alexpott. |
…ath()` to support directory uploads (danielburger1337)This PR was merged into the 7.1 branch.Discussion----------[HttpFoundation] Add `UploadedFile::getClientOriginalPath()` to support directory uploads| Q | A| ------------- | ---| Branch? | 7.1| Bug fix? | no| New feature? | yes| Deprecations? | no| License | MITThis removes the "hotfix"#42112 and now supports "full_path" PHP file uploads.Since PHP 8.1 is now the lowest allowed version of PHP, this shouldnt cause any problems.---Although I'm quite certain that I understand the messy FileBag code, I'm very concerned for the potential impact this change might have because of the widespread use of http-foundation.To the best of my knowledge, this won't break Symfony BC (yet) because this is all done by an `optional` argument in the constructor of UploadedFile (this should eventually be replaced by a mandatory argument though).Also, I'm not quite happy with the tests. Since this is only a "passthrough" of the PHP value, this _shouldnt_ be a huge problem.Someone should definitly go through this change very carefully. The tests pass and I also threw some "real world" application tests at it and everything seems to work just fine. But because of the nature of HttpFoundation and its intangled mess, I can't guarantee that I accounted for every possible or even simple edge case.---Though it would be nice to still see this in 7.0, I know that it will probably be pushed back to 7.1.Commits-------dac592b [HttpFoundation] Add `UploadedFile::getClientOriginalPath()` to support directory uploads
…ath()` to support directory uploads (danielburger1337)This PR was merged into the 7.1 branch.Discussion----------[HttpFoundation] Add `UploadedFile::getClientOriginalPath()` to support directory uploads| Q | A| ------------- | ---| Branch? | 7.1| Bug fix? | no| New feature? | yes| Deprecations? | no| License | MITThis removes the "hotfix"symfony/symfony#42112 and now supports "full_path" PHP file uploads.Since PHP 8.1 is now the lowest allowed version of PHP, this shouldnt cause any problems.---Although I'm quite certain that I understand the messy FileBag code, I'm very concerned for the potential impact this change might have because of the widespread use of http-foundation.To the best of my knowledge, this won't break Symfony BC (yet) because this is all done by an `optional` argument in the constructor of UploadedFile (this should eventually be replaced by a mandatory argument though).Also, I'm not quite happy with the tests. Since this is only a "passthrough" of the PHP value, this _shouldnt_ be a huge problem.Someone should definitly go through this change very carefully. The tests pass and I also threw some "real world" application tests at it and everything seems to work just fine. But because of the nature of HttpFoundation and its intangled mess, I can't guarantee that I accounted for every possible or even simple edge case.---Though it would be nice to still see this in 7.0, I know that it will probably be pushed back to 7.1.Commits-------dac592b513 [HttpFoundation] Add `UploadedFile::getClientOriginalPath()` to support directory uploads
Seehttps://php.watch/versions/8.1/$_FILES-full-path - in \Symfony\Component\HttpFoundation\FileBag::convertFileInformation() we check against a list of hardcoded keys. This logic breaks in PHP 8.1 because of the new key.