WordPress.org

Make WordPress Core

Opened 20 months ago

Last modified 14 months 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:

Description

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 18 months ago.

Download all attachments as: .zip

Change History (9)

comment:1 @kovshenin20 months ago

  • Version set to 3.4

Introduced in [20212]

comment:2 @nacin20 months 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?

comment:3 @kovshenin20 months 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.

comment:4 @nofearinc20 months 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.

comment:5 @kovshenin20 months 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.

@kovshenin18 months ago

comment:6 @kovshenin18 months 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.

comment:7 @nacin15 months ago

  • Component changed from Themes to Appearance

comment:8 @kovshenin14 months ago

This blocks unit tests for #25156.

Note: See TracTickets for help on using tickets.