WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 weeks ago

#18206 closed task (blessed) (fixed)

Upgrade the file uploader

Reported by: azaozz Owned by:
Milestone: 3.3 Priority: normal
Severity: normal Version:
Component: Upload Keywords: needs-testing needs-patch
Focuses: Cc:

Description

As discussed at the dev. meetup on 2011/07/20, merge the GSoC project http://gsoc.trac.wordpress.org/search?q=jacobwg http://gsoc.trac.wordpress.org/browser/2011/jacobwg in trunk.

Attachments (10)

jacobwg-gsoc2011-patch01.diff (17.0 KB) - added by jacobwg 3 years ago.
Initial patch on latest WP trunk (r18463)
jacobwg-gsoc2011-patch02.diff (16.8 KB) - added by jacobwg 3 years ago.
New patch - should fix the issues with the previous patch
jacobwg-gsoc2011-patch03.diff (160.6 KB) - added by jacobwg 3 years ago.
wp-includes.zip (60.8 KB) - added by jacobwg 3 years ago.
plupload libraries for inclusion
18206-spelling.patch (933 bytes) - added by jacobwg 3 years ago.
Fix for spelling error
post_params_str-tabs.diff (502 bytes) - added by duck_ 3 years ago.
If we're trying to get nice output we should get it to work
18206.fix.resize.patch (1.4 KB) - added by ocean90 3 years ago.
ie8-plupload-silverlight-resize-off.png (112.9 KB) - added by kurtpayne 3 years ago.
JavaScript error when scale images is unchecked
ie8-plupload-silverlight-resize-on.png (101.9 KB) - added by kurtpayne 3 years ago.
Successful upload when scale images is checked
18206.diff (18.1 KB) - added by kurtpayne 3 years ago.
Workaround for plupload.silverlight jsonSerialize bug

Download all attachments as: .zip

Change History (70)

comment:1 sbressler3 years ago

  • Cc sbressler@… added

jacobwg3 years ago

Initial patch on latest WP trunk (r18463)

comment:2 jacobwg3 years ago

  • Cc me@… added
  • Keywords has-patch needs-testing added

To test, unzip the wp-includes.zip at the WP root (the new Plupload libraries with binary files), apply the patch, and delete wp-includes/js/swfupload/* and wp-includes/js/swfobject.js.

comment:3 SergeyBiryukov3 years ago

With the patch applied, there's a warning when uploading a file from Media > Add New:

Notice: Use of undefined constant IFRAME_REQUEST - assumed 'IFRAME_REQUEST' in wp-admin/includes/media.php on line 1569

After uploading:

Notice: Undefined variable: resize in wp-admin/includes/file.php on line 334

jacobwg3 years ago

New patch - should fix the issues with the previous patch

comment:4 ocean903 years ago

Nice work!

  • The link to the flash file has an error, it's js/plupload/plupload.flash.swf. But after the change it seems that it doesn't work anyway. ( OX Chrome and Firefox)
  • Also view this ticket #11520. If it's final check if you need to use localize or add_script_data
  • If I want to upload a ZIP file I get this message (because it's not an image ;) )
Warning: imagecreatefromstring() [function.imagecreatefromstring]: Data is not in a recognized format in wp-includes/media.php on line 254
376
  • Don't know if it's public, but "Allowed files" isn't translatable.

comment:5 scribu3 years ago

  • Cc scribu added

jacobwg3 years ago

plupload libraries for inclusion

comment:7 jacobwg3 years ago

Thanks, ocean90! I believe I fixed the things you mentioned... I've uploaded a new patch (patch number 3) and replaced the wp-includes.zip as the patch now contains most of the new files in that folder.

As always, you can get my latest updates at http://gsoc.svn.wordpress.org/2011/jacobwg

Thanks for testing!

comment:8 follow-up: pento3 years ago

  • Cc gary@… added

So, is it too late for feature requests? :-D

In wp-admin/includes/admin.php, media_upload_form(), you have JavaScript to create the uploader, and bind the various events to the handler functions. Currently, you only have a filter for the uploadSuccess() function, plupload_success_handler. It would be nice to have filters on all of the other event handlers, too.

comment:9 kawauso3 years ago

  • Cc kawauso added

comment:10 in reply to: ↑ 8 azaozz3 years ago

Replying to pento:

...Currently, you only have a filter for the uploadSuccess() function, plupload_success_handler. It would be nice to have filters on all of the other event handlers, too.

Perhaps we can add JS hooks there. We are using simple pubsub in DFW that works very well (thanks to @koopersmith that put it together), maybe it's time to make it global.

comment:11 azaozz3 years ago

There are two TODOs in script-loader:

  1. "find out if we can use the plupload.full.js (it does load browserplus)". Think we should use it. There may be time in the future when we turn browserplus support on.
  1. "find out if we really need this debug clause with plupload". SCRIPT_DEBUG is checked so we can load the .dev.js files but that concerns only our JS. We don't have non-minified versions for most external libraries (jQuery, UI, etc.) so no need to check for SCRIPT_DEBUG. Or perhaps we can load plupload.full.js by default and switch to loading the different components if SCRIPT_DEBUG.
Last edited 3 years ago by azaozz (previous) (diff)

comment:12 azaozz3 years ago

In [18482]:

Pluploader take 1, props jacobwg, see #18206

comment:13 azaozz3 years ago

In [18483]:

Remove debug bits, see #18206

comment:14 azaozz3 years ago

Will probably need to keep the old uploader around too for plugin compat. Also to make sure it still can be used.

comment:15 follow-up: scribu3 years ago

It works, but it looks just like before. Am I missing something?

comment:16 ocean903 years ago

  • Keywords has-patch removed

Am I missing something?

No, only that it's now HTML5 instead of Flash (if you have a modern browser. ;))

comment:17 dimadin3 years ago

I don't see why are SWFObject and SWFUpload removed since they are used by plugins, we should keep them.

Also, you have a typo here:

'image_memory_exceeded' => __('Memery exceeded. Please try another smaller file.'),

jacobwg3 years ago

Fix for spelling error

comment:18 in reply to: ↑ 15 jacobwg3 years ago

Replying to scribu:

It works, but it looks just like before. Am I missing something?

Yeah, it's just a backend change - the currently enabled runtimes are html5, silverlight, flash, and html4 - so the uploader will pick the first one in that order that the browser supports when doing the uploading. The UI is the same (with the addition of a resize option box).

comment:19 azaozz3 years ago

In [18484]:

Add SWFUpload back for plugins that queue it directly, fix some escaping in media.php, see #18206

comment:20 scribu3 years ago

[18484] introduced a typo in script-loader.php:

// error messagoe for both plupload and swfupload 

comment:21 ocean903 years ago

Related: #16518

comment:22 follow-up: aaroncampbell3 years ago

I can drag/drop to upload if I go into a post and click to add an image. However, if I go to Media->Add New (wp-admin/media-new.php) drag/drop isn't working for me.

I'm on Chrome on Win7

comment:23 follow-up: DH-Shredder3 years ago

  • Cc mike.schroder@… added

I'm noticing that the default size is "Full Size" rather then "Medium" with the new uploader.

Is this intentional?

comment:24 in reply to: ↑ 23 ; follow-up: jacobwg3 years ago

Replying to DH-Shredder:

I'm noticing that the default size is "Full Size" rather then "Medium" with the new uploader.

Is this intentional?

Yeah, that was intentional as per a request from azaozz. I think it's mainly intended to combat the uploading of huge resolutions when they will never get used on the site.

comment:25 in reply to: ↑ 24 ; follow-up: DH-Shredder3 years ago

Replying to jacobwg:

Yeah, that was intentional as per a request from azaozz. I think it's mainly intended to combat the uploading of huge resolutions when they will never get used on the site.

Hmm. It seems to me that this could get a bit frustrating, as I often do want my "actual" image to be larger than the one that's in the post.

If you can choose which size is default within settings somewhere, this sounds fine, although I do think you'll get a few complaints, since I can think of very few times I want a full size image inline within my post.

comment:26 in reply to: ↑ 25 ; follow-up: azaozz3 years ago

Replying to DH-Shredder:

Resizing the image on/after uploading is optional (that's why we have the checkbox). If it's selected the image will be resized to the "large" size in Media Settings->Image sizes and then saved. You can of course uncheck the checkbox and the original image will be uploaded and saved. We will need better explanation there :)

When the image matches the "large" size, the choice in media library skips it and offers "full size" instead as they are the same. There's no default size there, WP "remembers" the last size you selected.

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

comment:27 in reply to: ↑ 22 azaozz3 years ago

Replying to aaroncampbell:

I can drag/drop to upload if I go into a post and click to add an image. However, if I go to Media->Add New (wp-admin/media-new.php) drag/drop isn't working for me.

Yes, not working there yet. Will need to add another wrapper. On the other hand there are plans to change the UI around the uploader so the current HTML will disappear.

duck_3 years ago

If we're trying to get nice output we should get it to work

comment:28 azaozz3 years ago

In [18489]:

Remove tabs from JS, props duck_, see #18206

comment:29 in reply to: ↑ 26 DH-Shredder3 years ago

Replying to azaozz:

<snip>
When the image matches the "large" size, the choice in media library skips it and offers "full size" instead as they are the same. There's no default size there, WP "remembers" the last size you selected.

Gotcha, thanks for the explanation! Essentially, what I'd been noticing was that WP wasn't "remembering" my choice between image inserting "sessions" (i.e. new post -> insert image, insert image [remembers]; another new post -> insert image [forgets last choice] )

However, I'm now (of course) not able to reproduce it again, so it's possible it was a one-time quirk. If I can find a way to regularly reproduce it again, I'll post here.

comment:30 Otto423 years ago

Bug: The new resize option box doesn't work. It always resizes my uploaded images even with it unchecked. I'm attempting to upload an image with a 3000px size, and it's resizing to 1024 on upload.

comment:31 ocean903 years ago

I can confirm the bug mentioned by otto42.

Is it possible to hide the Or you can drop the files into this window notice if the runtime, eg flash, doesn't support it?

Last edited 3 years ago by ocean90 (previous) (diff)

ocean903 years ago

comment:32 ocean903 years ago

18206.fix.resize.patch will fix the resize bug.


Another warning:

Notice: Undefined variable: plupload in /wp-admin/includes/media.php on line 1592

comment:33 ocean903 years ago

I have tested the Silverlight runtime but it fails. Chrome and IE on Win 7 64bit.

Chrome:

Uncaught TypeError: Cannot read property 'length' of null

IE:

SCRIPT5007: Unable to get value of the property 'length': object is null
or undefined
plupload.silverlight.js?ver=1.4.3.2, line 1 letter 329

It works on the demo page.


Drag&drop doesn't work on /wp-admin/media-new.php.

Last edited 3 years ago by ocean90 (previous) (diff)

comment:34 azaozz3 years ago

In [18517]:

Fix handling of resizing images after upload, props ocean90, see #18206

comment:35 mau3 years ago

  • Cc ngomau@… added

comment:36 ocean903 years ago

  • Keywords needs-patch added

Related: #18540

kurtpayne3 years ago

JavaScript error when scale images is unchecked

kurtpayne3 years ago

Successful upload when scale images is checked

comment:37 kurtpayne3 years ago

  • Cc kpayne@… added

Replying to ocean90:

I have tested the Silverlight runtime but it fails. Chrome and IE on Win 7 64bit.

Chrome:

Uncaught TypeError: Cannot read property 'length' of null

IE:

SCRIPT5007: Unable to get value of the property 'length': object is null
or undefined
plupload.silverlight.js?ver=1.4.3.2, line 1 letter 329

It works on the demo page.

This seems to be related to a bug with plupload's silverlight library. It doesn't like the image_resize checkbox coming through as a null value. See ie8-plupload-silverlight-resize-off.png and ie8-plupload-silverlight-resize-on.png for an example. The patch on #18540 should resolve this.

comment:38 follow-up: azaozz3 years ago

If this is caused by the 'image_resize' checkbox (i.e. our implementation) we should probably ensure it doesn't pass a null value regardless of whether that's fixed in plupload.

kurtpayne3 years ago

Workaround for plupload.silverlight jsonSerialize bug

comment:39 in reply to: ↑ 38 ; follow-up: kurtpayne3 years ago

Replying to azaozz:

If this is caused by the 'image_resize' checkbox (i.e. our implementation) we should probably ensure it doesn't pass a null value regardless of whether that's fixed in plupload.

18206.diff replaces the null value with a false value. If I've read the php code correctly, it's looking for a true value, so this change shouldn't affect the server-side resizing operation. Tested on Windows 7 64-bit + FF 6, Chrome 13, IE 8.

comment:40 in reply to: ↑ 39 azaozz3 years ago

Replying to kurtpayne:

...If I've read the php code correctly, it's looking for a true value, so this change shouldn't affect the server-side resizing operation.

Yes, it specifically matches the string "true".

In the future could you patch the .dev.js file(s). It's easier for you (they are not minified) and it's easier to read the changes. To use the non-minified js files in WordPress add define('SCRIPT_DEBUG', true); to the top of your wp-config.php file.

comment:41 azaozz3 years ago

In [18620]:

Fix silverlight uploading when not resizing files on the fly, props kurtpayne, see #18206

comment:42 follow-ups: SergeyBiryukov3 years ago

Since [18482], there's a notice on the uploader page:

Notice: Undefined variable: plupload in wp-admin/includes/media.php on line 1591

To reproduce:

  • Visit "Media" > "Add New" with Debug Bar enabled.
  • Or "Edit Post" > "Add an Image", open Firebug, search for "undefined" in the thickbox window (the notice is hidden by hide-if-js class).

Code reference:
http://core.trac.wordpress.org/browser/trunk/wp-admin/includes/media.php?rev=18596#L1591

There used to be media_upload_html_bypass() function hooked to post-html-upload-ui, which accepted $flash as a parameter. The function is now gone, so perhaps we can remove $plupload parameter from there, or just pass false.

comment:43 in reply to: ↑ 42 azaozz3 years ago

Replying to SergeyBiryukov:

Yes, with the old uploader it was possible to select whether to use the flash (multi-file) or the standard 'file' form field. That was needed as the old flash method had numerous problems with certain server configs so some users couldn't use it at all.

The new uploader tries several different multi-file methods before falling back to the single file field. We can probably add back the old single-file bypass if it turns out it's still needed. More importantly on the Add new media screen we need a <noscript> way of uploading a file too.

comment:44 follow-up: scribu3 years ago

The media uploader is currently borked. I get a JS error from Plupload:

t.name is undefined

comment:45 in reply to: ↑ 44 kurtpayne3 years ago

Replying to scribu:

The media uploader is currently borked. I get a JS error from Plupload:

t.name is undefined

I am unable to reproduce this with the latest trunk in Chrome / IE / Firefox with or without the debug bar. Can you post the steps you're using and the browser?

comment:46 in reply to: ↑ 42 kurtpayne3 years ago

Replying to SergeyBiryukov:

Since [18482], there's a notice on the uploader page:

Notice: Undefined variable: plupload in wp-admin/includes/media.php on line 1591

...
Code reference:
http://core.trac.wordpress.org/browser/trunk/wp-admin/includes/media.php?rev=18596#L1591

... perhaps we can remove $plupload parameter from there, or just pass false.

I can duplicate this, too. I can't find a reference to $plupload anywhere else in the code. Any reason this is still needed?

comment:47 ocean903 years ago

I already mentioned the notice in comment:32.

In comment:33 I mentioned, that the Drag&Drop feature isn't working on media-new.php. Should it working there too?

comment:48 azaozz3 years ago

In [18674]:

Improve handling of plupload init and add a filter, stop including cookies in the init (not needed with plupload), see #18206

comment:49 azaozz3 years ago

In [18701]:

Update plupload to 1.5b and add "drop area" for drag and drop uploading, see #18206

comment:50 follow-up: ocean903 years ago

azaozz: We need to merge media.css into wp-admin.css or enqueue media.css on media-new.php or copy the drag-drop style part into wp-admin.css, because on media-new.php the drag&drop box isn't styled.

comment:51 azaozz3 years ago

In [18703]:

Add the styling for "drop area" to Media->Add New, see #18206

comment:52 in reply to: ↑ 50 azaozz3 years ago

Replying to ocean90:

Right. I had media.css merged but it's mostly redundant and since we were trying new uploader styles, had to leave it stand-alone with the intention to delete it completely. Perhaps we can merge it in 3.4 when we will have a look at the uploader UI (drop the popup, etc.) hopefully.

In any case would love some patches on #18314.

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

comment:53 jane3 years ago

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

Feature freeze! This new feature is in core. At this point, if people find new bugs with it, they should open individual bug tickets.

comment:54 azaozz3 years ago

In [18902]:

As per the recommendations: make the text inside the files drop box larger, improve the explanation about the "resize image" checkbox, see #18206

comment:55 azaozz3 years ago

In [18957]:

Fix visibility of Select Files button in IE7, change border color when hovering over the drag drop area of the uploader, see #18918, see #18206

comment:6 azaozz2 years ago

In [19100]:

Fix uploader "file size exceeded" error message, fix displaying uploaded queue errors, see #18206

comment:7 azaozz2 years ago

In [19177]:

Uploader: split "Drop files here -or-..." on two lines, see #18206

comment:8 follow-up: azaozz2 years ago

In [19247]:

Uploader: fix progress bar JS and update the style (light blue bars), see #18206

comment:9 azaozz2 years ago

In [19262]:

Uploader: fix file-size-exceeded error handling, see #18206

comment:10 ircbot3 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by azaozz. View the logs.

Note: See TracTickets for help on using tickets.