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

Proposed fix for #238 #239

Merged
merged 1 commit into from
Sep 2, 2019
Merged

Proposed fix for #238 #239

merged 1 commit into from
Sep 2, 2019

Conversation

s0600204
Copy link
Collaborator

Resolve second reported problem within issue #238.

This problem was caused by an imperfect approach to translating between different week starts.

Current Approach

php's DateTime::setISODate() follows ISO-8601 and expects weeks to start on a Monday, with days being identified numerically like so:

    Mon  Tue  Wed  Thu  Fri  Sat  Sun
     1    2    3    4    5    6    7

rfc5545 defaults to Monday week starts, but allows alternate week starts to be used within RRULEs.

The way I had initially implemented the translation between two different week starts was to offset the days on or after the alternate week start by -7. Thus, given WKST=TH, the week day numerical identifiers would be as follows:

    Thu  Fri  Sat  Sun  Mon  Tue  Wed
    -3   -2   -1    0    1    2    3

If we test this against an iCal snippet:

DTSTART:20190806
RRULE:FREQ=WEEKLY;INTERVAL=2;BYDAY=TU,WE,FR,SA;WKST=TH;UNTIL=20190915

We would expect August 6th, 7th, 16th, 17th, 20th, 21st, 30th, and 31st, and September 3rd, 4th, 13th, and 14th.

In the first loop, the RRULE parsing code would first generate August 2nd, 3rd, 6th and 7th. The first two are before the start date, and the third date is the start date, so these three are discarded leaving August 7th.

Bump up the base date up by two weeks to 20th August (as we have an INTERVAL of two) and the second iteration generates August 16th, 17th, 20th, and 21st .

Bump the base date up another two weeks to 3rd September, and the third iteration generates 30th and 31st of August, and the 3rd and 4th of September.

Bump up another two weeks to 17th September, and this is after the end date stated in the UNTIL, so we stop. But we're still missing the final two dates.

Alternate Approach

An alternate way might be to offset all days before the week start by +7, so with the same WKST as before, the day numerical identifiers passed to setISODate look like the following:

    Thu  Fri  Sat  Sun  Mon  Tue  Wed
     4    5    6    7    8    9    10

However this approach is also flawed. Retaining the same ICal snippet as before, the first iteration - starting at 6th August - would generate the following dates: August 9th, 10th, 13th and 14th... none of which are dates we want.

Changing the offset to +(7 x INTERVAL) generates the following dates on the first iteration: August 16th, 17th, 20th, 21st. Better as these are dates we'd want, but note that August 7th has not been generated (and never will be).

The New Approach

The approach actually implemented in this PR is slightly more complex. Instead of one set of offsets, we have two. First, offset the day numerical ids that fall before the day of the initial date (not the WKST) +7. E.g. when using the same ICal snippet as before:

    Tue  Wed  Thu  Fri  Sat  Sun  Mon
     2    3    4    5    6    7    8

Then, offset the days that fall on or after the WKST by +(7 * (INTERVAL - 1)):

    Tue  Wed  Thu  Fri  Sat  Sun  Mon
     2    3    11   12   13   14   15

Now, when we run the above snippet, the first iteration generates August 6th, 7th, 16th, and 17th.

The second iteration, starting from the base date of August 20th, generates 20th, 21st, 30th and 31st.

The third iteration, starting from the base date of September 3rd, generates September 3rd, 4th, 13th and 14th.

And the next iteration would start from the base date of September 17th, but this is after the UNTIL so we stop.

And we now have all the dates we'd expect from this RRULE.

Testing

This clears composer test; and if we test against the provided ICal snippet provided by @britweb-tim in issue #238:

DTSTART;TZID=Europe/London:20190818T103000
RRULE:FREQ=WEEKLY;WKST=SU;UNTIL=20190902;BYDAY=SU

The day numerical ids passed to setISODate are:

    Sun  Mon  Tue  Wed  Thu  Fri  Sat
     7    8    9    10   11   12   13

And the dates ultimately generated by the parser are: Aug 18th and 25th, and Sept 1st

Copy link
Owner

@u01jmg3 u01jmg3 left a comment

Choose a reason for hiding this comment

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

Works a treat

@u01jmg3 u01jmg3 merged commit f551c3b into u01jmg3:master Sep 2, 2019
@u01jmg3 u01jmg3 added this to the v2.x.x milestone Sep 2, 2019
@s0600204 s0600204 deleted the issue-238 branch September 3, 2019 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants