WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#36389 closed defect (bug) (fixed)

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

Reported by: scamartist26 Owned by: westonruter
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.6
Component: Customize Keywords: has-patch commit
Focuses: Cc:

Description

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.

Attachments (6)

36389.0.diff (1.5 KB) - added by westonruter 3 years ago.
36389.1.diff (2.2 KB) - added by westonruter 3 years ago.
36389.2.diff (6.6 KB) - added by westonruter 3 years ago.
36389.3.diff (6.6 KB) - added by westonruter 3 years ago.
36389.4.diff (6.6 KB) - added by westonruter 3 years ago.
Removed extraneous unit test groups: https://github.com/xwp/wordpress-develop/pull/149/commits/31965b0163f269f022e2637dce0958c3da060afa
36389.minimized.0.diff (1.9 KB) - added by westonruter 3 years ago.
Minimizing patch to address symptom, with underlying cause (previous patches) to be part of 4.6-early

Download all attachments as: .zip

Change History (39)

#1 @westonruter
3 years ago

  • Keywords reporter-feedback added

@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.

#2 @scamartist26
3 years ago

  • Resolution set to invalid
  • Status changed from new to closed

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

#3 @scamartist26
3 years ago

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

#4 @westonruter
3 years ago

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

#5 @scamartist26
3 years ago

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.

#6 @westonruter
3 years ago

  • Keywords reporter-feedback removed
  • Resolution invalid deleted
  • Status changed from closed to reopened

I can reproduce this issue now.

#7 @westonruter
3 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.5
  • Owner set to westonruter
  • Status changed from reopened to accepted

#8 @westonruter
3 years ago

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
3 years ago

#9 @westonruter
3 years ago

  • Keywords has-patch added; needs-patch removed

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

#10 @westonruter
3 years ago

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
3 years ago

#11 @westonruter
3 years ago

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.

#12 @ocean90
3 years ago

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

@westonruter
3 years ago

#13 @westonruter
3 years ago

  • Keywords has-unit-tests added

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?

#14 @mikeschroder
3 years ago

  • Keywords commit added

This ticket was mentioned in Slack in #core by mike. View the logs.


3 years ago

#16 @jorbin
3 years ago

@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.

This ticket was mentioned in Slack in #core by mike. View the logs.


3 years ago

#18 follow-up: @DrewAPicture
3 years ago

@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.

#19 @DrewAPicture
3 years ago

  • Keywords needs-docs added

#20 in reply to: ↑ 18 ; follow-up: @westonruter
3 years ago

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?

#21 in reply to: ↑ 20 @DrewAPicture
3 years ago

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
3 years ago

#22 @westonruter
3 years ago

  • Keywords needs-docs removed

@DrewAPicture there: 36389.3.diff

This ticket was mentioned in Slack in #core by mike. View the logs.


3 years ago

#24 @ocean90
3 years ago

@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.

#25 @westonruter
3 years ago

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.

This ticket was mentioned in Slack in #core by mike. View the logs.


3 years ago

@westonruter
3 years ago

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

#27 @westonruter
3 years ago

  • Keywords has-unit-tests removed

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.

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


3 years ago

#29 @jorbin
3 years ago

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

#30 @ocean90
3 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

#31 @westonruter
3 years ago

#36431 was marked as a duplicate.

#32 @westonruter
3 years ago

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.

#33 @ocean90
3 years ago

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.

Note: See TracTickets for help on using tickets.