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

Handle summer time jumps in event recurrences #653

Merged
merged 19 commits into from
Jun 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
b9c39da
Reproduce bug where dst leap is passed on to subsequent occurences
cyrilvanschreven-proton Apr 22, 2024
b37ef3d
Fix test code format
phil-davis May 9, 2024
963189b
Handle summer time start for daily recurrences
phil-davis May 9, 2024
2feab3f
Handle summer time start for weekly recurrences
phil-davis May 9, 2024
3b37fbc
Handle summer time start for monthly recurrences
phil-davis May 9, 2024
f4a0bba
Handle summer time start for yearly recurrences
phil-davis May 9, 2024
778177c
Refactor summer time start logic into advanceTheDate function
phil-davis May 9, 2024
fb5689a
Handle summer time start for hourly recurrences
phil-davis May 9, 2024
018789e
Refactor advanceTheDate
phil-davis May 9, 2024
d0cb455
fix: refactor advanceTheDate
phil-davis May 17, 2024
eef9fa6
Handle case when BYMONTHDAY falls on summer time start
phil-davis May 17, 2024
9b20d5e
Handle case when day at or near end of month falls on summer time start
phil-davis May 17, 2024
102909e
refactor hourly time jump logic into adjustForTimeJumpsOfHourlyEvent …
phil-davis May 27, 2024
85d72e0
refactor original start time calculation into startTime method
phil-davis May 27, 2024
cc112fb
refactor adjustForTimeJumpsOfHourlyEvent to be protected
phil-davis May 27, 2024
9d68c7a
Handle summer time start for weekly BYDAY recurrences
phil-davis May 30, 2024
5a3dd88
Add test case for Weekly BYDAY with BYHOUR on summer-time
phil-davis May 30, 2024
9039f90
Add test cases and fix YEARLY with BYMONTH on summer-time transition
phil-davis May 30, 2024
1d0d0bd
Add test cases and fix YEARLY with BYMONTH BYDAY on summer-time trans…
phil-davis May 30, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 74 additions & 9 deletions lib/Recur/RRuleIterator.php
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,14 @@ public function fastForward(\DateTimeInterface $dt): void
*/
protected ?\DateTimeInterface $currentDate;

/**
* The number of hours that the next occurrence of an event
* jumped forward, usually because summer time started and
* the requested time-of-day like 0230 did not exist on that
* day. And so the event was scheduled 1 hour later at 0330.
*/
protected int $hourJump = 0;

/**
* Frequency is one of: secondly, minutely, hourly, daily, weekly, monthly,
* yearly.
Expand Down Expand Up @@ -276,12 +284,65 @@ public function fastForward(\DateTimeInterface $dt): void

/* Functions that advance the iterator {{{ */

/**
* Gets the original start time of the RRULE.
*
* The value is formatted as a string with 24-hour:minute:second
*/
protected function startTime(): string
{
return $this->startDate->format('H:i:s');
}

/**
* Advances currentDate by the interval.
* The time is set from the original startDate.
* If the recurrence is on a day when summer time started, then the
* time on that day may have jumped forward, for example, from 0230 to 0330.
* Using the original time means that the next recurrence will be calculated
* based on the original start time and the day/week/month/year interval.
* So the start time of the next occurrence can correctly revert to 0230.
*/
protected function advanceTheDate(string $interval): void
{
$this->currentDate = $this->currentDate->modify($interval.' '.$this->startTime());
}

/**
* Does the processing for adjusting the time of multi-hourly events when summer time starts.
*/
protected function adjustForTimeJumpsOfHourlyEvent(\DateTimeInterface $previousEventDateTime): void
{
if (0 === $this->hourJump) {
// Remember if the clock time jumped forward on the next occurrence.
// That happens if the next event time is on a day when summer time starts
// and the event time is in the non-existent hour of the day.
// For example, an event that normally starts at 02:30 will
// have to start at 03:30 on that day.
// If the interval is just 1 hour, then there is no "jumping back" to do.
// The events that day will happen, for example, at 0030 0130 0330 0430 0530...
if ($this->interval > 1) {
$expectedHourOfNextDate = ((int) $previousEventDateTime->format('G') + $this->interval) % 24;
$actualHourOfNextDate = (int) $this->currentDate->format('G');
$this->hourJump = $actualHourOfNextDate - $expectedHourOfNextDate;
}
} else {
// The hour "jumped" for the previous occurrence, to avoid the non-existent time.
// currentDate got set ahead by (usually) 1 hour on that day.
// Adjust it back for this next occurrence.
$this->currentDate = $this->currentDate->sub(new \DateInterval('PT'.$this->hourJump.'H'));
$this->hourJump = 0;
}
}

/**
* Does the processing for advancing the iterator for hourly frequency.
*/
protected function nextHourly(): void
{
$previousEventDateTime = clone $this->currentDate;
$this->currentDate = $this->currentDate->modify('+'.$this->interval.' hours');
$this->adjustForTimeJumpsOfHourlyEvent($previousEventDateTime);
}

