Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
dd32 opened this issue Nov 22, 2016 · 5 comments
Closed

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

dd32 opened this issue Nov 22, 2016 · 5 comments

Comments

@dd32
Copy link
Contributor

dd32 commented Nov 22, 2016

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: dd32@fbd23d2
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
Copy link
Contributor Author

dd32 commented Dec 31, 2016

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
Copy link
Member

rmccue commented Jan 10, 2017

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

@dd32
Copy link
Contributor Author

dd32 commented Jan 10, 2017

@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
Copy link
Member

rmccue commented Jan 10, 2017

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
Copy link
Contributor

A schedule column was added in #62.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants