humanmade / Cavalcade

A better wp-cron. Horizontally scalable, works perfectly with multisite.
https://engineering.hmn.md/projects/cavalcade/
Other
515 stars 46 forks source link

`wp_get_schedule()` not supported, causes issues with Jetpack 4.4 #29

Closed dd32 closed 5 years ago

dd32 commented 7 years ago

Currently cavalcade doesn't support wp_get_schedule() properly - it always returns __fake_schedule. This isnt' a problem for WordPress normally, however it has caused issues with the latest Jetpack release.

Consider the following code, boiled down from https://plugins.trac.wordpress.org/browser/jetpack/tags/4.4/sync/class.jetpack-sync-actions.php?marks=295-302#L289

if ( ! wp_next_scheduled( $hook ) ) {
    wp_schedule_event( time(), 'twicedaily', $hook );
} else if ( 'twicedaily' != wp_get_schedule( $hook ) ) { // It was queued as 'daily' previously
    wp_clear_scheduled_hook( $hook );
    wp_schedule_event( time(), 'twicedaily', $hook );
}

Cavalcade will return __fake_schedule for wp_get_schedule() and cause it to endlessly clear the hook and requeue it again.

As we've run into this on WordPress.org, I've temporarily put the following in place: https://github.com/dd32/Cavalcade/commit/fbd23d263521cd4192598deed98706defb8d9bac Unfortunately as pointed out in the commit, this is only a temporary solution, in the event that two schedules with identical timings, but different names, there's a good chance that it'll return the incorrect schedule.

I don't really see a way around this other than storing the schedule name in the database which a job is attached to, that probably means an extra field in the jobs table.

dd32 commented 7 years ago

I've started adding a proper schedule field in https://github.com/dd32/Cavalcade/tree/add-schedule-support

It still needs an upgrade routine, and probably the ability for the plugin to operate on an out-of-date table (unless manually upgrading tables prior to upgrading the plugin is an option).

rmccue commented 7 years ago

We've hit into this as well. @dd32 Is your branch ready for a pull request?

dd32 commented 7 years ago

@rmccue I've been using it locally (not on W.org) successfully without issue (With Jetpack).

It lacks any form of upgrade routine to add the column to the database, and will probably explode if activated on a older schema. Other than that, it should be ready to go, shall I PR it in it's current state?

rmccue commented 7 years ago

Better to file the PR early. :)

As to how we handle upgrades, I'd add detection for the column and use it if we can, trying to avoid errors if possible. We can potentially store a version in network options if needed for performance. The actual upgrade itself I think is best on a CLI command, and we can add a trigger_error( ..., E_USER_NOTICE ) if the version is out of date.

peterwilsoncc commented 5 years ago

A schedule column was added in #62.