Selective refresh for widgets gets lost once `refresh` is used

[scamartist26]

I added add_theme_support( 'customize-selective-refresh-widgets' );. When editing a text widget all works as expected. However, once a setting is changed that uses refresh transport, the text widget then uses refresh as well.

[westonruter]

@scamartist26 I cannot reproduce the issue. Here are the steps that I took:

First I did:

1. Add a text widget to a widget area.
2. Add a new control with a setting that has a refresh transport.

Then to test:

1. Edit the text widget, and see the selective refresh apply as expected.
2. Edit the new control that has a refresh transport, and note the entire preview refresh as expected.
3. Edit the text widget again, and see the selective refresh apply as expected.

As I understand the issue in your ticket description, step 3 in my test should have instead caused a full refresh instead of a selective refresh. I cannot reproduce this, however.

Please elaborate.

[scamartist26]

Sorry to waste your time @westonruter. In an attempt to track down the issue I have seen problems in other areas of the osfware I am working with. It does not look to be a CORE issue but likely something else. Should I be able to reproduce I will open a ticket. Thanks!
Nathan

[scamartist26]

I wanted to add a note of my findings in case this behavior is seen by others. I placed the following code in a mu-plugins plugin:

<?php
add_action( 'after_setup_theme', function(){ 
        add_theme_support( 'customize-selective-refresh-widgets' );
} );

This worked fine on the initial load in the Customizer, but then once the preview was fully refreshed it would not.
The solution was simply to add a slightly later priority to the action like this:

<?php
add_action( 'after_setup_theme', function(){ 
        add_theme_support( 'customize-selective-refresh-widgets' );
}, 11 );

[westonruter]

@scamartist26 that's strange. I don't see how a later priority would make a difference.

[scamartist26]

I think it has something to do with sidebar registration. I am not sure, but I recreated the behavior with the Twenty Sixteen theme and no active plugins like this:

1. Comment out line 123 from functions.php in Twenty Sixteen
2. Create mu-plugins directory with a {plugin}.php file
3. Add this to file:

   <?php
   add_action( 'after_setup_theme', function() {
           add_theme_support( 'customize-selective-refresh-widgets' );
   });
   

4. Go to Customizer/Widgets/Sidebar and add a text widget with some text. ( Note it refreshes partial )
5. Next go to Customizer/Site Identity and change "Display Site Title and Tagline" to make preview refresh
6. At this point, anything that would do a Partial refresh is lost.
7. If you add the 11 priority to the after_theme_setup action, refresh the Customizer and run the test again you will see it now behaves correctly.

[westonruter]

I can reproduce this issue now.

[westonruter]

OK, I think the issue is this: when you edit a widget, it will be included among the customized state and will be seen when accessing WP_Customize_Manager::unsanitized_post_values(). The WP_Customize_Widgets::register_settings() method is called at after_setup_theme and it ensures that any widgets in the customized state are registered up front. The WP_Customize_Widgets::get_selective_refreshable_widgets() method is getting called before the widgets_init is triggered, and because of this none of the widgets have been registered and so an empty array gets cached.

[westonruter]

Ultimately it looks like the issue is that the widget settings are being registered too early, and that they need to be deferred until widgets_init. @scamartist26 please test 36389.0.diff​

[westonruter]

Seeing a problem with the patch. If the preview is refreshed after a new widget is added, it will not be included among the activeControls and so it will then appear to fade out even though the widget appears in the preview.

[westonruter]

There, 36389.1.diff​ fixes the issue by ensuring the preview filter for sidebars_widgets gets added before the sidebars are iterated for adding the controls.

[ocean90]

Used the steps from comment:5 to reproduce the issue, 36389.1.diff​ fixes it for me.

[westonruter]

I've added unit tests for this issue in 36389.2.diff​. Adding unit tests also necessitated introducing a static WP_Customize_Setting::reset_aggregated_multidimensionals() to reset the protected static $aggregated_multidimensionals variable after each test. (In hindsight, it may have been better to store the $aggregated_multidimensionals as a non-static variable on WP_Customize_Manager so that it would be cleaned up automatically after the manager is destructed.)

Unit tests are running on a PR: ​https://github.com/xwp/wordpress-develop/pull/149

+1 for commit for RC2?

[jorbin]

@DrewAPicture If we want to cause the reset_aggregated_multidimensionals method to not be parsed into to the devhub docs?

If the goal of the function is to only be used by unit tests, I think it makes sense to not expose it in our documentation.

[DrewAPicture]

@jorbin: If we're going to add an @ignore to the DocBlock, the docs should be complete and explicit about what it's supposed to be used for and why it exists. Docs are just as important internally as externally :-)

