#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: | Customize | 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)
Change History (27)
#1
@
12 years 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.
#2
@
12 years ago
- Milestone changed from Awaiting Review to 3.4
- Severity changed from normal to blocker
#4
@
12 years 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.
#5
@
12 years 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.
#7
@
12 years 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.
#8
follow-up:
↓ 9
@
12 years 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.
#9
in reply to:
↑ 8
@
12 years 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).
#10
@
12 years 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.
#11
@
12 years 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.
#14
@
12 years 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.
#15
@
12 years 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.
#18
@
12 years 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."
Use remove_theme_mod when removing custom header.