WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#39221 closed defect (bug) (fixed)

Customize: Prevent setting previewing logic when settings will be saved

Reported by: westonruter Owned by: westonruter
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: has-patch has-unit-tests commit
Focuses: Cc:
PR Number:

Description

When a setting is previewed this may very well cause a save to fail because WP may detect that the value is already set in the database (as in the case of options) and thus no-op. Or in the case of deleting a nav menu in the customizer, the preview filter causes the nav menu object to fail to be returned if the preview filter is applied (see #39103).

The way this has been accounted for in core up until now is to prevent calling WP_Customize_Setting::preview() when a customize_save admin ajax request is being done. This is not ideal, however, as it is too closely bound to the existing customize.php app and is not generic enough for other applications which may be built for live preview, such as REST API (#38900), although to be sure it is unlikely that the customizer preview logic would be booted when manipulating a changeset endpoint.

See https://core.trac.wordpress.org/attachment/ticket/39103/39103.1.dff for one possible approach, where a non-previewing WP_Customize_Manager instance could be made.

Attachments (3)

39221.diff (7.4 KB) - added by stubgo 2 years ago.
39221.1.diff (7.4 KB) - added by stubgo 2 years ago.
Updated with a fix.
39221.2.diff (10.1 KB) - added by westonruter 2 years ago.
https://github.com/xwp/wordpress-develop/pull/231

Download all attachments as: .zip

Change History (22)

#1 @celloexpressions
3 years ago

  • Keywords needs-patch added
  • Type changed from enhancement to defect (bug)

This can probably be considered a bug, as its manifestations will be bugs and the underlying issue here should be fixed.

#2 @westonruter
3 years ago

  • Milestone changed from 4.8 to Future Release

I think that this actually won't be necessary per:

it is unlikely that the customizer preview logic would be booted when manipulating a changeset endpoint

@stubgo @dlh thoughts re: https://github.com/WP-API/wp-api-customize-endpoints

#3 @stubgo
2 years ago

@westonruter When manipulating the changeset via an endpoint the WP_Customize_Manager is initialized per each changeset and also the WP_Customize_Setting::preview() is called for relevant settings. It seems that when a changeset is created for deleting a menu and then this changeset is published, the menu is not actually deleted.

Seems that it does need preventing previewing logic. (Thanks for tagging me here.)

#4 @westonruter
2 years ago

  • Milestone changed from Future Release to 4.8.1

@stubgo excellent. Great feedback. I wonder what the best solution would be for this. Maybe there should be a skip_setting_preview argument that is added to the the WP_Customize_Manager constructor args that when supplied would be checked for whether or not preview should be called on each setting instead of hackily looking at the current Ajax action? The argument could default to false, but then in _wp_customize_include() it could then look at supplying the skip_setting_preview arg if doing the ajax action or something. Then in the changeset endpoint, where you are directly instantiating WP_Customize_Manager you would then explicitly pass in skip_setting_preview=>true. Seems like that would work but I'm not sure it is the most elegant. Thoughts?

Milestoning for 4.8.1 to match #38900.

#5 @stubgo
2 years ago

@westonruter Sounds like adding an additional argument is reasonable. Maybe it would also make sense to create a method WP_Customize_Manager::maybe_preview_settings() (or something similar) and instead of making a check separately in WP_Customize_Nav_Menus and WP_Customize_Widgets etc. just call $this->manager->maybe_preview_settings( $settings )?

Couldn't think of a better alternative to adding an additional argument to WP_Customize_Manager for skipping the preview.

#6 @westonruter
2 years ago

Yeah, either that or perhaps it would be easier to add a method like WP_Customize_Manager::should_preview_settings() which could be checked before a component calls WP_Customize_Setting::preview().

#7 @stubgo
2 years ago

Note: starting working on the issue to implement the discussed approach.

#8 @stubgo
2 years ago

@westonruter Created the initial PR:
https://github.com/xwp/wordpress-develop/pull/231

Github diff:
https://github.com/xwp/wordpress-develop/pull/231.diff

Tested that it works in Customizer and for the customize changeset endpoint as well.

#9 @westonruter
2 years ago

  • Keywords has-patch added; needs-patch removed
  • Owner set to stubgo
  • Status changed from new to assigned

#10 @westonruter
2 years ago

  • Keywords needs-testing added

@stubgo
2 years ago

@stubgo
2 years ago

Updated with a fix.

#11 @utkarshpatel
2 years ago

Tested works fine.

#12 @westonruter
2 years ago

  • Owner changed from stubgo to westonruter
  • Status changed from assigned to reviewing

#13 @westonruter
2 years ago

  • Milestone changed from 4.8.1 to 4.9

#14 @westonruter
2 years ago

Anne Louise Currie found a defect in core related to this, specifically in publishing a changeset from outside the Customizer, such as via the edit post screen as provided by the Customize Snapshots plugin. In particular, publishing changes to options will fail due to the preview filters being added.

I have amended the PR with a fix: https://github.com/xwp/wordpress-develop/pull/231/commits/3e4dea479df35a8e3b046a2509566c4a95657a91

#15 @westonruter
2 years ago

  • Keywords has-unit-tests commit added; needs-testing removed
  • Version set to 4.7

39221.2.diff incorporates unit test from https://github.com/xwp/wp-customize-snapshots/pull/137

Any last feedback/tweaks before I commit?

#16 @stubgo
2 years ago

Weston: no additional comments from me, thank you for the fix.

#17 @westonruter
2 years ago

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

In 41205:

Customize: Introduce settings_previewed arg and getter on WP_Customize_Manager which controls whether WP_Customize_Setting::preview() should be called on settings.

The settings_previewed property eliminates the need for the Customizer components from having to look at global doing_ajax state. This is in particular needed when saving settings, as some settings will short-circuit the update operation if they detect no changes are introduced. This is also needed for low-level integrations with the Customizer, such as in REST API endpoints under development.

Props stubgo, westonruter, utkarshpatel for testing.
See #38900.
Fixes #39221.

#18 @westonruter
2 years ago

In 42138:

Customize: Fix reliability of just-in-time publishing for changesets that miss their schedule when visiting customize.php.

When just doing wp_publish_post() for the changeset from customize.php, any option-based settings will fail to get saved because WP_Customize_Manager would have already been loaded with settings_previewed, resulting in update_option() calls being short-circuited. So an admin-ajax request to customize_save is used to work around this.

Props westonruter, jeremyfelt, dlh for testing, LittleBigThing for testing.
Amends [41626].
See #28721, #39221.
Fixes #42457 for trunk.

#19 @westonruter
2 years ago

In 42139:

Customize: Fix reliability of just-in-time publishing for changesets that miss their schedule when visiting customize.php.

When just doing wp_publish_post() for the changeset from customize.php, any option-based settings will fail to get saved because WP_Customize_Manager would have already been loaded with settings_previewed, resulting in update_option() calls being short-circuited. So an admin-ajax request to customize_save is used to work around this.

Props westonruter, jeremyfelt, dlh for testing, LittleBigThing for testing.
Amends [41626].
See #28721, #39221.
Fixes #42457 for 4.9.

Note: See TracTickets for help on using tickets.