Side note: I recognize why we need it in core, but it definitely feels a little weird adding a method specifically for testing purposes. Like maybe the underlying code isn't flexible enough in its implementation to be easily testable in the first place. I register my hesitation to make this a habit.

[westonruter]

Replying to DrewAPicture:

Side note: I recognize why we need it in core, but it definitely feels a little weird adding a method specifically for testing purposes. Like maybe the underlying code isn't flexible enough in its implementation to be easily testable in the first place. I register my hesitation to make this a habit.

You're absolutely right. See my comment above:

Adding unit tests also necessitated introducing a static WP_Customize_Setting::reset_aggregated_multidimensionals() to reset the protected static $aggregated_multidimensionals variable after each test. (In hindsight, it may have been better to store the $aggregated_multidimensionals as a non-static variable on WP_Customize_Manager so that it would be cleaned up automatically after the manager is destructed.)

So, what should be done specifically to prevent this method from being included in docs? Let it be like it is right now, but include @ignore?

[DrewAPicture]

Replying to westonruter:

Replying to DrewAPicture:

Side note: I recognize why we need it in core, but it definitely feels a little weird adding a method specifically for testing purposes. Like maybe the underlying code isn't flexible enough in its implementation to be easily testable in the first place. I register my hesitation to make this a habit.

You're absolutely right. See my comment above:

Adding unit tests also necessitated introducing a static WP_Customize_Setting::reset_aggregated_multidimensionals() to reset the protected static $aggregated_multidimensionals variable after each test. (In hindsight, it may have been better to store the $aggregated_multidimensionals as a non-static variable on WP_Customize_Manager so that it would be cleaned up automatically after the manager is destructed.)

So, what should be done specifically to prevent this method from being included in docs? Let it be like it is right now, but include @ignore?

Yep, just add an @ignore tag in there. And I'd change the description from "This is used by unit tests. It should not be used generally." to the more explicit "This is intended only for use by unit tests."

[westonruter]

@DrewAPicture there: 36389.3.diff​

[ocean90]

@westonruter Why are you adding a trac-36389 group to the tests? @ticket 36389 seems to be sufficient.

I'm a bit worried about moving the filter/priorities around late in the cycle, but I couldn't break it while testing it with random themes/plugins and widgets. Otherwise 36389.3.diff​ looks good to me.

[westonruter]

Removed extraneous unit test groups: ​https://github.com/xwp/wordpress-develop/pull/149/commits/31965b0163f269f022e2637dce0958c3da060afa

[westonruter]

I've also ensured that the changes here do not break the functionality and unit tests for the ​Customize Widgets Plus's ​Widget Posts module, which is does quite a bit of low-level integrations. The unit tests for the plugin pass, along with the Core unit tests with the plugin active.

[westonruter]

Minimizing patch to address symptom, with underlying cause (previous patches) to be part of 4.6-early

[westonruter]

I've opened #36431 to apply the full fix for the underlying problem in 4.6.

In the mean time, the fix for the symptom is 36389.minimized.0.diff​.

[jorbin]

After discussing in Slack( see the above link) I am +1 on commit to https://core.trac.wordpress.org/attachment/ticket/36389/36389.4.diff

[ocean90]

In 37166:

Customize: Harden assignment of Customizer settings transports for selective refreshable widgets

Theme support for customize-selective-refresh-widgets can be added _after_ the logic for registering the settings for incoming widgets that have been changed. This is due to themes adding the theme support in after_setup_theme which is also the action where WP_Customize_Widgets::register_settings() is called. If these both happen at priority 10, which one is called first depends on which one was added first. The other issue is that at the time that WP_Customize_Widgets::register_settings() is called at after_setup_theme, it is called before widgets_init and thus no widgets are yet registered. This means that any settings registered at this point will always have a refresh transport even if the theme supports customize-selective-refresh-widgets, since the WP_Widget instance is not visible yet to see if it supports selective refresh.

The fix: Defer WP_Customize_Widgets::register_settings() from after_setup_theme to widgets_init at priority 95 when the widget objects have all been registered. Also, ensure that the preview filter for sidebars_widgets is added before the sidebars are iterated for adding the controls.

Props westonruter.
Fixes #36389.

[westonruter]

#36431 was marked as a duplicate.

[westonruter]

In 37352:

Customize: Handle filtering sidebars_widgets when the underlying option is non-existent.

Fixes warning related to a non-array argument for array_merge() in WP_Customize_Widgets::customize_register().

See [37166].
See #36389.
Fixes #36660.

[ocean90]

In 37453:

Customize: Handle filtering sidebars_widgets when the underlying option is non-existent.

Fixes warning related to a non-array argument for array_merge() in WP_Customize_Widgets::customize_register().

Merge of [37352] to the 4.5 branch.

See [37166].
See #36389.
Fixes #36660.