Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#19800 closed defect (bug) (fixed)

Media & WXR Uploads: Inaccurate error reporting because move_uploaded_file() fails to return false.

Reported by: emhr's profile emhr Owned by: duck_'s profile duck_
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.3
Component: Upload Keywords: has-patch
Focuses: Cc:


Steps to reproduce:
Current month directory exists in uploads directory hierarchy but is not writable.

Case-1 Upload a new media item. Data stored in database but file is not uploaded.

Case-2 Upload a WXR import file using the WP importer plugin. Plugin reports it's fallback error message: "This does not appear to be a WXR file, missing/invalid WXR version number" even if the WXR file has a valid wxr version number. The import fails.

Expected behavior:
The uploader should return the following error, "The uploaded file could not be moved to"…path

Pass precise path to wp_tempnam() instead of default

Bug Since: #18206

Related: #19720

Attachments (5)

uploader-#19800-patch.diff (531 bytes) - added by emhr 12 years ago.
uploader-19800-patch.diff (527 bytes) - added by emhr 12 years ago.
minor optimization
19800.check-copy.diff (575 bytes) - added by duck_ 12 years ago.
19800.less-copying.diff (1.8 KB) - added by duck_ 12 years ago.
19800.remove-resize.diff (1.5 KB) - added by duck_ 12 years ago.

Download all attachments as: .zip

Change History (11)

12 years ago

minor optimization

#1 @duck_
12 years ago

  • Version changed from 3.3.1 to 3.3

Good catch, thanks!

The changeset that caused this was [18482].

I am uploading two different patch options:

  1. Check the return value of copy() and return an error then.
  2. move_uploaded_file() to the uploads directory as before. If a resize is requested the new, resized image is made and renamed.

The second has the benefit of only moving/copying the upload once if a resize is not requested. Currently we always move and then copy anyway. I haven't done proper testing for when a resize is requested for either (though it's only the second that could have a bug), and I don't think the second does enough to ensure that the resized file name is unique.

A couple of other things I noticed about [18482]:

  1. If the upload cannot be moved to wp_tempnam($filename) (this is rather unlikely) then the error message will display an incorrect path
  2. There's no feedback returned to the user if an image resize fails

A third option is to remove this resizing code. This was actually (mostly) done in [19223] for #19174, but it came back in [19225] and [19226] to allow plugins to make use of resizing. However, since this code has been in wp_handle_upload() for a release it probably cannot be taken out now.

Finally, I'm not really sure why, but I seem to feel that it's a strange location for resizing code.

#2 @emhr
12 years ago

I tested 19800.check-copy.diff and 19800.less-copying.diff They both provide proper error reporting for the two cases I mentioned.
They both assign unique filenames as expected for all files including the resized ones.

19800.less-copying.diff looks great in comparison. Reducing disk writes is good.

#3 follow-up: @nacin
12 years ago

It would be good to get azaozz's input, but I would pull the resizing code.

#4 in reply to: ↑ 3 @azaozz
12 years ago

Replying to nacin:

...but I would pull the resizing code.

Agreed. The best option is to let Plupload resize images (before uploading), the PHP functionality was a fallback for the browsers that still don't support this in JS.

Last edited 12 years ago by azaozz (previous) (diff)

#5 @duck_
12 years ago

  • Owner set to duck_
  • Resolution set to fixed
  • Status changed from new to closed

In [20019]:

Drop image resizing code from wp_handle_upload(). Fixes #19800.

This code stops wp_handle_upload() from reporting errors when the upload couldn't be moved to its final local and it was a non-JS fallback that is unused.

#6 @ocean90
12 years ago

  • Milestone changed from Awaiting Review to 3.4
Note: See TracTickets for help on using tickets.