WordPress.org

Make WordPress Core

Opened 23 months ago

Closed 23 months ago

Last modified 8 months ago

#20871 closed defect (bug) (fixed)

Removing header image does not remove header_image_data theme mod

Reported by: kovshenin Owned by: koopersmith
Milestone: 3.4 Priority: highest omg bbq
Severity: blocker Version: 3.4
Component: Appearance Keywords: has-patch commit
Focuses: Cc:

Description

When hitting Remove Header Image in Appearance > Header, the header image mod is set to remove-header which causes get_header_image to return nothing, however, the value set for the header_image_data theme mod remains, so get_custom_header()->url will always show the last chosen header image, even if it has been removed.

This gets even nastier with flex width and height headers. When removing a header image, get_custom_header()->height will show the removed image height, and not the default theme mod height as (I) expected.

Attachments (7)

20871.diff (464 bytes) - added by kovshenin 23 months ago.
Use remove_theme_mod when removing custom header.
20871.2.diff (10.5 KB) - added by nacin 23 months ago.
20871.3.diff (12.3 KB) - added by SergeyBiryukov 23 months ago.
20871.4.diff (12.2 KB) - added by koopersmith 23 months ago.
20871.5.diff (18.4 KB) - added by koopersmith 23 months ago.
20871.6.diff (18.0 KB) - added by koopersmith 23 months ago.
20871.7.diff (1.5 KB) - added by nacin 23 months ago.

Download all attachments as: .zip

Change History (27)

kovshenin23 months ago

Use remove_theme_mod when removing custom header.

comment:1 kovshenin23 months ago

Also, the WP_Customize_Header_Image_Control doesn't seem to even touch the header_image_data theme mod. Setting the custom header from Appearance > Header (A) and then setting it to a different header using Live Preview (B) will change header_image to image B, but header_image_data will still have the data for image A.

Version 0, edited 23 months ago by kovshenin (next)

comment:2 nacin23 months ago

  • Milestone changed from Awaiting Review to 3.4
  • Severity changed from normal to blocker

comment:3 nacin23 months ago

  • Component changed from Themes to Appearance

comment:4 nacin23 months ago

  • Priority changed from normal to highest omg bbq

It is clear this is a blocker for release, as it results in bad or incomplete data in a theme mod, which can result in bad heights/widths, as well as disassociating uploaded items with their attachment_id.

We need a custom setting to handle custom headers, which will have to wrap methods (that need to be created) in the Custom_Header class (custom-header.php) in order to properly save or remove header_image_data.

comment:5 nacin23 months ago

We also do a whole lot of esc_url()'s when setting the header_image theme_mod. These should be esc_url_raw(). Unsure if this is a regression, but we definitely proliferated it in 3.4.

comment:6 nacin23 months ago

In [21027]:

Remove the set_theme_mods() method from customize manager. We ended up not using it. see #20871.

nacin23 months ago

comment:7 nacin23 months ago

20871.2.diff is a necessary cleanup of the Custom_Header class to abstract all theme mod changes into four distinct functions: removing an image, resetting an image, choosing an existing image (from the page), and selecting a new image (from the media library or view an upload). The documentation:

/**
 * Remove a header image.
 *
 * @since 3.4.0
 */
final public function remove_header_image()

/**
 * Reset a header image to the default image for the theme.
 *
 * This method does not do anything if the theme does not have a default header image.
 *
 * @since 3.4.0
 */
final public function reset_header_image()

/**
 * Choose a header image, selected from existing uploaded and default headers.
 *
 * @param string $choice Which header image to select. Allows for values of 'random-default-image',
 * 	for randomly cycling among the default images; 'random-uploaded-image', for randomly cycling
 * 	among the uploaded images; the key of a default image registered for that theme; and
 * 	the key of an image uploaded for that theme (the basename of the URL).
 */
final public function choose_existing_header_image( $choice )

/**
 * Select a header image, either newly uploaded or from the media library.
 *
 * @since 3.4.0
 *
 * @param array $args An array of arguments: attachment_id, url, width, height. All are required.
 */
final public function select_new_header_image( $args )

