feat(calendar): migrate iCalendar export to sabre/vobject

[JohnVillalovos]

Replace the Smarty-templated ICS renderer with programmatic
VCALENDAR/VEVENT/VALARM construction via sabre/vobject and remove the
now-unused tpl/Export/ical.tpl. Render() also accepts an optional
$calendarName, emitted as NAME/X-WR-CALNAME, for use by subscription
feeds.

ExtraIcalLines (plugin-supplied raw ICS text) is parsed with Sabre's own
Reader, wrapped in a throwaway VCALENDAR/VEVENT shell, instead of being
concatenated into the template output verbatim. That preserves property
parameters (e.g. ATTENDEE;CN=...), nested components (e.g.
BEGIN:VALARM), and RFC 5545 line folding, and it stops a plugin from
injecting arbitrary calendar structure. A caught ParseException skips
only the malformed fragment so one bad plugin-supplied fragment can't
break the whole feed.

iCalendarReservationView no longer pre-escapes SUMMARY/DESCRIPTION for
RFC 5545 (removes toRfc5545Text): Sabre's serializer already escapes
TEXT values, so the old pre-escaping caused reserved characters and
newlines to be double-escaped in the output.

Assisted-by: Claude:claude-sonnet-5

[Copilot]

Pull request overview

This PR migrates LibreBooking’s iCalendar (ICS) export from a Smarty template to programmatic generation using sabre/vobject, improving correctness for escaping, parameters, and nested components while removing the legacy template renderer.

Changes:

• Replace Smarty-templated ICS generation with Sabre\VObject-based VCALENDAR/VEVENT/VALARM construction.
• Stop pre-escaping TEXT fields in iCalendarReservationView and rely on Sabre’s serializer for RFC 5545 escaping.
• Add/adjust tests for reserved-character escaping and robust parsing of plugin-supplied ExtraIcalLines, and update PHPStan config to accommodate Sabre’s magic properties.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tpl/Export/ical.tpl	Removes the legacy Smarty ICS template renderer.
Pages/Export/CalendarExportDisplay.php	Implements ICS generation via sabre/vobject, including alarms and plugin-supplied fragments.
lib/Application/Schedule/iCalendarReservationView.php	Removes RFC 5545 pre-escaping so Sabre handles TEXT serialization.
tests/Presenters/CalendarExportPresenterTest.php	Updates/extends tests to validate raw view values and serialized output escaping + fragment parsing behavior.
phpstan.neon	Adds Sabre\VObject\Node as a universal object crate to suppress magic-property false positives.
phpstan_next.neon	Same PHPStan adjustment for the “next” ruleset.

[JohnVillalovos]

Code Review: feat(calendar): migrate iCalendar export to sabre/vobject

Review written by: Claude Code (Anthropic) — model claude-opus-5

