Skip to content

feat/modern date api#10

Merged
ralflang merged 13 commits into
FRAMEWORK_6_0from
feat/modern-date-api
May 11, 2026
Merged

feat/modern date api#10
ralflang merged 13 commits into
FRAMEWORK_6_0from
feat/modern-date-api

Conversation

@ralflang
Copy link
Copy Markdown
Member

@ralflang ralflang commented Apr 19, 2026

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.

  • fix: Casting and coupling problems
  • feat: Flesh out Horde_Date src/ branch
  • test: Cover src/ variant with tests
  • feat: Recurrence needs to mind timezones
  • test: Amend recurrence tests
  • chore: Add Torben. Jan isn't currently active.
  • feat: interface-compatible drop-in replacement for Horde_Date
  • docs: update UPGRADING guide to .md format and strategies for adopting modern 3.0
  • test: Cover HordeLegacyDate compatibility with Horde_Date
  • chore: Add keywords, use PHP 8.1+

@ralflang ralflang requested a review from TDannhauer April 19, 2026 07:38
Copy link
Copy Markdown
Contributor

@TDannhauer TDannhauer left a comment

Choose a reason for hiding this comment

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

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?

@ralflang ralflang merged commit 8d02894 into FRAMEWORK_6_0 May 11, 2026
0 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants