WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 21 months ago

#19840 closed task (blessed) (fixed)

Select custom header or background images from media library

Reported by: jane Owned by: ryan
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.3
Component: Media Keywords:
Focuses: Cc:

Description (last modified by aaroncampbell)

In a recent version, we made it so uploaded custom header images get saved in the media library. Awesome! Next step: on the custom header and background screens, we need an option to choose an image from the media library. UI-wise, should just be an option next to the select/upload line. Should be implemented on both screens. The fix was to make step() nonce-aware so it could better return the proper step.

Attachments (25)

19840.diff (5.0 KB) - added by greuben 2 years ago.
v2.diff (5.5 KB) - added by greuben 2 years ago.
v3.diff (11.7 KB) - added by greuben 2 years ago.
19840.2.diff (12.3 KB) - added by aaroncampbell 2 years ago.
19840.3.diff (13.3 KB) - added by aaroncampbell 2 years ago.
19840.4.diff (13.9 KB) - added by greuben 2 years ago.
19840.5.diff (14.1 KB) - added by greuben 2 years ago.
19840.5.2.diff (14.9 KB) - added by sabreuse 2 years ago.
Fixes remaining notices
19840.6.diff (13.9 KB) - added by aaroncampbell 2 years ago.
19840.7.diff (15.0 KB) - added by aaroncampbell 2 years ago.
19840.8.diff (15.2 KB) - added by ryan 2 years ago.
Another refresh
Screen Shot 2012-03-01 at 2.43.22 PM.png (30.9 KB) - added by ryan 2 years ago.
Screenshot, Choose from image library
Screen Shot 2012-03-01 at 2.44.56 PM.png (54.6 KB) - added by ryan 2 years ago.
Screenshot, Dialog
19840.9.diff (15.2 KB) - added by greuben 2 years ago.
fix horizontal scroll
19840.10.diff (14.0 KB) - added by greuben 2 years ago.
19840.11.diff (14.0 KB) - added by sabreuse 2 years ago.
refresh of 19840.10.diff
19840.12.diff (14.8 KB) - added by aaroncampbell 2 years ago.
19840.13.diff (15.0 KB) - added by aaroncampbell 2 years ago.
19840.14.diff (15.1 KB) - added by aaroncampbell 2 years ago.
19840.15.diff (14.6 KB) - added by aaroncampbell 2 years ago.
19840.16.diff (14.4 KB) - added by aaroncampbell 2 years ago.
19840.17.diff (14.4 KB) - added by aaroncampbell 2 years ago.
19840.18.diff (5.8 KB) - added by ryan 2 years ago.
Call load_image_to_edit() from wp_crop_image(). Get h/w from meta if file does not exist. Make sure upload dir exists.
Screen-shot-2012-04-06-at-9.jpg (29.6 KB) - added by thee17 2 years ago.
I am experiencing this issue when the box to add from media lib in Chrome on OSX
19840.19.diff (6.3 KB) - added by ryan 2 years ago.

Download all attachments as: .zip

Change History (72)

comment:1 DrewAPicture2 years ago

  • Cc xoodrew@… added

comment:2 sabreuse2 years ago

  • Cc sabreuse@… added

comment:3 Mamaduka2 years ago

  • Cc georgemamadashvili@… added

comment:4 aaroncampbell2 years ago

  • Cc aaroncampbell added

comment:5 mpvanwinkle772 years ago

  • Cc mpvanwinkle77 added

greuben2 years ago

comment:6 greuben2 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

ticket:19840:19840.diff adds the link on custom background page. If the patch is good, will duplicate it for custom header.

comment:7 aaroncampbell2 years ago

Seems to work really well for me. For the header images there will be a little more to do because we'll need to add the meta to keep them listed as headers. I'm also assuming we want to allow the user to crop for headers since a lot of headers will still require fixed dimensions.

@greuben - If you're up for doing the patch though, that sounds great!

comment:8 sabreuse2 years ago

@greuben, thanks for the patch!

The one issue I'm seeing right away is that regardless of the image size I choose in the image library, the background is being set to use the thumbnail size.

I'll also second Aaron's comments about headers -- whatever functionality we currently apply to headers has to also be available for headers from the image library -- it should act exactly the same as choosing any other image for the end user, so we'd have to allow for cropping (including flex sizes), storing as part of the available set for random headers, etc.

greuben2 years ago

comment:9 greuben2 years ago

@aaroncampbell: Yes header images do need a little more coding. Will post a patch for it asap.

@sabreuse: Thanks for the feedback. Check v2.diff, it removes other unnecessary form fields i.e url, title, caption and allows to select right image size

greuben2 years ago

comment:10 greuben2 years ago

v3.diff - patch for both custom background & header.

aaroncampbell2 years ago

comment:11 aaroncampbell2 years ago

The v3 patch didn't work for me at all. I found out it's because the JS in only enqueued/echod if NO_HEADER_TEXT is not defined or is false. Since I had header text turned off I didn't get any of the JS. 19840.2.diff fixes that.

aaroncampbell2 years ago

comment:12 aaroncampbell2 years ago

I talked to @nacin and 19840.3.diff includes his suggestions:

  • Context for the 'or' translation like drag-drop does
  • Instead of adding a second parameter to get_upload_iframe_src() the first parameter now accepts a string or an array.
  • Moved wp_set_background_image() inside the Custom_Background class.

comment:13 sabreuse2 years ago

With the latest patch (and also the previous one, but I wasn't ever able to capture the actual messages), I'm seeing a brief flash of several notices within the thickbox, immediately after clicking "Use as header image" or "Use as background".

Notice: Undefined index: post_title in /var/www/core/wp-admin/includes/media.php on line 489 
Notice: Undefined index: url in /var/www/core/wp-admin/includes/media.php on line 896 
Notice: Undefined index: post_excerpt in /var/www/core/wp-admin/includes/media.php on line 902 
Notice: Undefined index: post_title in /var/www/core/wp-admin/includes/media.php on line 902

Those refer to media_send_to_editor() and image_media_send_to_editor().

Everything after that point works as expected, but I'm not sure of the right approach for the notices - suppress (since they're not actually attributes we need in this case)? Or?

greuben2 years ago

comment:14 greuben2 years ago

19840.4.diff 19840.5.diff fixes the notices.

Last edited 2 years ago by greuben (previous) (diff)

greuben2 years ago

sabreuse2 years ago

Fixes remaining notices

comment:15 sabreuse2 years ago

It looks like my patch crossed paths with @greuben's 19840.5

comment:16 scribu2 years ago

  • Cc scribu added

comment:17 scribu2 years ago

So which patch should we test?

comment:18 greuben2 years ago

Replying to sabreuse:

It looks like my patch crossed paths with @greuben's 19840.5

The two patches differ a little bit

19840.5.diff removes the unwanted filter image_media_send_to_editor()

19840.5.2.diff does sanity checks i.e isset() in image_media_send_to_editor()

aaroncampbell2 years ago

comment:19 aaroncampbell2 years ago

19840.6.diff is based on .5 by @grueben. I like removing the default filter if our filter is handling things, I think it's cleaner. There's really just some code formatting cleanup (Whitespace fixes, etc) and the removal of a couple comments that were clearly for debugging.

comment:20 follow-up: ryan2 years ago

Needs refresh due to [19999]

aaroncampbell2 years ago

comment:21 in reply to: ↑ 20 aaroncampbell2 years ago

Replying to ryan:

Needs refresh due to [19999]

19840.7.diff is refreshed. It moves back to an args array as the parameter, so that other things can be passed in the future.

ryan2 years ago

Another refresh

comment:22 ryan2 years ago

Thickbox has a horizontal scrollbar for me. Do we need the "Edit Image" button?

ryan2 years ago

Screenshot, Choose from image library

ryan2 years ago

Screenshot, Dialog

greuben2 years ago

fix horizontal scroll

greuben2 years ago

comment:23 greuben2 years ago

19840.10.diff - modified the patch based on azaozz's suggestions in IRC.

sabreuse2 years ago

refresh of 19840.10.diff

comment:24 sabreuse2 years ago

