Make WordPress Core

Opened 3 years ago

Last modified 10 days ago

#25333 new defect (bug)

Can't remove theme support for certain custom header arguments

Reported by: kovshenin Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.4
Component: Customize Keywords: has-patch
Focuses: Cc:


While working on a ticket I tried to remove_theme_support the custom-header feature and add it back with a different default image, which did not work. Removing theme support *does* remove the feature array from the features global, but adding anything back resurrects the old values ignoring the new ones for:

  • header-text
  • height and width
  • default-text-color
  • default-image

This seems to be happening because of the back-compat code that works with the older constants. It defines the constants if they were not defined, and uses the values next time add_theme_support is called, bringing back the values from the first call to add_theme_support.

The workaround would be to not use remove_theme_support and instead call add_theme_support with the needed values *before* the call you're trying to override, which was sort of intended for child themes, because they run earlier, so this issue is far from major, but still lame. Run this during theme setup to reproduce:

add_theme_support( 'custom-header', array( 'default-image' => '%s/foo.png', ) );
var_dump( get_theme_support( 'custom-header', 'default-image' ) ); // foo.png

remove_theme_support( 'custom-header' );
add_theme_support( 'custom-header', array( 'default-image' => '%s/bar.png', ) );
var_dump( get_theme_support( 'custom-header', 'default-image' ) ); // expected bar.png but got foo.png

Excuse my debugging skills ;)

Attachments (1)

25333.diff (6.6 KB) - added by kovshenin 3 years ago.

Download all attachments as: .zip

Change History (11)

#1 @kovshenin
3 years ago

  • Version set to 3.4

Introduced in [20212]

#2 @nacin
3 years ago

The constants aren't defined until after init. If you need to do any removal, you need to do it before then — on after_setup_theme or on init. Is that what is happening here?

#3 @kovshenin
3 years ago

It looks like the constants are defined during the first call to add_theme_support regardless of when it's called, tested with plugins_loaded. Most likely because of this:

if ( defined( 'HEADER_IMAGE' ) )
    $args[0]['default-image'] = HEADER_IMAGE;
elseif ( isset( $args[0]['default-image'] ) )
    define( 'HEADER_IMAGE', $args[0]['default-image'] );

So the second time you call add_theme_support (even if you called remove_theme_support before that) the values are taken from the defined constants.

#4 @nofearinc
3 years ago

  • Keywords 2nd-opinion added

Confirmed, I can reproduce and the problem here is in the defines (they're indeed triggered in add_theme_support for the first call and just reused for all other calls).

I was thinking of using a global array instead of the defines in add_theme_support and then converting to defines afterwards, but most themes (including default ones) use defines on after_setup_theme.

Maybe nacin has a wiser idea, I see it as wontfix with hooking earlier in the life cycle if that needs to be refined.

#5 @kovshenin
3 years ago

Hooking earlier to change the values actually makes sense, and that behavior was explicitly set in r20212 so that child themes can easily override custom header arguments, since their functions.php is loaded before the parent's. And I'm fine with that.

What bugs me is the fact that after I explicitly remove_theme_support and add it again with different arguments, some of them get rewritten, and others (ones that don't use constants, like flex-width, etc) don't.

3 years ago

#6 @kovshenin
3 years ago

  • Keywords has-patch added; needs-patch 2nd-opinion removed

Sooooo, more back compat code, yey! 25333.diff makes sure that values from defines is read only once and doesn't set any defines before wp_loaded runs. With this approach, child themes can override the parent theme's values by providing them first, either with a define or with add_theme_support.

However, any theme or plugin at any stage can use remove_theme_support and add_theme_support to provide new values, regardless of any defines and prior calls to add_theme_support. Tests included with the patch. And since our tests run after wp_loaded I also created a little plugin to test those defines.

If this works, we can use the same approach for the defines in custom background.

#7 @nacin
3 years ago

  • Component changed from Themes to Appearance

#8 @kovshenin
3 years ago

This blocks unit tests for #25156.

#9 @chriscct7
17 months ago

Patch still applies cleanly

This ticket was mentioned in Slack in #core-customize by melchoyce. View the logs.

10 days ago

Note: See TracTickets for help on using tickets.