Fix recurring schedules, e.g. Cron and Interval schedules #333

thenbrent · 2019-07-19T06:48:40Z

This is a monster, late stage PR for 3.0.0. However, because there are breaking changes, and it resolves a number of discrepancies and gotchas in the recurring and cron actions & schedules, I think it's worth including in 3.0.0 instead of waiting for a 4.0.0, or shipping in a 3.x.x version.

Fixes #330 and associated issues - #256, #258, #291, #293 and #296.

As outlined in #330, there are a number of issues with recurring action that use either a Cron or Interval schedule.

This PR fixes recurring schedules, and in the process, refactors all schedules to avoid duplicate code and ambiguity of properties and parameters.

This patch introduces two important new base classes:

ActionScheduler_Abstract_Schedule and
ActionScheduler_Abstract_RecurringSchedule

These are then used to implement all other schedule types - cron, recurring/interval and simple. This helps unify the data on each schedule. For example, schedules used to have different property names referring to equivalent data like ActionScheduler_IntervalSchedule::start_timestamp and ActionScheduler_SimpleSchedule::timestamp.

Both of these timestamps actually referring to the date the action was scheduled, but due to ambiguity in naming, like "start" or "date" or "first", the data was sometimes used to refer to the scheduled date, and sometimes the first instance of a recurring schedule. While unifying the properties, their nomenclature has also been changed to avoid ambiguity.

While working on this, I also took the opportunity to:

harden safeguards when next instances are scheduled to make only recurring action are ever rescheduled
introduce a new ActionScheduler_CanceledSchedule class which takes advantage of the separation of scheduled / next dates on a schedule so that a cancelled action's scheduled date can be derived (and displayed in the admin)
improve docblocks to better record any gotchas, like time slippage when rescheduling an instance of a recurring action
add more unit tests, particularly to test cases of 2...n instances of a recurring action
log issues rescheduling a recurring action

thenbrent · 2019-07-24T06:17:36Z

I've rebased this on version_3_0_0 now that #259 has been merged. One of the new benefits of this changeset I found while merging in that code is that it's possible to migrate cancelled actions now, because we can access their scheduled date via the new ActionScheduler_CanceledSchedule. Previously, they were simply deleted from the old data store instead of being migrated.

rrennick · 2019-07-24T14:18:10Z

@thenbrent This is looking good. Just a couple things:

Related to #331, we should start ensuring any new code merged has consistent spacing. The WP coding standards for spacing provide for good readability. I used the following to find some of the inconsistent spacing (in added code only)