Notably:

  • It sets _wp_attachment_is_custom_header, which means the customizer must stop doing this for headers. (But continue doing it for backgrounds.)
  • The header_image_data theme mod is now removed when the header is "removed".
  • The header_image_data theme mod is now removed when either "random" option is chosen (random of uploads, or random of defaults). This is not actually going to get called in get_custom_header(), but it is good to make sure the DB is up to date/accurate.
  • esc_url_raw() is always used when saving data, rather than esc_url(). (This change is also made to backgrounds.)
  • I think get_custom_header() and other header and background template functions may need an esc_url() added, but I did not focus on that.

We will then need to implement a custom setting and/or control to leverage these four methods in the customizer.

comment:8 follow-up: ryan23 months ago

When you choose an image from the library and then skip cropping, using the image as is, I see:

Image could not be processed. Please go back and try again.

SergeyBiryukov23 months ago

comment:9 in reply to: ↑ 8 SergeyBiryukov23 months ago

Replying to ryan:

Image could not be processed. Please go back and try again.

Fixed in 20871.3.diff (replaced $id with $attachment_id in a couple more places).

koopersmith23 months ago

comment:10 nacin23 months ago

20871.4.diff reduces things down to three methods:

  • remove_header_image()
  • reset_header_image()
  • set_header_image()

It also introduces get_header_image_data(), which we can use for filtering on the frontend, but it won't be available on the frontend, so... We're discussing alternatives. That logic will likely just be duplicated in a custom customizer control.

koopersmith23 months ago

comment:11 koopersmith23 months ago

  • Keywords has-patch needs-testing added
  • Owner set to koopersmith
  • Status changed from new to accepted

Latest patch adds appropriate customizer integration. Give it a whirl.

koopersmith23 months ago

comment:12 koopersmith23 months ago

Latest patch cleans up some of the data logic.

comment:13 ryan23 months ago

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

In [21037]:

  • Introduce remove_header_image(), reset_header_image(), set_header_image(), and get_header_image_data() for Custom_Image_Header.
  • Handle all set/get of header theme mod through these methods.
  • Use these methods in the customizer.

Props kovshenin, nacin, SergeyBiryukov, koopersmith.
fixes #20871

comment:14 nacin23 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This also applies for background_image_thumb, which can break the admin preview.

Rather than reworking the customizer to also set this value, we can hack in something like this:

  • Instead of get_theme_mod( 'background_image_thumb', get_background_image() ), we also replace a set-but-empty theme mod value with get_background_image(), like this:
    $thumb = get_theme_mod( 'background_image_thumb' );
    if ( ! $thumb )
    	$thumb = get_background_image();
    
  • Also, the customizer would need to consistently remove_theme_mod( 'background_image_thumb' ) whenever it set the background_image theme mod.

Or, we can just make it always set background_image_thumb to be either the full URL or the thumbnail URL.

Or, we can just get rid of background_image_thumb entirely. I don't really see the usefulness as we only show a single preview background (one image), not a tile of uploaded backgrounds.

nacin23 months ago

comment:15 nacin23 months ago

20871.7.diff causes the customizer to always remove background_image_thumb.

This is a temporary stopgap between a decision in 3.5 to either kill background_image_thumb, or support it in the customizer like we do for header_image_data. As it is only used for the Appearance > Background preview, this is sufficient for now.

The addition of the final keyword was at koopersmith's request.

comment:16 nacin23 months ago

  • Keywords commit added; needs-testing removed

comment:17 ryan23 months ago

Looks good.

comment:18 ryan23 months ago

Test plan proposed in IRC:

"Good way to test: Upload an image via Appearance > Background. Upload a different image via Customizer. Go back to Appearance > Background.

"Without the patch: You'll see a thumbnail of the first image, not the second.

"With the patch: you'll see a full-size preview of the second image."

Last edited 23 months ago by ryan (previous) (diff)

comment:19 nacin23 months ago

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

In [21053]:

Theme Customizer: Remove background_image_thumb when saving settings. fixes #20871.

This prevents the background_image and background_image_thumb settings from getting out of sync.
In 3.5 we can consider using background-size on Appearance > Background, eliminating _thumb.

comment:20 nacin8 months ago

Follow-up breakage: #25156

Note: See TracTickets for help on using tickets.