Recurrence in 5th week of month gets included in first week of next month if there is no 5th week

[room34]

PHP Version

8.1.2

PHP date.timezone

America/Los_Angeles

ICS Parser Version

3.2.0

Operating System

Linux

Description

A user of my WordPress plugin that uses ics-parser has a recurring event with this rule:

RRULE:FREQ=MONTHLY;BYDAY=5SA;UNTIL=20231231T000000Z

His understanding (and mine) is that if a month doesn't have a 5th Saturday, the event simply should not occur in that month. He reports that he has tested importing his ICS feed into Google Calendar, and that's how it's working. But with my plugin (using ics-parser), in addition to the 5th Saturdays, it is creating a number of extra occurrences on the 1st Wednesday of months.

Steps to Reproduce

Parse the sample ICS file in the attached ZIP.

rrule-5sa-test.ics.zip

[room34]

Here's some additional information I just received from the user:

I figured out the logic the parser uses. When the month doesn't have a 5th Saturday, it takes the number of days after the end of the month until a 5th Saturday would be, and then puts the event on that date at the beginning of the month. For example, if the month ends on a Friday, the event goes on the 1st. If it ends on a Tuesday, the event goes on the 4th. In the case of May, since the month ends on a Wednesday, it put the event on the 3rd.

[room34]

To clarify, in conjunction with the quote above: it looks like if there's no 5th Saturday, it's putting the event on the date in the same month that corresponds to the number of days past the end of the month when the 5th Saturday would occur.

For instance, in May 2023 there is no 5th Saturday, but the 5th Saturday would be 3 days after the month ends, and a false instance is being shown on May 3. In June 2023, the 5th Saturday would be 1 day after the month ends, and the false instance is on June 1.

[room34]

I think I've identified the issue. Looking at the ICal::getDaysOfMonthMatchingByDayRRule() method, there's an if / elseif / else statement. The else condition has this line:

while ($bydayDateTime->format('n') === $initialDateTime->format('n')) {

I find by adding an equivalent if statement before assigning the value of $matchingDays[] in the if and elseif conditions, I can get the events to only appear in months that actually have a 5th Saturday. Here's the full conditional statement:

            if ($ordwk < 0) { // -ve {ordwk}
                $bydayDateTime->modify((++$ordwk) . ' week');
                if ($bydayDateTime->format('n') === $initialDateTime->format('n')) {
                    $matchingDays[] = $bydayDateTime->format('j');
                }
            } elseif ($ordwk > 0) { // +ve {ordwk}
                $bydayDateTime->modify((--$ordwk) . ' week');
                if ($bydayDateTime->format('n') === $initialDateTime->format('n')) {
                    $matchingDays[] = $bydayDateTime->format('j');
                }
            } else { // No {ordwk}
                while ($bydayDateTime->format('n') === $initialDateTime->format('n')) {
                    $matchingDays[] = $bydayDateTime->format('j');
                    $bydayDateTime->modify('+1 week');
                }
            }

Explanation: The issue is that the calculated $bydayDateTime->format('j'); is actually pushing the event into the next month (since that's how PHP handles day numbers that exceed the length of the month), but since the code is only looking at the day-of-month value (j), it's keeping the event in the same month. The new if statements check if the calculated date is really in the same month or not, which is how it is effectively removing these erroneous instances.

[u01jmg3]

It would be great if you could put a PR together for us to take a look at.

[room34]

I apologize for my ignorance here but I've never created a pull request on GitHub and don't know how to go about it. I've spent a few minutes researching it, but I'm not really getting anywhere.

I've checked out a local copy of the project, and have my fix in place (in the trunk) in my local copy, but I'm not sure what to do from there. Any advice you can give would be appreciated.

(The change is very minimal: as I described above, it's adding an if with the exact same condition as the while, wrapping both definitions of $matchingDays[] in the ICal.php file that currently appear at lines 1774 and 1777.)

[s0600204]

The short answer is that you'll need to create a fork of the project under your account here on Github, add it as a remote to your local copy, push your changes to your fork, at which point GitHub will prompt you to create a pull request from the recently pushed branch.

However: looking into this, it appears there's an edge case that needs handling, so would you object if I created the PR instead, using your proposed changes as a base?

[room34]

@s0600204

However: looking into this, it appears there's an edge case that needs handling, so would you object if I created the PR instead, using your proposed changes as a base?

I would be very happy if you did that. I don't really have the "bandwidth" to become an active contributor on this project and that's just too much overhead for me to push this small modification, as much as I want to see it added. Thanks!