Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#42457 closed defect (bug) (fixed)

Scheduled changeset won't publish if Customizer is opened first after missing schedule

Reported by: littlebigthing's profile LittleBigThing Owned by: westonruter's profile westonruter
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.9
Component: Customize Keywords: has-patch commit dev-reviewed
Focuses: Cc:

Description

There seems to be a bug with publishing scheduled changes for some Customizer settings if the Customizer is opened first after missing the scheduled date.

If I understand correctly from the blog post introducing the changes in WP 4.9, by default, scheduled changesets are autoloaded when the Customizer is loaded (/wp-admin/customize.php). This works correctly. However, when the site has not been visited after the scheduled date is passed and the Customizer is then loaded, the message appears that "Your scheduled changes just published". If then I click through to the Customizer or visit the site the changes are not present for some Customizer settings. This was the case for the site title, description, (text) widgets and the home page setting (blog or page).
However, it seemed to work correctly for changes for the menus, colors and the header image. Sorry for not testing "everything".

When I looked in the posts table in the database, the changes seemed properly cued as a future changeset before opening the Customizer (post_status = 'future'). After opening the Customizer, the changeset had status 'trash'. (Note that I am not aware of the correct structure for the post_content of changesets.)

To reproduce:

  • go to the Customizer
  • change a setting (e.g. site title)
  • schedule the change for a (near) future date (just a couple of minutes)
  • close the Customizer
  • let scheduled time pass
  • open the Customizer (you'll receive the message that scheduled changes are published)
  • go further to the Customizer or check the site directly for the changes

I tested locally with WP 4.9-RC2 using the themes Twenty Fifteen, Sixteen en Seventeen as administrator.

Sorry if I'm missing something and this is no bug...

Attachments (3)

42457.0.diff (1.3 KB) - added by westonruter 7 years ago.
42457.1.diff (1.9 KB) - added by westonruter 7 years ago.
42457.diff (1.7 KB) - added by jeremyfelt 7 years ago.

Download all attachments as: .zip

Change History (17)

@westonruter
7 years ago

#1 @westonruter
7 years ago

  • Keywords has-patch needs-testing added
  • Milestone changed from Awaiting Review to 4.9

I can reproduce this. For anyone who wants to test, it is particularly helpful to add define( 'DISABLE_WP_CRON', true ); to your wp-config.php.

The problem is that wp_publish_post( $changeset_post->ID ) is getting in customize.php called after the settings have already been previewed (after the filters have been added to apply the changeset's values to WordPress). In the case of options particularly, calls to update_option() will short-circuit if the value being updated is the same value that it finds when doing a get_option() call. Since the filters would be supplying the changeset's previewed value at that point, then none of the options would get saved because update_option() would short-circuit for each one. This problem is what we introduced the settings_previewed arg for in #39221. However, in customize.php this argument is not set to false like it is in _wp_customize_publish_changeset() which is normally called when WP Cron publishes a scheduled changeset.

So the fix here in 42457.0.diff is simply to replace the call to wp_publish_post() with a call to spawn_cron(). This will result in a loopback request to wp-cron.php being opened up, and in that request WP_Customize_Manager will not be instantiated with settings_previewed=true, allowing option settings to be saved successfully.

I noticed that all of the settings you described failing to save were in fact of the option type. So that confirms my suspicion.

@LittleBigThing Please test the patch.

This may be the same issue as reported at https://wordpress.org/support/topic/beta-v4-9-twenty-seventeen-does-not-make-any-scheduled-changes-to-customizer/

#2 @LittleBigThing
7 years ago

I tested the above settings (incl. options, but also theme_mods) and it works fine now.
I did some testing without define( 'DISABLE_WP_CRON', true ), too, OK as well.

#3 @westonruter
7 years ago

  • Keywords dev-feedback added; needs-testing removed

#4 @dlh
7 years ago

If ALTERNATE_WP_CRON is enabled, then spawn_cron() will redirect the user back to the Customizer. However, WP_Customize_Manager calls remove_action( 'init', 'wp_cron' ).

In my testing, my changes were not published in this scenario, but maybe I didn't replicate the scenario correctly? ALTERNATE_WP_CRON might not be too common, either.

#5 @westonruter
7 years ago

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

@dlh Thanks very much for testing that. I forgot about ALTERNATE_WP_CRON.

Regardless, if the loopback request fails (which is a common cause for scheduling problems) then spawn_cron() won't do anything either.

The simplest thing I can think of is to open an Ajax request to invoke the customize save method like when hitting Publish in the Customizer. Can you think of a better way?

#6 @dlh
7 years ago

I can't think of anything more straightforward than an Ajax request so far, especially at this point in the 4.9 cycle.

The only alternative I can think of, although I'm not really sold on it, is to take advantage of the fact that the changeset post ID is available before each setting's preview() method is called here: https://github.com/WordPress/wordpress-develop/blob/9f77ec13ffcf1ab14c8699fdb68e728709eb6bb5/src/wp-includes/class-wp-customize-manager.php#L923.

Because the manager knows the changeset ID, it could perform the same "missed schedule" logic that's in customize.php and skip calling preview() if the changeset has missed the schedule, or even call wp_publish_post() itself.

But, apart from being more code than the Ajax request, that approach seems like it would conflict with the idea behind the settings_previewed argument to begin with.

Thinking out loud a bit, maybe in the future WP_Customize_Manager (or WP_Customize_Changeset) could have a "missed schedule" property that _wp_customize_include() could use to switch WP_Customize_Manager::$settings_previewed as needed.

#7 @westonruter
7 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

@dlh @LittleBigThing Please test 42457.1.diff. It uses the Ajax approach. It's also not perfect, as a user could be coming to the Customizer who lacks the capability to publish the changeset. But for 4.9 I think the liklihood of an under-permissioned user visiting the Customizer while also a schedule has been missed to be quite rare, and something that we can look into further after 4.9.

@westonruter
7 years ago

#8 @LittleBigThing
7 years ago

Tested it. Works OK.

About the problem with an underpermissioned user: isn't that a theoretical problem? The planned publish date is already passed here. So if anyone (even without permissions) visited the site, the changes would have been published already.

But maybe I am missing some (future) complexity in how changesets are implemented (like in branched mode).

#9 @westonruter
7 years ago

  • Keywords dev-feedback added; needs-testing removed

@LittleBigThing yes, it is somewhat of a theoretical problem. In fact, a user visiting the site usually causes WP Cron to spawn. It just seems that we should make sure that it is actually getting published when we tell the user that the changes have been published. For example, if DISABLE_WP_CRON is enabled and system cron pings WordPress every minute, then the changeset may not get published for another 59 seconds while waiting for WP Cron to get spawned by system cron. So that's why I think it's important to proactively publish the changes immediately. Another reason for this is that if there is a delay, then if the user clicks “Customize New Changes” then there would be a potential for them to encounter the same missed schedule message again.

#10 @westonruter
7 years ago

  • Keywords commit added

@jeremyfelt
7 years ago

#11 @jeremyfelt
7 years ago

  • Keywords dev-reviewed added; dev-feedback removed

42457.1.diff works as expected and looks good for commit. Creative. +1 :)

I took a stab at tweaking the docs a bit in 42457.diff. No changes to code were made.

#12 @dlh
7 years ago

42457.diff works as expected for me, too.

#13 @westonruter
7 years ago

  • Owner set to westonruter
  • Resolution set to fixed
  • Status changed from new to closed

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.

#14 @westonruter
7 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.