mirror_php/moodle
1
0
Fork 0
mirror of https://github.com/moodle/moodle.git synced 2025-01-18 22:08:20 +01:00
Code Issues Projects Releases Wiki Activity

MDL-79966 core_task: Scheduled task timing (crontab fields) is wrong

Browse Source
This commit is contained in:
sam marshall 2023-11-01 16:42:55 +00:00
parent 94ad185d09
commit 47a38fd827
3 changed files with 280 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
admin/tool/task/tests/form_test.php
View File

@ -236,6 +236,8 @@ class form_test extends \advanced_testcase {
$checker->set_day_of_week('6');
$this->assertTrue($checker->is_valid($checker::FIELD_DAYOFWEEK));
$checker->set_day_of_week('7');
$this->assertTrue($checker->is_valid($checker::FIELD_DAYOFWEEK));
$checker->set_day_of_week('8');
$this->assertFalse($checker->is_valid($checker::FIELD_DAYOFWEEK));
$checker->set_day_of_week('20');
$this->assertFalse($checker->is_valid($checker::FIELD_DAYOFWEEK));
@ -247,7 +249,7 @@ class form_test extends \advanced_testcase {
$checker->set_day_of_week('*/6');
$this->assertTrue($checker->is_valid($checker::FIELD_DAYOFWEEK));
$checker->set_day_of_week('*/7');
$this->assertFalse($checker->is_valid($checker::FIELD_DAYOFWEEK));
$this->assertTrue($checker->is_valid($checker::FIELD_DAYOFWEEK));
$checker->set_day_of_week('*/13');
$this->assertFalse($checker->is_valid($checker::FIELD_DAYOFWEEK));
$checker->set_day_of_week('*/35');
@ -271,6 +273,8 @@ class form_test extends \advanced_testcase {
$checker->set_day_of_week('65-2');
$this->assertFalse($checker->is_valid($checker::FIELD_DAYOFWEEK));
$checker->set_day_of_week('3-7');
$this->assertTrue($checker->is_valid($checker::FIELD_DAYOFWEEK));
$checker->set_day_of_week('3-8');
$this->assertFalse($checker->is_valid($checker::FIELD_DAYOFWEEK));
}
}

171
lib/classes/task/scheduled_task.php
View File

@ -55,6 +55,8 @@ abstract class scheduled_task extends task_base {
const DAYOFWEEKMIN = 0;
/** Maximum dayofweek value. */
const DAYOFWEEKMAX = 6;
/** Maximum dayofweek value allowed in input (7 = 0). */
const DAYOFWEEKMAXINPUT = 7;
/**
* Minute field identifier.
@ -77,6 +79,11 @@ abstract class scheduled_task extends task_base {
*/
const FIELD_DAYOFWEEK = 'dayofweek';
/**
* Time used for the next scheduled time when a task should never run. This is 3000-01-01 00:00 GMT.
*/
const NEVER_RUN_TIME = 32503680000;
/** @var string $hour - Pattern to work out the valid hours */
private $hour = '*';
@ -333,13 +340,25 @@ abstract class scheduled_task extends task_base {
break;
case self::FIELD_DAYOFWEEK:
$min = self::DAYOFWEEKMIN;
$max = self::DAYOFWEEKMAX;
$max = self::DAYOFWEEKMAXINPUT;
break;
default:
throw new \coding_exception("Field '$field' is not a valid crontab identifier.");
}
return $this->eval_cron_field($this->{$field}, $min, $max);
$result = $this->eval_cron_field($this->{$field}, $min, $max);
if ($field === self::FIELD_DAYOFWEEK) {
// For day of week, 0 and 7 both mean Sunday; if there is a 7 we set 0. The result array is sorted.
if (end($result) === 7) {
// Remove last element.
array_pop($result);
// Insert 0 as first element if it's not there already.
if (reset($result) !== 0) {
array_unshift($result, 0);
}
}
}
return $result;
}
/**
@ -459,9 +478,14 @@ abstract class scheduled_task extends task_base {
/**
* Calculate when this task should next be run based on the schedule.
*
* @param int $now Current time, for testing (leave 0 to use default time)
* @return int $nextruntime.
*/
public function get_next_scheduled_time() {
public function get_next_scheduled_time(int $now = 0): int {
if (!$now) {
$now = time();
}
// We need to change to the server timezone before using php date() functions.
\core_date::set_default_server_timezone();
@ -471,27 +495,64 @@ abstract class scheduled_task extends task_base {
$validdaysofweek = $this->get_valid(self::FIELD_DAYOFWEEK);
$validmonths = $this->get_valid(self::FIELD_MONTH);
$nextvalidyear = date('Y');
$currentminute = date("i") + 1;
$currenthour = date("H");
$currentday = date("j");
$currentmonth = date("n");
$currentdayofweek = date("w");
$nextvalidminute = $this->next_in_list($currentminute, $validminutes);
if ($nextvalidminute < $currentminute) {
$currenthour += 1;
// If any of the fields contain no valid data then the task will never run.
if (!$validminutes || !$validhours || !$validdays || !$validdaysofweek || !$validmonths) {
return self::NEVER_RUN_TIME;
}
$nextvalidhour = $this->next_in_list($currenthour, $validhours);
if ($nextvalidhour < $currenthour) {
$currentdayofweek += 1;
$currentday += 1;
$result = self::get_next_scheduled_time_inner($now, $validminutes, $validhours, $validdays, $validdaysofweek, $validmonths);
return $result;
}
/**
* Recursively calculate the next valid time for this task.
*
* @param int $now Start time
* @param array $validminutes Valid minutes
* @param array $validhours Valid hours
* @param array $validdays Valid days
* @param array $validdaysofweek Valid days of week
* @param array $validmonths Valid months
* @param int $originalyear Zero for first call, original year for recursive calls
* @return int Next run time
*/
protected function get_next_scheduled_time_inner(int $now, array $validminutes, array $validhours,
array $validdays, array $validdaysofweek, array $validmonths, int $originalyear = 0) {
$currentyear = (int)date('Y', $now);
if ($originalyear) {
// In recursive calls, check we didn't go more than 8 years ahead, that indicates the
// user has chosen an impossible date. 8 years is the maximum time, considering a task
// set to run on 29 February over a century boundary when a leap year is skipped.
if ($currentyear - $originalyear > 8) {
// Use this time if it's never going to happen.
return self::NEVER_RUN_TIME;
}
$firstyear = $originalyear;
} else {
$firstyear = $currentyear;
}
$nextvaliddayofmonth = $this->next_in_list($currentday, $validdays);
$nextvaliddayofweek = $this->next_in_list($currentdayofweek, $validdaysofweek);
$currentmonth = (int)date('n', $now);
// Evaluate month first.
$nextvalidmonth = $this->next_in_list($currentmonth, $validmonths);
if ($nextvalidmonth < $currentmonth) {
$currentyear += 1;
}
// If we moved to another month, set the current time to start of month, and restart calculations.
if ($nextvalidmonth !== $currentmonth) {
$newtime = strtotime($currentyear . '-' . $nextvalidmonth . '-01 00:00');
return $this->get_next_scheduled_time_inner($newtime, $validminutes, $validhours, $validdays,
$validdaysofweek, $validmonths, $firstyear);
}
// Special handling for dayofmonth vs dayofweek (see man 5 cron). If both are specified, then
// it is ok to continue when either matches. If only one is specified then it must match.
$currentday = (int)date("j", $now);
$currentdayofweek = (int)date("w", $now);
$nextvaliddayofmonth = self::next_in_list($currentday, $validdays);
$nextvaliddayofweek = self::next_in_list($currentdayofweek, $validdaysofweek);
$daysincrementbymonth = $nextvaliddayofmonth - $currentday;
$daysinmonth = date('t');
$daysinmonth = (int)date('t', $now);
if ($nextvaliddayofmonth < $currentday) {
$daysincrementbymonth += $daysinmonth;
}
@ -501,41 +562,65 @@ abstract class scheduled_task extends task_base {
$daysincrementbyweek += 7;
}
// Special handling for dayofmonth vs dayofweek:
// if either field is * - use the other field
// otherwise - choose the soonest (see man 5 cron).
if ($this->dayofweek == '*') {
$daysincrement = $daysincrementbymonth;
} else if ($this->day == '*') {
$daysincrement = $daysincrementbyweek;
} else {
// Take the smaller increment of days by month or week.
$daysincrement = $daysincrementbymonth;
if ($daysincrementbyweek < $daysincrementbymonth) {
$daysincrement = $daysincrementbyweek;
$daysincrement = min($daysincrementbymonth, $daysincrementbyweek);
}
// If we moved day, recurse using new start time.
if ($daysincrement != 0) {
$newtime = strtotime($currentyear . '-' . $currentmonth . '-' . $currentday .
' 00:00 +' . $daysincrement . ' days');
return $this->get_next_scheduled_time_inner($newtime, $validminutes, $validhours, $validdays,
$validdaysofweek, $validmonths, $firstyear);
}
$currenthour = (int)date('H', $now);
$nextvalidhour = $this->next_in_list($currenthour, $validhours);
if ($nextvalidhour != $currenthour) {
if ($nextvalidhour < $currenthour) {
$offset = ' +1 day';
} else {
$offset = '';
}
$newtime = strtotime($currentyear . '-' . $currentmonth . '-' . $currentday . ' ' . $nextvalidhour .
':00' . $offset);
return $this->get_next_scheduled_time_inner($newtime, $validminutes, $validhours, $validdays,
$validdaysofweek, $validmonths, $firstyear);
}
$nextvaliddayofmonth = $currentday + $daysincrement;
if ($nextvaliddayofmonth > $daysinmonth) {
$currentmonth += 1;
$nextvaliddayofmonth -= $daysinmonth;
// Round time down to an exact minute because we need to use numeric calculations on it now.
// If we construct times based on all the components, it will mess up around DST changes
// (because there are two times with the same representation).
$now = intdiv($now, 60) * 60;
$currentminute = (int)date('i', $now);
$nextvalidminute = $this->next_in_list($currentminute, $validminutes);
if ($nextvalidminute == $currentminute && !$originalyear) {
// This is not a recursive call so time has not moved on at all yet. We can't use the
// same minute as now because it has already happened, it has to be at least one minute
// later, so update time and retry.
$newtime = $now + 60;
return $this->get_next_scheduled_time_inner($newtime, $validminutes, $validhours, $validdays,
$validdaysofweek, $validmonths, $firstyear);
}
$nextvalidmonth = $this->next_in_list($currentmonth, $validmonths);
if ($nextvalidmonth < $currentmonth) {
$nextvalidyear += 1;
if ($nextvalidminute < $currentminute) {
// The time is in the next hour so we need to recurse. Don't use strtotime at this
// point because it will mess up around DST changes.
$minutesforward = $nextvalidminute + 60 - $currentminute;
$newtime = $now + $minutesforward * 60;
return $this->get_next_scheduled_time_inner($newtime, $validminutes, $validhours, $validdays,
$validdaysofweek, $validmonths, $firstyear);
}
// Work out the next valid time.
$nexttime = mktime($nextvalidhour,
$nextvalidminute,
0,
$nextvalidmonth,
$nextvaliddayofmonth,
$nextvalidyear);
return $nexttime;
// The next valid minute is in the same hour so it must be valid according to all other
// checks and we can finally return it.
return $now + ($nextvalidminute - $currentminute) * 60;
}
/**

147
lib/tests/task/scheduled_task_test.php
View File

@ -165,6 +165,153 @@ class scheduled_task_test extends \advanced_testcase {
$this->assertEquals(1, date('j', $nexttime));
}
/**
* Data provider for get_next_scheduled_time_detail.
*
* Note all times in here are in default Australia/Perth time zone.
*
* @return array[] Function parameters for each run
*/
public static function get_next_scheduled_time_detail_provider(): array {
return [
// Every minute = next minute.
['2023-11-01 15:15', '*', '*', '*', '*', '*', '2023-11-01 15:16'],
// Specified minute (coming up) = same hour, that minute.
['2023-11-01 15:15', '18', '*', '*', '*', '*', '2023-11-01 15:18'],
// Specified minute (passed) = next hour, that minute.
['2023-11-01 15:15', '11', '*', '*', '*', '*', '2023-11-01 16:11'],
// Range of minutes = same hour, next matching value.
['2023-11-01 15:15', '*/15', '*', '*', '*', '*', '2023-11-01 15:30'],
// Specified hour, any minute = first minute that hour.
['2023-11-01 15:15', '*', '20', '*', '*', '*', '2023-11-01 20:00'],
// Specified hour, specified minute = that time.
['2023-11-01 15:15', '13', '20', '*', '*', '*', '2023-11-01 20:13'],
// Any minute, range of hours = next hour in range, 00:00.
['2023-11-01 15:15', '*', '*/6', '*', '*', '*', '2023-11-01 18:00'],
// Specified minute, range of hours = next hour where minute not passed, that minute.
['2023-11-01 18:15', '10', '*/6', '*', '*', '*', '2023-11-02 00:10'],
// Specified day, any hour/minute.
['2023-11-01 15:15', '*', '*', '3', '*', '*', '2023-11-03 00:00'],
// Specified day (next month), any hour/minute.
['2023-11-05 15:15', '*', '*', '3', '*', '*', '2023-12-03 00:00'],
// Specified day, specified hour.
['2023-11-01 15:15', '*', '17', '3', '*', '*', '2023-11-03 17:00'],
// Specified day, specified minute.
['2023-11-01 15:15', '17', '*', '3', '*', '*', '2023-11-03 00:17'],
// 30th of every month, February.
['2023-01-31 15:15', '15', '10', '30', '*', '*', '2023-03-30 10:15'],
// Friday, any time. 2023-11-01 is a Wednesday, so it will run in 2 days.
['2023-11-01 15:15', '*', '*', '*', '5', '*', '2023-11-03 00:00'],
// Friday, any time (but it's already Friday).
['2023-11-03 15:15', '*', '*', '*', '5', '*', '2023-11-03 15:16'],
// Sunday (week rollover).
['2023-11-01 15:15', '*', '*', '*', '0', '*', '2023-11-05 00:00'],
// Specified days and day of week (days come first).
['2023-11-01 15:15', '*', '*', '2,4,6', '5', '*', '2023-11-02 00:00'],
// Specified days and day of week (day of week comes first).
['2023-11-01 15:15', '*', '*', '4,6,8', '5', '*', '2023-11-03 00:00'],
// Specified months.
['2023-11-01 15:15', '*', '*', '*', '*', '6,8,10,12', '2023-12-01 00:00'],
// Specified months (crossing year).
['2023-11-01 15:15', '*', '*', '*', '*', '6,8,10', '2024-06-01 00:00'],
// Specified months and day of week (i.e. first Sunday in December).
['2023-11-01 15:15', '*', '*', '*', '0', '6,8,10,12', '2023-12-03 00:00'],
// It's already December, but the next Friday is not until next month.
['2023-12-30 15:15', '*', '*', '*', '5', '6,8,10,12', '2024-06-07 00:00'],
// Around end of year.
['2023-12-31 23:00', '10', '3', '*', '*', '*', '2024-01-01 03:10'],
// Some impossible requirements...
['2023-12-31 23:00', '*', '*', '30', '*', '2', scheduled_task::NEVER_RUN_TIME],
['2023-12-31 23:00', '*', '*', '31', '*', '9,4,6,11', scheduled_task::NEVER_RUN_TIME],
// Normal years and leap years.
['2021-01-01 23:00', '*', '*', '28', '*', '2', '2021-02-28 00:00'],
['2021-01-01 23:00', '*', '*', '29', '*', '2', '2024-02-29 00:00'],
// Missing leap year over century. Longest possible gap between runs.
['2096-03-01 00:00', '59', '23', '29', '*', '2', '2104-02-29 23:59'],
];
}
/**
* Tests get_next_scheduled_time using a large number of example scenarios.
*
* @param string $now Current time (strtotime format)
* @param string $minute Minute restriction list for task
* @param string $hour Hour restriction list for task
* @param string $day Day restriction list for task
* @param string $dayofweek Day of week restriction list for task
* @param string $month Month restriction list for task
* @param string|int $expected Expected run time (strtotime format or time int)
* @dataProvider get_next_scheduled_time_detail_provider
* @covers ::get_next_scheduled_time
*/
public function test_get_next_scheduled_time_detail(string $now, string $minute, string $hour,
string $day, string $dayofweek, string $month, $expected): void {
// Create test task with specified times.
$task = new scheduled_test_task();
$task->set_minute($minute);
$task->set_hour($hour);
$task->set_day($day);
$task->set_day_of_week($dayofweek);
$task->set_month($month);
// Check function results.
$nowtime = strtotime($now);
if (is_int($expected)) {
$expectedtime = $expected;
} else {
$expectedtime = strtotime($expected);
}
$actualtime = $task->get_next_scheduled_time($nowtime);
$this->assertEquals($expectedtime, $actualtime, 'Expected ' . $expected . ', actual ' . date('Y-m-d H:i', $actualtime));
}
/**
* Tests get_next_scheduled_time around DST changes, with regard to the continuity of frequent
* tasks.
*
* We want frequent tasks to keep progressing as normal and not randomly stop for an hour, or
* suddenly decide they need to happen in the past.
*
* @covers ::get_next_scheduled_time
*/
public function test_get_next_scheduled_time_dst_continuity(): void {
$this->resetAfterTest();
$this->setTimezone('Europe/London');
// Test task is set to run every 20 minutes (:00, :20, :40).
$task = new scheduled_test_task();
$task->set_minute('*/20');
// DST change forwards. Check times in GMT to ensure it progresses as normal.
$before = strtotime('2023-03-26 00:59 GMT');
$this->assertEquals(strtotime('2023-03-26 00:59 Europe/London'), $before);
$one = $task->get_next_scheduled_time($before);
$this->assertEquals(strtotime('2023-03-26 01:00 GMT'), $one);
$this->assertEquals(strtotime('2023-03-26 02:00 Europe/London'), $one);
$two = $task->get_next_scheduled_time($one);
$this->assertEquals(strtotime('2023-03-26 01:20 GMT'), $two);
$three = $task->get_next_scheduled_time($two);
$this->assertEquals(strtotime('2023-03-26 01:40 GMT'), $three);
$four = $task->get_next_scheduled_time($three);
$this->assertEquals(strtotime('2023-03-26 02:00 GMT'), $four);
// DST change backwards.
$before = strtotime('2023-10-29 00:59 GMT');
// The 'before' time is 01:59 Europe/London, but we won't explicitly test that because
// there are two 01:59s so it might fail depending on implementation.
$one = $task->get_next_scheduled_time($before);
$this->assertEquals(strtotime('2023-10-29 01:00 GMT'), $one);
// We cannot compare against the Eerope/London time (01:00) because there are two 01:00s.
$two = $task->get_next_scheduled_time($one);
$this->assertEquals(strtotime('2023-10-29 01:20 GMT'), $two);
$three = $task->get_next_scheduled_time($two);
$this->assertEquals(strtotime('2023-10-29 01:40 GMT'), $three);
$four = $task->get_next_scheduled_time($three);
$this->assertEquals(strtotime('2023-10-29 02:00 GMT'), $four);
// This time is now unambiguous in Europe/London.
$this->assertEquals(strtotime('2023-10-29 02:00 Europe/London'), $four);
}
public function test_timezones() {
global $CFG, $USER;