feat/modern date api#10
Conversation
TDannhauer
left a comment
There was a problem hiding this comment.
Too big for human only review.
I suggest to separate indentation fixes and date updates from real changes , this would reduce PR size tremendously.
Some digging with AI:
Findings
High: Legacy public-property compatibility is not fully preserved in Horde_Date_Recurrence
The class removes real public properties ($start, $recurType, $exceptions, etc.) and replaces them with magic __get/__set.
That changes behavior for get_object_vars(), property references, serializers that inspect declared properties, and code relying on public members existing physically.
This is a BC risk because the PR description says it preserves the legacy public API.
Medium: INTERVAL=0 now persists as zero in modern RRULE parsing
Old code effectively rejected non-positive intervals (kept default 1), but new Recurrence::setInterval() accepts 0.
This can silently make a recurrence non-functional (nextRecurrence() returns null when interval is 0) for malformed-but-seen-in-the-wild data.
Medium: RRULE BYDAY parsing became exception-throwing for unknown tokens
New parser throws InvalidArgumentException on unknown day abbreviations.
Previous behavior was lenient (bad tokens were effectively ignored/notice-level), so this can hard-fail imports from loosely formatted producers.
Open Questions / Assumptions
I’m assuming strict BC for legacy consumers is a release requirement (as implied in the PR text).
I reviewed the patch-level code paths and tests in the diff, but did not execute the PR test matrix locally.
Change Summary
Big modernization direction looks solid, and test coverage is significantly expanded.
The main risks I’d ask the author to address before merge are BC edge-cases in Horde_Date_Recurrence surface compatibility and stricter parsing behavior for malformed recurrence rules.
@ralflang what do you think about the first two findings?
Keep the Horde_Date largely as it was
Add a new PSR-4 drop-in which behaves like Horde_Date and types like Horde_Date but really uses modern PHP Builtins (Upgrade Path B)
Add a modern, OO-expressive implementation to be used by new code.
As with other changes I want to provide a way forwards but don't do risky stuff right before RC release.