http://www.sfu.ca/~jdbates/tmp/drupal/200910300/patch

My upload_max_filesize php.ini directive is set to "0" (zero), so there is no PHP per file upload maximum file size, I can upload files of any size, so long as the total request doesn't exceed the post_max_size php.ini directive

However, I can't set a Drupal "default maximum file size per upload"

When I enter any value into this field on the "file uploads" configuration page, and submit, Drupal displays an error, "Your PHP settings limit the maximum file size per upload to 0 bytes."

To reproduce, set php.ini directives post_max_size to e.g. "8M" and upload_max_filesize to "0"

Enter any value into the "default maximum file site per upload" field on the "file uploads" configuration page, and submit

This patch fixes this by correctly interpreting an upload_max_filesize value of "0" as unlimited

With this patch applied, I can successfully set a Drupal "default maximum file size per upload", provided it's less than the post_max_size

- if I try entering a value which exceeds the post_max_size, Drupal correctly displays an error, e.g. "Your PHP settings limit the maximum file size per upload to 8 MB."

CommentFileSizeAuthor
#11 619434-11.patch758 bytesthedavidmeister
#8 619434-8.patch892 bytesjablko
#5 619434b.patch1.07 KBjim0203
#1 619434.patch784 bytesjim0203
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jim0203’s picture

jim0203 CreditAttribution: jim0203 commented
FileSize
619434.patch784 bytes

Attaching the patch to the issue so the bot reviews it and I can use dreditor with it :-).

jim0203’s picture

jim0203 CreditAttribution: jim0203 commented
Status: Needs review » Reviewed & tested by the community

Recreated the bug as per instructions on my clean D7 install. The patch fixes the bug and the code is clean. Set to RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs review

This makes sense. I'm trying to figure out how we could test this, though. Is it possible for a test to set post_max_size and upload_max_filesize to ridiculously small numbers and then attempt to upload one of the files from the SimpleTest files directory?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs review » Needs work

To answer my own question, it looks like the answer is no:

phpinfo(); 

ini_set('upload_max_filesize', 1);
ini_set('post_max_size', 1);

phpinfo();

In both instances of phpinfo(), I get the same result.

However, let's comment this a bit better so it explains the interaction between upload_max_filesize and post_max_size. There's useful information in this issue that's not grokkable from the source code. Also, make sure that all comments end in a period, for consistency.

jim0203’s picture

jim0203 CreditAttribution: jim0203 commented
FileSize
619434b.patch1.07 KB

Thanks, webchick. Have added comments; new patch attached.

jim0203’s picture

jim0203 CreditAttribution: jim0203 commented
Status: Needs work » Needs review

Changing status.

MichaelCole’s picture

MichaelCole CreditAttribution: MichaelCole commented

#5: 619434b.patch queued for re-testing.

jablko’s picture

jablko CreditAttribution: jablko commented
FileSize
619434-8.patch892 bytes

I reproduced today with D7 from CVS

I set php.ini directives post_max_size to e.g. "8M" and upload_max_filesize to "0"

The "file uploads" page has moved since I reported this issue - now I go to "structure" in the dashboard, "content types", and click "edit" next to "article"

Click "manage fields" and click "edit" next to the "image" field

Currently under "maximum upload size" I see, "If left empty the file sizes will be limited only by PHP's maximum post and file upload sizes (current limit 0 bytes)."

This patch fixes current limit, so I see e.g. "current limit 8 MB"

Currently I can save any value in the "maximum upload size", including values greater than the "current limit", or the string "banana" - will report as a new issue

ngstigator’s picture

ngstigator CreditAttribution: ngstigator commented
Version: 6.x-dev » 7.x-dev
Status: Needs review » Reviewed & tested by the community

Reproduced on clean D7 install. Applied patch and works as advertised.

I've noticed that the field does not validate values that exceed "post_max_size" nor strings like "banana" (thx webchick). Searching issue queue for this.

UPDATE: separate issue created for this #870576: Insufficient validation of the max upload file size field

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Version: 7.x-dev » 6.x-dev

Nice work on the comments, thanks! Also great work cleaning up the legibility of that code.

Committed to HEAD. :) Welcome to the core team, jablko and chris_five! :D

Looks like this applies with an offset to 6.x too. If we could get a quick test on that, go ahead and bump back to RTBC.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Version: 7.x-dev » 6.x-dev
FileSize
619434-11.patch758 bytes

rerolled for d6.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 619434-11.patch, failed testing.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.