Handle summer time jumps in event recurrences

[phil-davis]

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.

[codecov]

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.

[schreven]

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]

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!

[schreven]

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'));

[phil-davis]

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!)

[phil-davis]

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.

[phil-davis]

@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.

[phil-davis]

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]

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.