Sync: Callables: Allow shutdown sync on CLI commands

[nickdaugherty]

Steps to reproduce the issue

1. Run wp option update home https://some-new-domain.com
2. Check the home option on WP.com / Jetpack side - it's unchanged
3. "Manually" sync callables (such as via the JP Debug tool)
4. home is now correct on WP.com / Jetpack side

What I expected

Like other content changes that are synced on shutdown, changing options that are handled via "functions/callables" should also cause a sync on shutdown.

What happened instead

The new value was not synced until a sync was manually triggered.

This is because of the conditional here:

jetpack/packages/sync/src/modules/Callables.php

Line 219 in 40e18f0

if ( ! is_admin() || Settings::is_doing_cron() ) {

Related - it also bails on Cron, which doesn't seem great, because then callables are only synced on admin requests - is that still desired?

cc @Automattic/poseidon

[gititon]

Hi @nickdaugherty,

I wanted to let you know that I've been working on this issue. At this point it's a matter of getting all of the tests passing, which I hope to finish tomorrow.

[tyxla]

Hey @nickdaugherty we've also got the tests to pass, so the PR is also in a testable state right now.

We'd appreciate your help in testing/reviewing the PR in order to ship it and resolve the issue as soon as possible.

[gititon]

@nickdaugherty #13015 has been merged, so this should work now.

[nickdaugherty]

Thanks @gititon, much appreciated!

[nickdaugherty]

Hey @gititon, I did some testing and I don't think it's quite fixed. The changes come through eventually (I'm guessing through cron), but not in direct response to the CLI command itself, it seems.

I left a note on the PR, I think we're still not properly doing the sync on shutdown for CLI commands, b/c it aborts if the request isn't Cron (which WP CLI is not).

[gititon]

Hi @nickdaugherty , can you please try again on master and confirm whether the latest changes do the trick? Thanks!

[nickdaugherty]

Hey @gititon, I just retested on fresh build of master, and there are 2 things not working right for me:

The home option syncs correctly and instantly the first 2 times I run it. However, it then stops syncing once the jetpack_sync_callables_await transient exists (which I guess is by design?). The problem is, if a site has that lock for something else, then you run wp option update home https://my-new-domain.com, the new value doesn't get synced, so the syncing is unreliable. It syncs again once the transient expires (60s).

The other problem is that siteurl doesn't seem to be updating at all. I think that's because we also need to map siteurl => site_url in this mapping (?):

jetpack/packages/sync/src/modules/Callables.php

Line 66 in 3d9e58d

'home' => 'home_url',

wp option update siteurl https://my-new-domain.com doesn't sync the new value up.

[gititon]

Thanks for your feedback, @nickdaugherty. I'll prepare another patch in the morning to address site_url and give the wait time some thought. For context, can you please tell me why we need these options to bypass normal sync constraints and always sync immediately?

[gititon]

Hi @nickdaugherty,

I have a pull request in that will let siteurl sync as well, and ensures that both siteurl and home sync instantly when changed via WP CLI, #13132.

[gititon]

Hi @nickdaugherty, #13132 has been merged, so you should have the additional functionality you were looking for now.

[nickdaugherty]

Awesome, I'll try it out as soon as I can. To answer your question, the use case is launching a new site - site has a "placeholder" domain up until the point of DNS switching, then DNS is flipped and the home/siteurls are updated back to back...so it'll start receiving traffic and it's important the cache site is in sync. Technically a delay of a minute or two may be ok, but IMO better to do it straight away.

[gititon]

Roger that, thanks for filling me in, @nickdaugherty. Please let me know how it works for you when you have a chance.

[jeherve]

Closing this for now since #13132 has been merged. @nickdaugherty Feel free to reopen if there are still issues in your testing.

[nickdaugherty]

Awesome, tested it out and it seems much better. Changing the home/siteurl several times in a row occasionally ended up with out-of-date data on the cache site, but it does seem much more reliable (and siteurl now syncs). Thanks for working on this!