fix(ics): close four defects found reviewing the 2.1.0 range - #254
Merged
Conversation
An ATTENDEE address, a URL and an RRULE are all written to the file as they are given: none of them is a TEXT value, so escapeString() is not there to drop a control character out of them. A CR or an LF therefore ended the property and started another one. guest() already rejected such an address, but Link::$guests is a public property that can be assigned around it, so the encoding now happens where the address is written instead of only where it is added. URL and RRULE were checked for CR and LF alone; every other control character was written out to make a file no parser accepts.
An RRULE repeats the local time of its DTSTART, so an event written as a UTC instant repeats in UTC: a weekly 09:00 in Warsaw became 10:00 from the week the zone moved its clocks. A recurring event in a zone that observes a change is now written as a local time named by a TZID, with a VTIMEZONE to resolve it. UTC, a zone that keeps one offset the year round, and a zone that names no place are all left as they were, since none of them has anything for the occurrences to drift against. The VTIMEZONE components also ran out where their window did, which put occurrences past that year on the wrong offset for part of every later year. The last observance of each kind now carries a yearly rule, but only for a zone that demonstrably repeats that change on a fixed weekday of a fixed month: Africa/Casablanca moves both of its yearly changes for Ramadan and is left without a rule rather than given a guessed one. shouldDefineTimezones() is now derived from referencedTimezones() rather than restating its condition, so an override cannot leave the file with a component defining a zone nothing names.
The alarm read REMINDER.TIME and REMINDER.DESCRIPTION back at generation time and fell through to the default alarm for anything it could not use, so a caller who passed a date string got a reminder fifteen minutes before the event with nothing said about it. An unknown presentation format fell through to the data URI the same way, which handed back a link to a caller who had asked for a file. Both are checked in the constructor now, where DTSTAMP already was: the stack trace points at the caller that supplied the value rather than at whatever renders the link later on.
Documents the behaviour changes the three fixes above introduce, and adds the two entries the 2.1.0 changelog was missing: the bare flag rendering of a false URL parameter, and the description or address of '0' that used to be dropped as if it were empty. Adds a note on the two changes visible to a subclass. Neither touches the public API, but both can stop a 2.0.x subclass loading: the presentation options property gained an array type, and four generator methods went from private to protected, either of which is a fatal error for a subclass that redeclares them the old way.
Four cleanups over the code the previous commits added, with the output byte for byte unchanged. The no-transitions branch built a whole observance by hand rather than letting the synthetic transition fall through the loop that builds every other one. Restoring the fall-through drops the duplicate, and the rule derivation is skipped by counting the table instead, which also stops a fixed offset zone reading years it has no changes in. observesAChange() was answering the same question three times per link, since generate() reaches shouldNameTimezones() once directly and twice more through referencedTimezones(). Reading a zone's transition table is the most expensive thing this class does, so the answer is kept. annualRecurrenceRules() walked the whole table twice, once per kind of observance, carrying a parallel offset variable as it went. One pass keyed by kind says the same thing, and lastOnsetOfEachKind() gives the step a name. annualRuleFor() used one variable as both the agreed week of the month and the flag for there being no agreement, and `??=` then re-seeded it after it had been cleared: onsets in weeks 2, 3 and 4 came back out as week 4 rather than as no rule at all. Collecting the parts and comparing them at the end removes the state that made that possible.
It sat immediately above the method that reads it, which put a property declaration in the middle of the method block. The reasoning for keeping the answer moves up with it, and observesAChange() points at it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context and Purposes
A review of everything that landed between
2.0.1andmasterturned up four defects in the ICS generator and two gaps in the changelog. Each finding was reproduced before it was fixed, and each fix comes with the test that reproduces it.An attendee address could inject calendar properties
Ics::escapeCalendarAddress()percent encodes the characters that would change the meaning of amailtoURI, but not a control character.Link::guest()rejects one, so the ordinary path was safe.Link::$guestsis a public property, though, and an address assigned straight to it never passes that check:Control characters are now percent encoded where the address is written, so the guard does not depend on which door the address came in by.
URLandRRULEwere checked for CR and LF alone for the same reason (neither is a TEXT value thatescapeString()cleans up), and now reject every control character, which none ofURI(section 3.3.13) orRECUR(section 3.3.10) admits.A recurring event drifted an hour across daylight saving
An
RRULErepeats the local time of itsDTSTART(section 3.8.5.3), so an event written as a UTC instant repeats in UTC. A weekly 09:00 standup in Warsaw came out as:Every occurrence from 2026-03-30 onwards lands at 10:00 local.
RRULEsupport is new in this release, so the broken combination is new with it.A recurring event in a zone that observes a change is now written as a local time named by a
TZID, with theVTIMEZONEto resolve it. Everything else is untouched: an event without anRRULE, a recurrence in UTC or in a zone that keeps one offset the year round (Asia/Tokyo), and a zone that names no place all keep the UTC instants they had, since none of them has anything to drift against.The VTIMEZONE components stopped where their window did
The components list every change the zone makes in the year either side of the event and carried no recurrence rule, so a client resolved every occurrence past that year against the last change written, which is the wrong offset for part of each later year. RFC 5545 asks a referenced
VTIMEZONEto cover the recurrences that lean on it (section 3.6.5).The last observance of each kind now carries a yearly rule, derived only where the zone demonstrably repeats that change: the onsets past the window are read as well, and all of them have to fall on the same weekday of the same week of the same month at the same local time before a rule is written.
Verified against the EU rule (
Europe/Warsaw, last Sunday of March and October), the US rule (America/New_York, second Sunday of March and first of November), and the southern hemisphere (Australia/Lord_Howe, first Sunday of October and April).Asia/TokyoandAsia/Tehranobserve nothing and get no rule.Africa/Casablanca, which suspends its summer time for Ramadan and restores it weeks later, moves both of its yearly changes and is deliberately left without a rule rather than given a guessed one.Two options were silently ignored when they could not be used
REMINDER.TIMEandREMINDER.DESCRIPTIONwere read back at generation time and dropped for the defaults if they were of the wrong type, so a caller who passed a date string got a reminder fifteen minutes before the event and no indication that theirs had been discarded. An unrecognised presentationformatfell through to the data URI the same way, so['format' => 'FILE']returned a link to a caller who had asked for a file.Both are checked in the constructor now, where
DTSTAMPalready was, and for the reason its comment gives: the stack trace points at the caller that supplied the value.Changelog
The 2.1.0 entry was missing the bare flag rendering of a
falseURL parameter and the description or address of'0'that used to be dropped as if it were empty. Both are user visible and are now listed.A
Notes for subclassessection records the two changes that can stop a 2.0.x subclass loading. Neither touches the public API and neither is introduced by this PR, but both are fatal errors rather than deprecations, so they are worth stating outright:Not addressed
IANA backward aliases are still read as distinct zones, so
Japanpaired withAsia/Tokyowrites twoTZIDvalues and twoVTIMEZONEcomponents for one physical zone, andEST5EDTat either end sends both ends to the UTC fallback. Times stay correct in both cases and only the zone identity is lost, so it is left for its own change.Tests
12 new tests, 229 total, alongside
psalmaterrorLevel 1andphp-cs-fixerboth clean. The one snapshot change is the twoRRULElines added to the flight'sVTIMEZONEcomponents.