19840.11.diff is a refresh of 19840.10 (previous patch didn't apply cleanly after recent changes to /wp-admin/includes/media.php)

Last edited 2 years ago by sabreuse (previous) (diff)

comment:25 sabreuse2 years ago

New issue introduced in 19840.10.diff: after the js changes in this version, choosing a custom background image from the media library correctly updates the background, but tb_remove() fails, and the thickbox remains onscreen with no indication that the bg has been changed.

(I'm also confused about why this script ended up in the set-post-thumbnail.js file -- as I understood the IRC conversation, the point was to no longer override send_to_editor, but sticking it in a file that's not related to backgrounds at all doesn't seem very maintainable. Was that a mistake or am I missing some logic there?)

Confirmed that choosing a background was working as expected as of 19840.9, and that selecting a custom header works as expected in the latest.

comment:26 mau2 years ago

  • Cc ngomau@… added

aaroncampbell2 years ago

comment:27 aaroncampbell2 years ago

19840.12.diff seems to work for me even post-nacin's war on insanity. I agree with @sabreuse that the JS didn't really belong in the set-post-thumbnail.js file, so I added a custom-header.dev.js which makes more sense.

aaroncampbell2 years ago

comment:28 aaroncampbell2 years ago

  • Description modified (diff)

19840.13.diff fixes an issue with the custom headers page. If you let the nonce expire and refreshed on step 2 or 3, it would load step 1. However, since step=2/3 was still in the URL the proper JS wasn't loaded and the "choose from image library" link stopped working.

aaroncampbell2 years ago

comment:29 aaroncampbell2 years ago

19840.14.diff fixes the background issue. It wasn't functioning because WPSetAsBackground() wasn't in the scope of the iframe. The patch calls it in the parent's scope, but also adds a size parameter, since from the parent it can't see the size form.

comment:30 follow-up: aaroncampbell2 years ago

Had a few people here at WCSD test this and it worked for all of them (once they got script debug on).

comment:31 in reply to: ↑ 30 jkudish2 years ago

  • Cc joachim.kudish@… added

Replying to aaroncampbell:

Had a few people here at WCSD test this and it worked for all of them (once they got script debug on).

Indeed, it worked perfectly!

comment:32 aaroncampbell2 years ago

  • Keywords commit added; needs-testing removed

aaroncampbell2 years ago

comment:33 aaroncampbell2 years ago

Koop noticed that .14 had an unneeded remnant of an old patch. Fixed in 19840.15.diff

comment:34 wpsmith2 years ago

  • Cc travis@… added

aaroncampbell2 years ago

comment:35 aaroncampbell2 years ago

19840.16.diff gets rid of the global function WPSetAsBackground(), uses listeners to get rid of the javascript: occurrences, and changes get_upload_iframe_src() to three params instead of $args.

aaroncampbell2 years ago

comment:36 aaroncampbell2 years ago

19840.17.diff includes some coding style fixes by azaozz and a fix for an accidentally global variable.

comment:37 ryan2 years ago

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

In [20358]:

Allow selecting custom header and background images from the media library. Props aaroncampbell, sabreuse, greuben. fixes #19840

comment:38 ryan2 years ago

In [20359]:

Add media-gallery js. see #19840

comment:39 duck_2 years ago

In [20369]:

Remove duplicate JavaScript. Props aaroncampbell. See #19840, fixes #20363.

comment:40 ryan2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This doesn't work with environments that save files remotely (using a replication plugin of some sort). The image editor accommodates remote file via load_image_to_edit(), which tries to do a url fopen if the file doesn't exist. Let's try to do the same for headers/backgrounds. Failing that, make it possible to turn off add from library via plugin.

Warning: getimagesize(/.../wp-content/blogs.dir/xxx/12345/files/2009/05/an-image.jpg) [<a href='function.getimagesize'>function.getimagesize</a>]: failed to open stream: No such file or directory in /.../wp-admin/custom-header.php on line 703

comment:41 ryan2 years ago

Basically, in step 2, if the filename from get_attached_file() does not exist then use the url from wp_get_attachment_image_src() and somehow make it work either way. :-)

ryan2 years ago

Call load_image_to_edit() from wp_crop_image(). Get h/w from meta if file does not exist. Make sure upload dir exists.

thee172 years ago

I am experiencing this issue when the box to add from media lib in Chrome on OSX

ryan2 years ago

comment:42 ryan2 years ago

In [20384]:

Make choosing a header image from the media library play nicely with file replication plugins that do not guarantee images will be retained in the local filesystem.

  • When passing an attachment ID to wp_crop_image(), use load_image_to_edit() to fetch the image via a url fopen when the image does not exist in the filesystem.
  • Move load_image_to_edit() to wp-admin/includes/image.php so that it is always available for admin pages loads.
  • Fallback to the height and width stored in the attachment meta when the image no longer exists in the filesystem.

see #19840

comment:43 ryan2 years ago

  • Keywords has-patch commit removed

comment:44 ryan2 years ago

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

comment:45 westi2 years ago

Opened #20657 for a nasty bug.

comment:46 hd-J2 years ago

  • Cc jeremy@… added

Opened #20856 for a new bug

comment:47 SergeyBiryukov21 months ago

#15578 was marked as a duplicate.

Note: See TracTickets for help on using tickets.