Commit reviewed: 2d30a8c33 on branch jlvillal/1545_c1 (PR #1611, in sync with origin)
Base: 10579c17a (origin/develop tip — linear, no merge commits)

---

Open findings

1. METHOD:REQUEST → METHOD:PUBLISH — undocumented and untested (blocking)

Pages/Export/CalendarExportDisplay.php:29

Verified: 0 occurrences of METHOD in the commit message, 0 in the test file.

The value reaches lib/Email/Messages/ReservationEmailMessage.php:221, the reservation.ics
attached to confirmation emails. Outlook/Exchange render METHOD:REQUEST as an actionable
meeting invitation with Accept/Decline and METHOD:PUBLISH as a plain "add to calendar"
item, so reservation emails visibly change for Outlook users.

Keep the change — RFC 5546 REQUEST requires ATTENDEE, which was never emitted — but
document it and pin it with an assertion. Suggested paragraph:

The calendar now advertises METHOD:PUBLISH rather than the template's
METHOD:REQUEST. The events carry no ATTENDEE properties, so they were
never valid iTIP meeting requests. This is user-visible in the
reservation.ics email attachment, which Outlook previously rendered as
an actionable invitation and now renders as a plain calendar item.

2. No coverage for the VALARM, RRULE, or calendar-name paths

tests/Presenters/CalendarExportPresenterTest.php

Verified 0 hits for METHOD, X-WR-CALNAME, StartReminder, EndReminder, RRULE, and
RecurRule.

Highest value is the two VALARM branches, including the deliberate asymmetry where the start
alarm uses Description and the end alarm uses Summary — that reads like a bug and will be
"fixed" by someone without a test guarding it. A single test can cover METHOD:PUBLISH and
both alarms together, closing most of #1 and #2 at once.

---

Nits

• Pages/Export/CalendarExportDisplay.php:92,100 — new VAlarm($vcal, 'VALARM'); $event->add($alarm); can be $alarm = $event->add('VALARM');, Sabre's own idiom.
• Pages/Export/CalendarExportDisplay.php:80 — getComponents()[0] unguarded. Not reachable
  today through the shell wrapper, but a check is cheap.
• tests/Presenters/CalendarExportPresenterTest.php builds 13 ReservationItemView fixtures
  with near-identical setup; a private helper would tighten the file.
• No Closes: footer (verified absent); the branch name suggests #1545.
• Assisted-by: Claude:claude-sonnet-5 — later rounds of work were claude-opus-5.

---

Worth mentioning in the commit message

Three improvements the message still doesn't claim:

1. ORGANIZER CN is now RFC 6868 quoted. The old template emitted
   ORGANIZER;CN={$Organizer}: raw — an owner name containing ,, ;, or : produced a
   broken property. Sabre emits CN="Doe, Jane; ...".
2. Long lines are folded per RFC 5545 §3.1; the template never folded.
3. The Atom feed is fixed as a side effect. Pages/Export/AtomSubscriptionPage.php:34
   uses $reservation->Summary directly as the Atom <title>; those titles previously
   carried literal \,, \;, \n iCalendar escapes.

Cosmetic wire-format deltas, all RFC-valid, for awareness: CALSCALE:GREGORIAN added,
URL;VALUE=URI: instead of URL:, lowercase mailto:.

---

Verdict

Merge — after fixing #1.

The migration is well-executed and materially more correct than the template it replaces:
proper TEXT escaping, RFC 5545 folding, RFC 6868 parameter quoting, and plugin data that is
parsed rather than concatenated. The ParseException containment is the right call for
plugin-supplied input.

One blocker remains: METHOD:PUBLISH is a silent user-visible change to confirmation emails
that a reviewer shouldn't have to discover by diffing the deleted template. Document it and
pin it with a test. #2 is worth doing in the same push, since one test closes both.

[JohnVillalovos]

AI agent/model: Codex (GPT-5)

Review of PR 1611

Commit reviewed: 2d30a8c33

Findings

1. Medium: Decide and enforce the correct iTIP METHOD

File: Pages/Export/CalendarExportDisplay.php

The deleted template emitted:

METHOD:REQUEST

The new renderer emits:

METHOD:PUBLISH

METHOD declares scheduling intent, so this is an observable behavior
change rather than a serialization detail:

• PUBLISH describes an unsolicited event publication. RFC 5546 requires
  an organizer and forbids attendees.
• REQUEST describes a scheduling request sent to attendees and is what
  clients commonly present as an invitation with Accept/Decline actions.

LibreBooking's normal generated events contain an organizer but no
attendees, making PUBLISH defensible for those events. However,
ExtraIcalLines explicitly supports ATTENDEE, and
testExtraIcalLinesPreservesPropertyParametersAndNestedComponents() adds
one beneath the hard-coded METHOD:PUBLISH. That combination is
non-conformant with RFC 5546.

The same renderer serves calendar downloads, subscription feeds, and
reservation.ics email attachments, even though those contexts may have
different scheduling semantics.

Before merging, choose and encode the intended behavior. Reasonable options
include:

• omit METHOD for generic exports;
• select PUBLISH only for attendee-free publication/subscription output;
• use REQUEST only for actual invitations populated with attendees;
• let each caller select the method explicitly.

Add tests for the chosen behavior, including the ExtraIcalLines attendee
case, and explain the decision in the commit message.

2. Low: Expand coverage for rewritten serialization paths

The following rewritten behavior remains untested:

• serialized newline escaping;
• RRULE output;
• start and end VALARM components;
• the intentional difference between start- and end-reminder descriptions;
• omission and logging of malformed plugin fragments;
• exact timestamp syntax.

The alarm and recurrence paths were fully reimplemented rather than
mechanically moved. Focused output assertions would reduce regression risk.

3. Low: Optional cleanup

• $fragment->getComponents()[0] assumes the constructed shell always
  produces a component. The wrapper or a ParseException currently
  enforces that assumption, but an explicit check would be more defensive.
• The new tests repeat substantial reservation fixture setup; a private
  helper would improve readability.

Verdict

Do not merge as-is.

Before upstream merge:

1. Decide the correct METHOD behavior for each renderer context.
2. Prevent or explicitly handle the non-conformant
   METHOD:PUBLISH + ATTENDEE combination.
3. Add tests for the chosen scheduling semantics.
4. Update the commit message and PR description accordingly.

The remaining coverage and cleanup items are non-blocking.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.