Removing header image does not remove header_image_data theme mod

[kovshenin]

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.

[kovshenin]

Use remove_theme_mod when removing custom header.

[kovshenin]

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.

If set_theme_mod had an action or a filter, we could hook to that to see if header image was set to remove-header and use remove_theme_mod with header_image_data like in ​20871.diff. I believe that would address both issues. Thoughts?

[nacin]

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.

[nacin]

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.

[nacin]

In [21027]:

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

[nacin]

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.

[ryan]

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.

[SergeyBiryukov]

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).

[nacin]

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.

[koopersmith]

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

[koopersmith]

Latest patch cleans up some of the data logic.

[ryan]

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

[nacin]

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.

[nacin]

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.

[ryan]

Looks good.

[ryan]

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."

[nacin]

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.

[nacin]

Follow-up breakage: #25156