WordPress.org

Make WordPress Core

Opened 2 weeks ago

Last modified 4 days ago

#48747 assigned defect (bug)

WP_Customize_Setting doesn't clean up after itself

Reported by: jon81 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: minor Version: 3.4
Component: Customize Keywords: needs-patch good-first-bug
Focuses: Cc:
PR Number:

Description

I've been working on a child theme for twentytwenty, and needed to replace the sanitize_callback on one of the customize settings registered in the parent theme.

I figured I could just delete the setting and re-create it with my own callback:-

// child theme
add_action( 'customize_register',  function( $wp_customize )
{
    $wp_customize->remove_setting( 'header_footer_background_color' );
    $wp_customize->add_setting( 'header_footer_background_color', array(
        'default'           => '#ffffff'
        'sanitize_callback' => 'my_sanitize_callback',
        'transport'         => 'postMessage',
    ) );

}, 20 );

function my_sanitize_callback( $value ) {
    // sanitize $value
    return $value;
}

.. but the $value in my callback was empty and I couldn't figure out why until I dug a little deeper. The callback set in the parent theme was still being called before my own callback.

WP_Customize_Setting has no destructor and doesn't remove the filters/actions added during it's construction, so my call to $wp_customize->remove_setting() was not removing the previous sanitize callback.

Attachments (1)

48747.diff (1.8 KB) - added by dlh 2 weeks ago.

Download all attachments as: .zip

Change History (11)

#1 follow-up: @dlh
2 weeks ago

  • Focuses template removed
  • Keywords reporter-feedback added
  • Version changed from 5.3 to 3.4

Hi @jon81, and welcome to WordPress Trac! Thanks for the report.

Putting aside the behavior of remove_setting() for a moment: If the setting is being replaced to use a custom sanitizing callback, is there a reason the customize_sanitize_{$id} filter can't be used instead?

I'm not sure whether there's a historical reason that the WP_Customize_Manager::remove_*() methods don't attempt to remove any hooks associated with the removed constructs. @westonruter sorry for the ping, but do you know?

#2 in reply to: ↑ 1 ; follow-up: @westonruter
2 weeks ago

Replying to dlh:

I'm not sure whether there's a historical reason that the WP_Customize_Manager::remove_*() methods don't attempt to remove any hooks associated with the removed constructs. @westonruter sorry for the ping, but do you know?

Hi! I think it's just because it's not a need that came up. I thought there was some ticket for it, but I couldn't find it.

It's not great that the constructor for WP_Customize_Setting is calling add_filter() right away. It would be better if those filters were added later once they would be used. Alas.

In any case, I suggest removing the filters. Assuming there is only a sanitize_callback you can just do:

<?php
remove_all_filters( 'customize_sanitize_header_footer_background_color' );

#3 in reply to: ↑ 2 @jon81
2 weeks ago

Replying to dlh:

is there a reason the customize_sanitize_{$id} filter can't be used instead?

Hi! I did look into removing the filter manually, though I'd need to remove all filters as @westonruter mentioned above. I've also considered just creating a new setting as a proxy or replacing the twentytwenty customize_register hook entirely.

Replying to westonruter:

In any case, I suggest removing the filters. Assuming there is only a sanitize_callback you can just do:

<?php
remove_all_filters( 'customize_sanitize_header_footer_background_color' );

Thanks! I think this is what I may end up doing.

I imagine there isn't a great need for WP_Customize_Setting to clean up, it was just a frustrating afternoon trying to figure out why remove_setting() wasn't behaving as I expected.

@dlh
2 weeks ago

#4 follow-up: @dlh
2 weeks ago

  • Keywords dev-feedback has-patch added

I can understand why that would be frustrating, for sure. Thinking about it more, though, I wonder whether the current behavior is preferable given that an instance of WP_Customize_Setting can be associated with more than one instance of WP_Customize_Manager (see #40527.) Same goes for controls, sections, and panels. So, I'm not sure that removing a setting from one instance of a manager necessarily means the setting should be torn down.

I threw together 48747.diff as a possible change to provide a workaround: Returning the removed construct to the developer for further processing, such as removing hooks. I'm not sure I'm sold on it, though. It's harmless enough, but would it have been all that useful in your case, @jon81?

Perhaps instead we should call this maybelater.

#5 in reply to: ↑ 4 ; follow-up: @jon81
2 weeks ago

Hey @dlh, to give a bit of context to what I was working on:-

I wanted to replace some of the color controls defined in the parent theme, with a control that supports rgba (Alpha Color Picker), I was coming unstuck with sanitize_hex_color running before/instead-of my own sanitize callback.

The patch you suggested would make it easier to remove the sanitize filter, though I feel it's use would only be apparent after one has discovered the issue at hand. Now that I know what the problem is I can workaround it myself.

I think what Weston suggested would be the way to go if possible, have filters added when needed, rather than in the constructor.. that way I could presumable remove a setting in the customize_register hook before any filters have been added.

Last edited 2 weeks ago by jon81 (previous) (diff)

#6 in reply to: ↑ 5 ; follow-up: @dlh
2 weeks ago

  • Keywords needs-patch added; dev-feedback has-patch reporter-feedback removed

Replying to jon81:

I think what Weston suggested would be the way to go if possible, have filters added when needed, rather than in the constructor.. that way I could presumable remove a setting in the customize_register hook before any filters have been added.

True enough. But, it would come with some backwards-compatibility challenges, I think. For example, the remove_all_filters() approach would no longer work as early in the Customize bootstrap process as it does now if the setting filters were added later.

How about, at least, describing this gotcha in the docs for the remove_*() methods? I'm thinking it could say exactly what's in the ticket description: that removing a construct from a manager doesn't destroy the class instance or "tear down" its filters. Would you be interested in writing a patch for that, @jon81?

#7 in reply to: ↑ 6 @jon81
2 weeks ago

Replying to dlh:

How about, at least, describing this gotcha in the docs for the remove_*() methods? I'm thinking it could say exactly what's in the ticket description: that removing a construct from a manager doesn't destroy the class instance or "tear down" its filters. Would you be interested in writing a patch for that, @jon81?

Absolutely, that sounds like a good idea to me!

#8 follow-up: @dlh
7 days ago

  • Keywords good-first-bug added
  • Owner set to jon81
  • Status changed from new to assigned

Since the focus of this ticket for the time being is the docs update, marking this as a good-first-bug and assigning it as "claimed."

@jon81, if it turns out you don't think you'll be able to upload a patch here, no worries at all. Just reply back and you'll be un-assigned.

#9 in reply to: ↑ 8 @jon81
5 days ago

Hi @dlh,

Sorry for the late reply! I didn't realise you were waiting on me to write a patch, I'd prefer to be unassigned for the time being.. as I don't have any experience writing patches and I'm very busy leading up to the end of year, as are we all no doubt?

Thankyou for all your help, and I wish you the best in the coming holidays!

#10 @dlh
4 days ago

  • Owner jon81 deleted

No problem, @jon81. If you do get the urge to write the patch after all, the core handbook has great documentation on setting up a development environment and working with patches.

Note: See TracTickets for help on using tickets.