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] FixgetMaxFilesize#32790
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
| publicstaticfunctiongetMaxFilesize() | ||
| { | ||
| $iniMax =strtolower(ini_get('upload_max_filesize')); | ||
| $iniMax =min([ini_get('upload_max_filesize'),ini_get('post_max_size')]); |
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.
min() cannot work here, we should parse post_max_size before calling it, as it can be a string with units (e.g. 8M)
please also add test cases
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.
Your're right. Would it be okay to add anparseFilesize to the class? Also since there seems to be no test forgetMaxFilesize till now I'm not entirely sure how to create one since it depends on the ini settings.
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.
Would it be okay to add an parseFilesize to the class
a private one yes,
not entirely sure how to create one since it depends on the ini settings
dunno if it's possible to use ini_set here... if not, I don't know either :)
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.
Well, I've implemented the proper conversion and check on the two values but regarding the tests it seems we're out of luck. We can't set e.g.upload_max_filesize usingini_set() becauseupload_max_filesize is aPHP_INI_PERDIR type which means it's only changeable via php.ini, .htaccess or httpd.conf ( seephp.net/manual/en/configuration.changes.modes.php)
That also explains why there was no test on this method to begin with.
xabbuh commentedJul 30, 2019
Can we add a test case for this? |
nicolas-grekas commentedJul 30, 2019
Not sure, see#32790 (comment) |
nicolas-grekas commentedJul 30, 2019
Thank you@bennyborn. |
This PR was squashed before being merged into the 3.4 branch (closes#32790).Discussion----------[HttpFoundation] Fix `getMaxFilesize`When checking for the maximum size of an uploaded file you can't just rely on `upload_max_filesize` since the request might also exceed `post_max_size`. Also discussed incontao/contao#498| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Commits-------54107ba [HttpFoundation] Fix `getMaxFilesize`
bennyborn commentedJul 30, 2019
No worries, glad I could help! |
This PR was merged into the 3.4 branch.Discussion----------Fix getMaxFilesize() returning zero| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#32790| License | MITWith#32790 a BC break got introduced. Previously an empty `upload_max_filesize` returned `PHP_INT_MAX` but after the changes from#32790 it returns `0`.Setting `upload_max_filesize` or `post_max_size` to `0` or `''` disables the limit so for both cases `PHP_INT_MAX` should be returned.Commits-------f4c2ea5 Fix getMaxFilesize() returning zero
When checking for the maximum size of an uploaded file you can't just rely on
upload_max_filesizesince the request might also exceedpost_max_size. Also discussed incontao/contao#498