WP_Customize_Setting doesn't clean up after itself

[jon81]

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.

[dlh]

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?

[westonruter]

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' );

[jon81]

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]

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.

[jon81]

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.

[dlh]

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?

[jon81]

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!

[dlh]

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.

[jon81]

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!

[dlh]

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.

[fgiannar]

Hi there,

@dlh I uploaded a patch, trying to do exactly what you described.
Please note this is my first ticket ever, so any comments are more than welcome :)

[dlh]

Welcome @fgiannar, and thanks for the patch!

I made a couple of adjustments to reduce the line length of the comments and specify which class instances are being referred to. Also, when generating diffs using Git, be sure to use the --no-prefix flag for compatibility with SVN checkouts. Otherwise, this looks great to me.

Assigning to mark the good first bug as "claimed." I'm hopeful that this can be included in 5.4.

[fgiannar]

Thanks a lot for your feedback @dlh!
I was thinking maybe your git related advice on --no-prefix could be also added in the ​corresponding documentation about creating patches.
Please let me know if I can provide any additional help!

[SergeyBiryukov]

In 47367:

Docs: Clarify in WP_Customize_Manager::remove_*() methods that removing a setting, panel, section, or control does not destroy the class instance or remove its filters.

Props dlh, fgiannar, jon81, westonruter.
Fixes #48747.