$ git show dfb1f11a10fec59c9e6d590d7a2c037ff5eb6a60 | grep ^+ | grep \( | grep -v '( '
+	public function test_next_instance_of_interval_action() {
+		$random    = md5(rand());
+		$date      = as_get_datetime_object('12 hours ago');
+		$store     = ActionScheduler::store();
+		$claim = $store->stake_claim(10, $date);
+		$actions = $claim->get_actions();
+		$this->assertCount(1, $actions);
+		$fetched_action_id = reset($actions);
+		$fetched_action    = $store->fetch_action($fetched_action_id);
+		$store->release_claim($claim);
+		$runner->run();
+		$date = as_get_datetime_object('+1 day');
+		$claim = $store->stake_claim(10, $date);
+		$fetched_action_id = reset($actions);
+		$fetched_action    = $store->fetch_action($fetched_action_id);
+		$store->release_claim($claim);
+		$date = as_get_datetime_object('+1 day');
+		$claim = $store->stake_claim(10, $date);
+		$actions = $claim->get_actions();
+		$this->assertCount(1, $actions);
+		$fetched_action_id = reset($actions);
+		$fetched_action    = $store->fetch_action($fetched_action_id);

I'm getting a warning in WP CLI and in the debug log:

$ wp option get siteurl
Notice: ActionScheduler_Schedule_Deprecated::next is <strong>deprecated</strong> since version 3.0.0! Use ActionScheduler_Schedule_Deprecated::get_date() instead. in /wp-includes/functions.php on line 4435
https://ron.localhost

thenbrent · 2019-07-26T11:28:42Z

we should start ensuring any new code merged has consistent spacing

@rrennick good call. I've fixed up spacing and rebased in the fixes.

I ran git show HEAD...a4934fcea90 | grep ^+ | grep \( | grep -v '( ' to review the whole diff.

I'm getting a warning in WP CLI and in the debug log:

I can't see any remaining calls to ActionScheduler_Schedule_Deprecated::next(). Is that notice coming from Action Scheduler Admin plugin?

rrennick · 2019-07-30T18:33:24Z

I can't see any remaining calls to ActionScheduler_Schedule_Deprecated::next(). Is that notice coming from Action Scheduler Admin plugin?

Yes. Other than a rebase this is good to go.

Recurring schedules issues, as outlined in #330. This patch fixes recurring schedules, and in the process, refactors all schedules to avoid duplicate code and ambiguity of properties and parameters. This patch introduces to important new base classes: * ActionScheduler_Abstract_Schedule and * ActionScheduler_Abstract_RecurringSchedule These are then used to implement all other schedule types - cron, recurring/interval and simple. This helps unify the data on each schedule. For example, schedules used to have different property names referring to equivalent data. For example, ActionScheduler_IntervalSchedule::start_timestamp was the same as ActionScheduler_SimpleSchedule::timestamp. Both of these timestamps actually referring to the date the action was scheduled, but due to ambiguity in naming, like 'start' or 'date' or 'first', the data was sometimes used to refer to the scheduled date, and sometimes the first instance of a recurring schedule. While unifying the properties, their nomenclature has also been changed to avoid ambiguity. Fixes #330 and associated issues - #256, #258, #291, #293 and #296.

Preserve prior behaviour where first run date is the next matching Cron date by calculating this in the factory method. Also make it more clear this is the behaviour of the $first / $timestamp param, by renaming them and improving php docblock. Also add a new unit test to ensure successive instances of cron actions are scheduled correcting aftering being run /processed.

By making sure that the $start date is used if it matches the cron expression, rather than forcing a date after $start.

Now that the scheduled date and future / next date calculations have been separated, we can create a new schedule which will still return a cancelled action's scheduled date, but never show a cancelled action as having a next date. Previously, cancelled actions used the Null schedule, which would always return NULL for both the next scheduled date, and the scheduled date of the action, meaning we could never find out when a cancelled action was originally scheduled to run.

Improvements: 1. test 3 instances not just 2, just in case 2. test both run() and process_action() methods 3. rename to disambiguate from the new test for cron actions

Instead of duplicating the same logic.

thenbrent · 2019-07-31T05:30:29Z

Rebased, thanks @rrennick!

rrennick

LGTM

Prior to #333, reccuring schedules used different property names to refer to equivalent data. For example, IntervalSchedule::start_timestamp was the same as SimpleSchedule::timestamp. PR #333 aligned properties and property names for better inheritance. To guard against the possibility of infinite loops if downgrading to Action Scheduler < 3.0.0, we need to also store the data with the old property names so if it's unserialized in AS < 3.0, the schedule doesn't end up with a 0 recurrance, which creates an infinite loop. Fixes #340.

Prior to #333, recurring schedules used different property names to refer to equivalent data. For example, IntervalSchedule::start_timestamp was the same as SimpleSchedule::timestamp. PR #333 aligned properties and property names for better inheritance. To guard against the possibility of infinite loops if downgrading to Action Scheduler < 3.0.0, we need to also store the data with the old property names so if it's unserialized in AS < 3.0, the schedule doesn't end up with a 0 recurrence. Fixes #340.

thenbrent mentioned this pull request Jul 19, 2019

Recurring actions admin view bug #262

Closed

thenbrent added this to the 3.0.0 milestone Jul 19, 2019

thenbrent added the type: bug The issue is a confirmed bug. label Jul 19, 2019

This was referenced Jul 19, 2019

Tweak fetch exception handling #328

Merged

Add database tables #259

Merged

thenbrent force-pushed the issue_330 branch 5 times, most recently from 8c0e2cc to 8fdc9f2 Compare July 24, 2019 06:08

thenbrent mentioned this pull request Jul 24, 2019

Use factory()->get_stored_action() in DBStore #339

Closed

thenbrent force-pushed the issue_330 branch 2 times, most recently from c8c93de to 3fb9454 Compare July 26, 2019 11:22

thenbrent mentioned this pull request Jul 30, 2019

Recurring schedule interval 0 #340

Closed

crstauf and others added 2 commits July 31, 2019 14:16

init #256

e10346c

Only reschedule recurring actions

809a897

thenbrent force-pushed the issue_330 branch from 3fb9454 to 77e9a1c Compare July 31, 2019 04:17

thenbrent added 7 commits July 31, 2019 14:46

Log errors when rescheduling a recurring action fails

9adcabe

Fix cron repeat() / rescheduleding

294c47e

By making sure that the $start date is used if it matches the cron expression, rather than forcing a date after $start.

Improve test for next recurring / interval action

95bab79

Improvements: 1. test 3 instances not just 2, just in case 2. test both run() and process_action() methods 3. rename to disambiguate from the new test for cron actions

Use factory()->get_stored_action() in DBStore

646be3d

Instead of duplicating the same logic.

thenbrent force-pushed the issue_330 branch from 77e9a1c to 646be3d Compare July 31, 2019 05:30

rrennick approved these changes Jul 31, 2019

View reviewed changes

rrennick merged commit d386c2a into version_3_0_0 Jul 31, 2019

delete-merged-branch bot deleted the issue_330 branch July 31, 2019 17:29

This was referenced Aug 2, 2019

Recurring actions admin view bug #256

Closed

Bug in recurring schedule #330

Closed

thenbrent mentioned this pull request Aug 6, 2019

Maintain property names for serialized schedules #345

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix recurring schedules, e.g. Cron and Interval schedules #333

Fix recurring schedules, e.g. Cron and Interval schedules #333

thenbrent commented Jul 19, 2019 •

edited

Loading

thenbrent commented Jul 24, 2019

rrennick commented Jul 24, 2019

thenbrent commented Jul 26, 2019

rrennick commented Jul 30, 2019

thenbrent commented Jul 31, 2019

rrennick left a comment

Fix recurring schedules, e.g. Cron and Interval schedules #333

Fix recurring schedules, e.g. Cron and Interval schedules #333

Conversation

thenbrent commented Jul 19, 2019 • edited Loading

thenbrent commented Jul 24, 2019

rrennick commented Jul 24, 2019

thenbrent commented Jul 26, 2019

rrennick commented Jul 30, 2019

thenbrent commented Jul 31, 2019

rrennick left a comment

Choose a reason for hiding this comment

thenbrent commented Jul 19, 2019 •

edited

Loading