Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The upload.module returns without error when it fails to upload a file that exceeds the php.ini upload size limit. The upload.module should report an error on the attachment screen that upload failed.
Comment | File | Size | Author |
---|---|---|---|
#78 | 30520-file-save-upload-error-handling.patch | 3.58 KB | marsdk |
#77 | file_save_upload_2.patch | 10.7 KB | brad.bulger |
#72 | file_save_upload.patch | 10.66 KB | cwgordon7 |
#59 | 30520-file-save-upload-D6.patch | 11.12 KB | Dave Reid |
#58 | 30520-file-save-upload-D6.patch | 11.23 KB | Dave Reid |
Comments
Comment #1
magico CreditAttribution: magico commentedConfirmed. It's still a bug.
Comment #2
RobRoy CreditAttribution: RobRoy commentedStill a bug in the latest 4.7. I was just looking at php.net and saw that...
I think the upload creates an invisible IFRAME or something to that effect. So maybe we should check to see if a $_GET var is set, if not we should return an error saying the file exceeded the PHP configuration limits.
Regardless, what is happening now is users are uploading files that are too large and the upload form is just returning blank. Meaning if you have a file uploaded already, and try to upload another that is too large, it will fail and remove the existing file from the list.
Comment #3
magico CreditAttribution: magico commentedConfirmed.
1. when uploads a file larger than PHP limit it fails without a message
2. when uploads a file larger than PHP limit, it removes the already uploaded files
Comment #4
Joshua Hesketh CreditAttribution: Joshua Hesketh commentedI can confirm this bug in 5.1
This has been open since 4.6, could somebody please look into it.
Comment #5
RobRoy CreditAttribution: RobRoy commentedMoving to 6.x-dev. Hope I get some time to work on my old issues before the freeze...Anyone have any ideas on this?
Comment #6
RobRoy CreditAttribution: RobRoy commentedCheck out related issue: http://drupal.org/node/104220.
Comment #7
drewish CreditAttribution: drewish commentedit looks like the problem goes back to the 4.7 days and the problem is that we check that is_uploaded_file() is true before checking for errors one of the comments on the patch alludes to the problem.
i've reworked the test so that it only checks that the file is_uploaded_file() when there's no error.
Comment #8
webernet CreditAttribution: webernet commentedTested the patch - I'm not seeing any change though, large files still fail silently.
There is a notice though: 'notice: Undefined property: stdClass::$vid in /modules/upload/upload.module on line 536.'
Comment #9
snufkin CreditAttribution: snufkin commentedI can confirm this, so i change the status.
Comment #10
drewish CreditAttribution: drewish commentedhere's a slightly improved version. it gives better errors.
i think the problem you guys are running into trying to test this is that it depends on php's max post and file upload sizes. if you upload a file bigger than the post max size you won't see any error because php silently discards the submission. so for example with:
uploading a 1mb file would work. uploading an 8mb file would fail with an error. but if you upload a 38mb file you'd loose everything submitted with the form. there's not much we can do about this.
also when you're testing this, use the preview button rather than the attach button. there's some issues with the javascript that are separate from this.
Comment #11
drewish CreditAttribution: drewish commentedi want to clarify that this patch only fixes one thing: if you submit a file that's larger than php's max file size and less than the max post size you will now get an error. we can't do anything about uploads that are larger than the post size.
Comment #12
snufkin CreditAttribution: snufkin commentedBecause of the unique error it drops on reaching the POST size limit I thought it is possible to filter out the conditions when this occurs. This is a try. This patch gives correct error messages when on node/add/story, but removes the previous attachments when editing an existing node and encountering the POST size limit. Maybe someone finds this useful.
Comment #13
webernet CreditAttribution: webernet commentedI've rerolled the patch:
- Fixed an incomplete comment.
- Removed the separate error messages since they weren't actually being used (if the file is larger than the user roles maximum file size, but less than the PHP maximum, the file uploads properly and the error is thrown later)
Comment #14
drewish CreditAttribution: drewish commentedi tested out snufkin's patch on #12 and it didn't seem to report an error for a file larger than the max post size.
webernet, i think your patch on #13 is a step backwards. you remerged distinct error messages. in one case it's larger than the max upload size in another someone put a constant on the form specifying a maximum upload size. totally different... a misleading error message is worse than none at all. i was trying to separate them out to make it clearer what had happened.
Comment #15
drewish CreditAttribution: drewish commentedComment #16
webernet CreditAttribution: webernet commentedThis patch once again splits the messages for the two cases.
Comment #17
drewish CreditAttribution: drewish commentedi tried to improve the error messages a bit and used webernet's suggestion on IRC of using the filename rather than $source in the error messages.
Comment #18
drewish CreditAttribution: drewish commentedComment #19
Gábor Hojtsyphp.net suggests (as quoted above) that we also pass a GET parameter, which signals that we should have files, and if we have no files, that means we have gone over the POST max size. We can easily use both POST and GET data, so we can detect the case when a file was expected but not uploaded at all.
Comment #20
drewish CreditAttribution: drewish commentedGabor, the attached patch would catch a single file that's oversize, the problem is when the whole request is larger than the max post size. in that case then entire request is discarded and you get a blank node upload form... should we be passing a default ?posted=1 to all pages?
Comment #21
RobRoy CreditAttribution: RobRoy commented@drewish: It sounds like that's what PHP recommends doing, I think that's a good idea.
Comment #22
drewish CreditAttribution: drewish commentedRobRoy, I agree it's a good idea but I've got no idea where to start hacking the Forms API so that we pass a GET when submitting all the forms with '#type'=>'file' element on them.
Considering that at this point there's two errors that aren't handled correctly, it seems to me that fixing one would be a good start.
Comment #23
RobRoy CreditAttribution: RobRoy commentedI came in late to this so probably don't grasp the whole situation, but just want to jot down some quick ideas that may inspire a real solution.
I was thinking we'd just do it manually somehow. Like in upload_form_alter() we would do a $form['#file_upload'] = TRUE; when adding the attachments fieldset. Then, when FAPI is gonna submit the form...it takes whatever $form['#action'] is set to and adds a file_upload=1 to the query string. Then, the submit handler could check for that.
But that isn't too well thought out, because I guess we only set file_upload=1 when a file upload is attempted, not just when a 'file' field is included in the form. So maybe all file FAPI elements have their own generic '#submit' handler called 'file_validate_form_upload' or something that does some magic based on data submitted.
Ehhh, ya those are some pretty worthless suggestions but might get the balls rolling. :P
Comment #24
sethcohn CreditAttribution: sethcohn commentedbump.
Can we agree this is a problem, and that having the problem continue to exist since late 2005 is pretty sad?
How about a fix that deals with some of the above, rather than leave the entire issue outstanding?
Comment #25
drewish CreditAttribution: drewish commentedbumping to HEAD, hopefully we can backport something once this gets committed.
Comment #26
drewish CreditAttribution: drewish commentedmarked #282605: New Audio content never created if file exceeds max upload (no warning given) as a duplicate of this.
Comment #27
drewish CreditAttribution: drewish commentedalso marked #248185: Wrong error management in file_save_upload as a duplicate. improving the title to make it easier to find
Comment #28
mfbLooks like the patch is not specific to upload.module.
Comment #29
yassin.mohii CreditAttribution: yassin.mohii commentedI tried the last patch and it worked for me on drupal 5.
This patch is for drupal 5 (the check was in a function called file_check_upload).
Comment #30
yassin.mohii CreditAttribution: yassin.mohii commentedIf The file size exceeds the post_max_size what about calling error_get_last() ?
The php generate this warning :
PHP Warning: POST Content-Length of (failed uploaded size) bytes exceeds the limit of (post_max_size) bytes
Comment #31
Damien Tournoud CreditAttribution: Damien Tournoud commentedMarked #289518: No error message for attachment larger than PHP settings limit as a duplicate.
Comment #32
drewish CreditAttribution: drewish commentedhere's a reroll against HEAD.
Comment #35
drewish CreditAttribution: drewish commentedDrupalTestbedBot is a wrong, looks like it grabbed the previous patch that yassin.mohii posted.
Update: Reported an issue for the test bot #307197: Testing the incorrect patch posting multiple comments.
Comment #36
drewish CreditAttribution: drewish commentedre-rolled for HEAD. if you want unit tests take a look at #310358: Add a test for file_save_upload and clean up file.test
Comment #38
drewish CreditAttribution: drewish commentedi'm going to drag the DrupalTestbedBot out back and shoot it: http://drupal.org/node/307197
Comment #39
drewish CreditAttribution: drewish commentedlast patch was rolled from /includes... re-rolled.
Comment #41
LArjona CreditAttribution: LArjona commentedI have drupal 5.12 and I could only apply the patch attached to comment #29, the rest fail for my system.
Anyway the patch only solves the problem of submitting a file greater than php upload_max_filesize AND smaller than post_max_size
If post_max_size is exceeded the problem continues.
Comment #42
drewish CreditAttribution: drewish commentedhere's a re-roll that restructures this a bit so that if there's no upload we hurry up and just return FALSE rather than indenting the rest of the code. all file and upload tests pass.
Comment #43
drewish CreditAttribution: drewish commentedre-rolled.
Comment #44
webchickExcellent! This has bitten me in the past and I'm very happy to see a working patch!
Tested by setting my max_post_size to 32M and max_upload_filesize to 1M and uploading a 3M file. Without the patch, nothing happens, leaving me scratching my head and feeling lost and alone. With the patch, I get a nice error message to tell me what happened.
Code looks good (it's basically just a re-shuffling) and file tests continue to pass. I asked drewish if we could supplement this with unit tests, but unfortunately we cannot; both of these INI values are not settable at run-time.
Committed to HEAD. Marking for porting to 6.x/5.x.
Comment #45
drewish CreditAttribution: drewish commentedHumm... not quite yet. I just noticed one other test case, submitting the form at admin/build/themes/settings with no files incorrectly gave the following errors:
I'm working on another patch right now but I'll pick this up when I'm finished.
FileSaveUploadTest really needs to add tests for submitting an empty upload to an optional and required fields.
Comment #46
drewish CreditAttribution: drewish commentedThis doesn't address the test yet but now it returns NULL if no upload was attempted.
Comment #47
Dave ReidMarked #357382: Error message shown when no upload file provided as a duplicate of this bug. The patch in #46 is contains unwanted debugging statement so posting a revised patch.
Comment #48
drewish CreditAttribution: drewish commentedmuch better looking. i'd like to get tests but it might be worth holding off on for them for now and getting this fixed.
Comment #49
drewish CreditAttribution: drewish commentedjust double checked and this patch fixes the problem in all the file elements that are currently in core (theme logo/favicon, user pictures, upload.modue).
Comment #50
catchTested, works, RTBC.
Comment #51
Dave ReidOh my...trying to write tests for this is just horrible. Can't use drupalPost() with an empty file field.
Comment #52
dopry CreditAttribution: dopry commentedLooks like dagnabbed artwork... works for me... and if it can't be tested I see no reason to hold it up.
Comment #53
Dave ReidI think I finally figured out a test for this. Before the change to file_save_upload(), this new test will fail. With the change, it passes.
Comment #54
Dave ReidCleaned out the unrelated whitespace changes and documentation revision for the test.
Comment #55
drewish CreditAttribution: drewish commentedthanks, #54 looks good and tests pass. RTBC.
Comment #56
webchickYay! :) Really glad to have this fixed AND tested so we don't break it again. :D
Committed to HEAD.
Comment #57
Gábor HojtsyFrom #44/#45, it looks like we need this backported.
Comment #58
Dave ReidHere's the backport for #43 and #54 combined. I tested uploading a new user picture on my 6.x patched install and it worked successfully. The only API change is that the function returns FALSE/NULL instead of 0, which all == FALSE.
Comment #59
Dave ReidRevised patch that includes the more helpful error messages in #361514: Error messages in file_save_upload should use the actual file name without changing any actual strings for translation.
Comment #60
tyler-durden CreditAttribution: tyler-durden commentedIs there any way to backport this to 5.x ?? I just found this error on my site today, and I won't be changing to 6.x for quite awhile. Thanks!
Comment #61
JaredAM CreditAttribution: JaredAM commented+1 on the backport please! I also just found this issue.
Comment #62
Dave ReidFeel free to review the patch and if it works for you, mark as "reviewed and tested by the community"
Comment #63
quicksketchConfirmed this patch fixes problems with both Upload module and FileField in Drupal 6. The patch looks large but doesn't change nearly as many lines as it appears to. A huge improvement to prevent upload.module from silently failing without giving the user any indication of an error whatsoever.
Tested by:
- Setting post_max_size to 10M and upload_max_filesize to 1M. Restart Apache.
- Uploaded a 2M file in upload module and in FileField. FileField reports unhelpfully "The file in the field was unable to be uploaded.", Upload doesn't report anything at all. After patching, both report "The file IMG_1208.JPG could not be saved, because it exceeds 1 MB, the maximum allowed size for uploads."
- Uploaded a 500K file to ensure files are still accepted.
Both modules still fail pretty spectacularly when an upload exceeds the post_max_size (the form element disappears entirely, preventing any uploads), but that's a different issue. Great work on solving this one.
Comment #64
Gábor HojtsyPretty big patch indeed, so I'd welcome if more people would test it before RTBC. I for example found a function called which does not even exist in Drupal 6: file_load_multiple(). This function is also used in a passage of code which is not in the Drupal 6 version currently and is not related to fixing this bug either.
Comment #65
drewish CreditAttribution: drewish commentedGabor's right this whole block of code in D7 only:
Comment #66
Alan D. CreditAttribution: Alan D. commentedI'm have also had issues with this, but checks on anything after the max size of $_POST is exceeded is almost pointless as the entire reference to the form build id is lost. The only solution that I have found so far is to change theme_form to append the action with the build id. This ensures that this is never lost and a failed POST can be detected.
Rather than pushing in a patch, I just post the code to get feedback on the idea.
So with the modifications, a var_dump on $_GET and $_POST normally will result in something like:
But if max_post_size is exceeded:
In both cases, we can still retain the actual form that was originally posted and also detect if the form data has been dropped.
Comment #67
drewish CreditAttribution: drewish commentedAlan D., I think that's an interesting approach but it should really be in a follow up issue since it seems a bit too big to get backported for D6.
Comment #68
Alan D. CreditAttribution: Alan D. commentedFollowup on #66, see Issue: Enabling handling of errors related to max_post_size.
Comment #69
inforeto CreditAttribution: inforeto commented+1 to backport to 5.x
flashvideo module needs this fix (http://drupal.org/node/463126)
Comment #70
Bilmar CreditAttribution: Bilmar commentedHello,
I was wondering if there has been further development in commiting this to D6?
I came across this issue today and glad to have come across this issue post.
Regards
Comment #71
robby.smith CreditAttribution: robby.smith commentedsubscribing
Comment #72
cwgordon7 CreditAttribution: cwgordon7 commentedPatch no longer applied, here is a rerolled patch that removes the Drupal 7 - specific code block.
Comment #74
hansfn CreditAttribution: hansfn commentedThis issue bit me yesterday - very hard to debug since there is no error in the Apache error log.
Looking forward to a fix.
Comment #75
dwightaspinwall CreditAttribution: dwightaspinwall commentedsubscribing - hansfn, the error goes to the php log, not apache
Comment #76
hansfn CreditAttribution: hansfn commentedThx for caring dwight, but if you are running PHP as an Apache module, the default behavior is to write PHP error messages to Apache's error log. And yes, all other PHP errors display in the Apache error log.
Comment #77
brad.bulger CreditAttribution: brad.bulger commentedRedo of the patch file against what I got from CVS for DRUPAL-6.
I'm seeing the better error handling for uploads and filefields.
This patch is removing the second file_munge_filename() call on files that have had ".txt" added because they have dangerous extensions (php, js, etc) - is that a carryover of a different fix from D7? It seems like an OK change to me but I don't know enough of the background to really tell.
Comment #78
marsdk CreditAttribution: marsdk commented5 years later it is still an error ;)
Comment #80
TechNikh CreditAttribution: TechNikh commentedThe patch in #78 is missing a closing bracket ")"
should be