/**
Expand All @@ -290,7 +351,7 @@ protected function nextHourly(): void
protected function nextDaily(): void
{
if (!$this->byHour && !$this->byDay) {
$this->currentDate = $this->currentDate->modify('+'.$this->interval.' days');
$this->advanceTheDate('+'.$this->interval.' days');

return;
}
Expand Down Expand Up @@ -349,7 +410,7 @@ protected function nextDaily(): void
protected function nextWeekly(): void
{
if (!$this->byHour && !$this->byDay) {
$this->currentDate = $this->currentDate->modify('+'.$this->interval.' weeks');
$this->advanceTheDate('+'.$this->interval.' weeks');

return;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Should we not handle the execution going to the rest of the function as well? I would well see a line at the very end to set the time straight:

$this->startDate->modify($this->startDate->format('H:i:s'));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit trickier. I will try have a look tonight. I just need to make sure that various BYDAY BYHOUR etc filters will skip along correctly while sometimes happening to hit a summer-time start day. There might be a few code paths to think about (or it might turn out to be easy!)

Copy link
Contributor Author

@phil-davis phil-davis May 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have done the scenario for WEEKLY BYDAY when the recurrence falls on Sunday at 0230.

It is trickier for: FREQ=WEEKLY;INTERVAL=2;BYDAY=SA,SU;WKST=MO;BYHOUR=2,14

The RRULE says to schedule at 0200 and 1400 on each Saturday and Sunday.
On the Sunday of summer-time transition there is no 0200. The code current skips scheduling anything on the Sunday morning of summer-time transition. Future recurrences on later weekends are correctly scheduled at 0200 and 1400.

The "bug" in this case is not that the scheduled time gets locked forward to 0300, but that the recurrence on the summer-time transition day is not scheduled at all.

The RFC does not say anything about what is required if one of the hours mentioned in BYHOUR does not exist on the day in question.

I pushed a test case for now, that demonstrates the current behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schreven I have handled all the cases that I could see where nextWeekly netMonthly or nextYearly have BYDAY or similar specified and the resulting day in the week, month or year might fall on a summer-time start date.
There are test cases and code fixes for all that.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks for the details

The "bug" in this case is not that the scheduled time gets locked forward to 0300, but that the recurrence on the summer-time transition day is not scheduled at all

Indeed that appears like a separate question that is out of scope

Looks good to me overall, with all the tests it's convincing that it behaves correctly

Expand All @@ -371,7 +432,7 @@ protected function nextWeekly(): void
if ($this->byHour) {
$this->currentDate = $this->currentDate->modify('+1 hours');
} else {
$this->currentDate = $this->currentDate->modify('+1 days');
$this->advanceTheDate('+1 days');
}

// Current day of the week
Expand Down Expand Up @@ -408,13 +469,13 @@ protected function nextMonthly(): void
// occur to the next month. We Must skip these invalid
// entries.
if ($currentDayOfMonth < 29) {
$this->currentDate = $this->currentDate->modify('+'.$this->interval.' months');
$this->advanceTheDate('+'.$this->interval.' months');
} else {
$increase = 0;
do {
++$increase;
$tempDate = clone $this->currentDate;
$tempDate = $tempDate->modify('+ '.($this->interval * $increase).' months');
$tempDate = $tempDate->modify('+ '.($this->interval * $increase).' months '.$this->startTime());
} while ($tempDate->format('j') != $currentDayOfMonth);
$this->currentDate = $tempDate;
}
Expand Down Expand Up @@ -465,11 +526,15 @@ protected function nextMonthly(): void
}
}

// Set the currentDate to the year and month that we are in, and the day of the month that we have selected.
// That day could be a day when summer time starts, and if the time of the event is, for example, 0230,
// then 0230 will not be a valid time on that day. So always apply the start time from the original startDate.
// The "modify" method will set the time forward to 0330, for example, if needed.
$this->currentDate = $this->currentDate->setDate(
(int) $this->currentDate->format('Y'),
(int) $this->currentDate->format('n'),
(int) $occurrence
);
)->modify($this->startTime());
}

/**
Expand Down Expand Up @@ -586,7 +651,7 @@ protected function nextYearly(): void
}

// The easiest form
$this->currentDate = $this->currentDate->modify('+'.$this->interval.' years');
$this->advanceTheDate('+'.$this->interval.' years');

return;
}
Expand Down Expand Up @@ -650,7 +715,7 @@ protected function nextYearly(): void
(int) $currentYear,
(int) $currentMonth,
(int) $occurrence
);
)->modify($this->startTime());

return;
} else {
Expand All @@ -667,7 +732,7 @@ protected function nextYearly(): void
(int) $currentYear,
(int) $currentMonth,
(int) $currentDayOfMonth
);
)->modify($this->startTime());

return;
}
Expand Down
Loading