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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented May 9, 2024

Issue #648

1st commit is the test code added by @schreven in PR #647

2nd commit is my first code that makes those test cases pass. It only touches the case of events that recur at an interval of days.

  • added more test cases for weekly, monthly and yearly recurring events, that are engineered so that some event recurrence fall in the 0200 to 0300 summer time start on the exact day that summer time does start.
  • adjusted code to make those pass (similar to what I did for the daily case)
  • refactored to avoid the repeated code pattern
  • added tests and adjusted code for nextHourly

Thanks to @gharlan and @schreven for input, code suggestions etc.

Copy link

codecov bot commented May 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.78%. Comparing base (72a1fe9) to head (2b32d40).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #653   +/-   ##
=========================================
  Coverage     98.78%   98.78%           
- Complexity     1869     1872    +3     
=========================================
  Files            71       71           
  Lines          5254     5265   +11     
=========================================
+ Hits           5190     5201   +11     
  Misses           64       64           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lib/Recur/RRuleIterator.php Outdated Show resolved Hide resolved
@schreven
Copy link

It looks better than my attempt. In the end wasn't it possible to store the "unaltered" datetime as mentioned here?

One idea would be to have a new object that extends DateTime and overrides DateTime::format() to not alter the datetime on leaps. It would then be used in the main loop, and transformed to a traditional datetime when returned.

@phil-davis
Copy link
Contributor Author

In the end wasn't it possible to store the "unaltered" datetime as mentioned

The "unaltered" time, for example "0230" is not a valid time on the date when summer time starts. So the DateTime object cannot store that date-time combination. I also thought about remembering the original required time-of-day in a separate variable. But in the end I could just detect if the time-jump happened, and remember that.

There are other possible edge-cases, but they don't really happen. For example, if 2 recurrences in a row fall on the summer time start date, then reversing the hour-jump would have to be delayed. But in real-life, summer time usually starts on a Sunday morning. For a yearly event, the recurrence in the next year is always 1 or 2 days later in the week, so there can't be 2 Sunday's in a row for a yearly event. But someone could design an event that happens at 0230 every 52nd Sunday - and that could fall on the summer time start date for a few years in a row!

@phil-davis phil-davis requested review from schreven, gharlan and staabm and removed request for gharlan and schreven May 17, 2024 10:09
@phil-davis phil-davis marked this pull request as ready for review May 17, 2024 10:10
} else {
$increase = 0;
do {
++$increase;
$tempDate = clone $this->currentDate;
$tempDate = $tempDate->modify('+ '.($this->interval * $increase).' months');
$tempDate = $tempDate->modify('+ '.($this->interval * $increase).' months '.$this->startDate->format('H:i:s'));

Choose a reason for hiding this comment

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

Nitpick: We can re-use the function right?

Suggested change
$tempDate = $tempDate->modify('+ '.($this->interval * $increase).' months '.$this->startDate->format('H:i:s'));
$tempDate = $tempDate->advanceTheDate('+ '.($this->interval * $increase).' months ');

@@ -349,7 +392,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'));

Comment on lines +308 to +327
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 0130 0330 0430 0530...
if ($this->interval > 1) {
$expectedHourOfNextDate = ($hourOfCurrentDate + $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;
}

Choose a reason for hiding this comment

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

Nitpick:
With the comments it's adding a lot of lines to this function. Perhaps we can move the new code to a private function "AdjustForTimeJumps"

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

Successfully merging this pull request may close these issues.

None yet

3 participants