WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 7 months ago

#38557 accepted defect (bug)

Format for registering a default header image is ambiguous

Reported by: bradyvercher Owned by: joemcgill
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch 2nd-opinion
Focuses: Cc:
PR Number:

Description

When registering a custom header with a default image, most resources seem to recommend using an absolute URL (Handbook, Codex, Make post). However, it can be registered with sprintf placeholders, which is what the old HEADER_IMAGE constant accepted. It's registered that way in Twenty Thirteen as well.

Any time core references the default image URL, it needs to account for the replacements. The problem is it doesn't take this into consideration in at least a couple places and obscures the replacement a bit by passing the default string through get_theme_mod() in some cases, which handles the replacement itself.

Here are a couple of places where the replacement isn't handled in core:

To see how this manifests itself:

  1. Switch to Twenty Ten or Twenty Thirteen
  2. Delete the header_image and header_image_data theme mods
  3. Access the Customizer
  4. Open the Header Image section

In either theme, the default image that was visible on the front end is missing in the Customizer preview and the "Current header" control doesn't show an image as being set.

If you access the old Custom Header screen in the admin panel (wp-admin/themes.php?page=custom-header), the option to reset the image is visible when it shouldn't be.

I imagine this also affects default images for the custom background feature as well.

Attachments (5)

38557.diff (4.3 KB) - added by bradyvercher 3 years ago.
38557.1.diff (3.7 KB) - added by bradyvercher 3 years ago.
38557.2.diff (3.7 KB) - added by bradyvercher 3 years ago.
38557.3.diff (5.2 KB) - added by bradyvercher 3 years ago.
38557.4.diff (1.5 KB) - added by joemcgill 3 years ago.

Download all attachments as: .zip

Change History (20)

@bradyvercher
3 years ago

#1 @bradyvercher
3 years ago

  • Component changed from General to Themes
  • Keywords has-patch added

The easy fix would be to handle the replacements in the two files mentioned, but in 38557.diff I tried doing the replacements when registering default images instead. That should move a bunch of similar logic to a single location, remove duplicate code, and reduce ambiguity when calling get_theme_support( 'custom-header', 'default-image' ).

If there's any external code that relies on $_wp_default_headers, it could potentially have trouble, but I'd expect that to be an extreme edge case. Otherwise, I can't think of any reason the replacements need to happen when reading the value instead of during assignment.

@bradyvercher
3 years ago

#2 @bradyvercher
3 years ago

38557.1.diff removes a debugging change from the last patch that shouldn't have been included.

@bradyvercher
3 years ago

#3 @bradyvercher
3 years ago

Default arguments are merged late, so 38557.2.diff adds a check to make sure default-image exists before doing the replacements to prevent notices in themes that don't define a default header image.

#4 @pento
3 years ago

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

In 39056:

REST API: Allow a CSV list of user roles to be passed to /users.

After [39048], this changes explicitly parses the list of user roles as slugs, and adds tests.

Props jnylen0.
Fixes #38557.

#5 @pento
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening, I typoed the ticket number.

#6 @jnylen0
3 years ago

The user roles ticket was #38577.

#7 @joehoyle
3 years ago

I made a note on #38553 also, as I think this shouldn't be required now:

I don't think this should have be required now I committed https://core.trac.wordpress.org/changeset/39061 (which I think should probably have been in https://core.trac.wordpress.org/ticket/38586 anyway). Does that sound right to you?

This ticket was mentioned in Slack in #core-restapi by joehoyle. View the logs.


3 years ago

@bradyvercher
3 years ago

#9 @bradyvercher
3 years ago

38557.3.diff moves the replacement logic to get_theme_support(), which has a few advantages:

  • Automatically works for the default image in the custom background feature, too
  • Is easier to unit test since add_theme_support( 'custom-header' ) can't be tested
  • Doesn't modify the format of data stored in the $_wp_theme_features global

The two things that could potentially be considered breaking changes:

  • Calling get_theme_support( 'current-header', 'default-image' ) will return the full URL instead of potentially returning a string with sprintf placeholders. I think this is a bug fix since core expects it to return a full URL in a couple of places.
  • The data stored in $_wp_default_headers is modified on assignment to consolidate the replacement logic for registered default headers. If the global shouldn't be changed, introducing a helper function to retrieve the values would be possible.

This ticket was mentioned in Slack in #core-themes by bradyvercher. View the logs.


3 years ago

#11 @joemcgill
3 years ago

  • Keywords 2nd-opinion added

I appreciate trying to reduce redundancy and the make the logic more clear here, but I'm not sure the best approach is to transform the format of the default-image for custom headers when the setting is registered without at least doing some research into how themes are using the registered data.

Seeing how the purpose of get_theme_support() (based on the function's description) is to get "theme support arguments passed when registering that support", I also don't think we should be doing any data transformations within that function.

As you mentioned, get_theme_mod() handles the sprintf transformation when you pass in a default, but that only helps when the theme mod isn't set. We also seem to process defaults in other places before setting the theme mod (e.g., reset_header_image()) I agree that one option might be to add a helper function for retrieving the default image URL to consolidate the logic here. In the mean time, I think we should add the sprintf logic to the customizer, which is where the this ambiguity has lead to bugs.

@joemcgill
3 years ago

#12 @joemcgill
3 years ago

38557.4.diff fixes the two places in core where we weren't accounting for get_theme_support() returning a string with sprintf placeholders.

This ticket was mentioned in Slack in #core-themes by joemcgill. View the logs.


3 years ago

#14 @joemcgill
3 years ago

In 39123:

Themes: Improve support for placeholders in default headers.

When themes register default headers, they can use sprintf style placeholder
strings in place of the template directory URI, which WordPress transforms
in several places by running the value of
get_theme_support( 'custom-header', 'default-image' ) through sprintf().

This fixes a few places where WordPress skipped the sprintf() step and
referenced the get_theme_support() value directly.

Props bradyvercher for initial patch.
See #38557.

#15 @joemcgill
3 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Owner changed from pento to joemcgill
  • Status changed from reopened to accepted

Let's do 38557.4.diff for now since it addresses the bugginess and we can look into adding a helper function or making the stored data less ambiguous in a future release.

Note: See TracTickets for help on using tickets.