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

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 (5d7ca00) to head (1d0d0bd).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #653   +/-   ##
=========================================
  Coverage     98.78%   98.78%           
- Complexity     1870     1875    +5     
=========================================
  Files            71       71           
  Lines          5256     5273   +17     
=========================================
+ Hits           5192     5209   +17     
  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 force-pushed the dst-leap-648 branch 3 times, most recently from a2e53c3 to 663937e Compare May 17, 2024 08:47
@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
lib/Recur/RRuleIterator.php Outdated Show resolved Hide resolved
@@ -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'));

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

lib/Recur/RRuleIterator.php Outdated Show resolved Hide resolved
@phil-davis phil-davis requested a review from schreven May 30, 2024 08:18
@phil-davis
Copy link
Contributor Author

I have not tried to change the behavior when a weekly, monthly, yearly etc. rule has BYHOUR that ends up specifying that the event occurs in the 0200 hour. In that case, the client has specifically asked for a time like 0200, and on the summer-time transition day that hour does not exist. The current code logic does not move the event to 0300, it does not schedule an event at all for that potential recurrence.

Firstly, the desired behavior needs to be defined. Then if different from the current code logic, a PR can be done.

I think that this PR is big enough already, and covers the more likely cases when this summer-time thing happens. So please review.

@schreven @gharlan @staabm

@phil-davis
Copy link
Contributor Author

Also note: this library does not currently support BYHOUR with MONTHLY or YEARLY RRULEs. So in nextMonthly and nextYearly it is fine to always re-set the time from the original start time. The recurrences for MONTHLY and YEARLY rules will all be only once on a day, at the same time (the time only varies if the time moves ahead one hour on summer-time transition days).

And this library does not do anything with BYMINUTE or BYSECOND, so there is no need to consider what might happen for rules that are requested to happen multiple times each hour etc.

@@ -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;
}
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

@phil-davis phil-davis merged commit 1b388aa into sabre-io:master Jun 6, 2024
8 checks passed
@phil-davis phil-davis deleted the dst-leap-648 branch June 6, 2024 10:53
@phil-davis
Copy link
Contributor Author

Backport to 4.5 release branch is PR #660

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.

4 participants