Format for registering a default header image is ambiguous

[bradyvercher]

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:

• ​wp-includes/class-wp-customize-manager.php
• ​wp-admin/custom-header.php

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.

[bradyvercher]

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]

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

[bradyvercher]

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.

[pento]

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.

[pento]

Re-opening, I typoed the ticket number.

[jnylen0]

The user roles ticket was #38577.

[joehoyle]

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?

[bradyvercher]

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.

[joemcgill]

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]

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

[joemcgill]

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.

[joemcgill]

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.