Refactor the processRecurrences method.
#236
Conversation
…RRule identifies (Constitutes part of a fix to issue u01jmg3#15)
This is created with full knowledge of the initial event's timezone.
(Also fixes u01jmg3#15)
Leaving behind the code that determines date sets
RFC5545, page 42, paragraph 1 states [^1]: > The BYDAY rule part MUST NOT be specified with a numeric value when the FREQ rule part is not set to MONTHLY or YEARLY. We now validate that, and emit an error (to the php log) in this case. ---- [^1] - https://tools.ietf.org/html/rfc5545#page-42)
(As a protected function, it's not publically accessible.)
(There is another - `$defaultWeekStart` - but I plan to use that again.)
The ics parser has a minimal php requirement of `5.3.9`. Unfortunately, I have used syntax and features that are not supported in such an early version. This patch removes them. * `ImmutableDateTime` is 5.5.0+ * `ImmutableDateTime::createFromMutable` is 5.6.0+ * Class member access on cloning (`(clone $an_object)->a_member`) is 7.0+ * `DateTime::createFromImmutable` is 7.3.0+ * Permitting trailing commas in function/method calls is 7.3+
🙏 💯 Will need some time to digest all your hard work but so far it is looking very good. |
There was a problem hiding this comment.
- Happy to approve, great job
My only query:
Whether intentionally or not, the former version of the
processRecurrences()method wasn't complying with this stipulation by not generating recurrences for this event (functionally rejecting the rrule as invalid). The refactored version posts an error (to the PHP log), then strips the prefixed number and continues with that instead. However, this can be changed back to the old behaviour (of rejecting the invalid rrule and not recurring) if you prefer.
- Would this constitute a breaking change if people are reliant on the old behaviour?
|
Thanks!
I suppose it would make it harder to notice the error, which some users could rely on to help debug their calendars... Would you like me to amend the PR so the code rejects the invalid rule instead of trying to correct it? |
Please but keep your changes because I think we should apply them in |
What it says on the tin - rewriting the aforementioned method.
(At the point of writing) the commits of this PR can be split into four logical groupings:
Prep Work (3 commits) - adding support code in preparation for...
The Rewrite (9 commits) - the actual refactoring of code and logic.
Post-Rewrite cleanup (8 commits) - removing code no longer used post-rewrite, and adapting the rewritten code so it runs on PHP versions prior to the
7.3.xversion I was using when I was working on this.Synchronisation (1 commit) - the commits of Some cleanup of the
processRecurrencesmethod #232 were originally going to be part of this PR. However, I changed my mind and supplied them in a separate PR. Unfortunately, this caused a merge conflict. This single commit rectifies that.For the most part, the resultant output should be similar to what it was before. However - as hinted by the fact that some tests have been uncommented - there are some differences.
One difference is that the file in the "examples" folder now generates more events than before. Or, more specifically, the "Paris Timezone Test" (lines 148-162) now recurs. This particular event has a RRULE of
FREQ=WEEKLY;BYDAY=2WE. According to the spec (Page 42, Paragraph 1, towards the end of the 10th line):Whether intentionally or not, the former version of the
processRecurrences()method wasn't complying with this stipulation by not generating recurrences for this event (functionally rejecting the rrule as invalid). The refactored version posts an error (to the PHP log), then strips the prefixed number and continues with that instead. However, this can be changed back to the old behaviour (of rejecting the invalid rrule and not recurring) if you prefer.Another thing of note is that this PR resolves issue #15. That wasn't - I might point out - intentional: it's simply that the new class method used to determine which days in a month a BYDAY refers to is used by both the YEARLY frequency (which did support multiple BYDAYs (so long as a BYMONTH stanza also existed)) and the MONTHLY frequency (which didn't (but now does)).
Other perks include: no more use of
strtotime(in this method anyhow); reduced code duplication; and that implementation of the other RRULE stanzas should hopefully now be easier.