Skip to content

Assert timezone invariants across every TZDB identifier, instead of fixing zone classes one at a time #252

Description

@alies-dev

What do you want to achieve?

Catch a whole class of timezone bug automatically, instead of one zone at a time.

Every timezone defect found while closing #234, #235, #238 and #239 had the same shape: a category of DateTimeZone name nobody had thought of. Each was found by hand or by a reviewer, one at a time, and each needed its own targeted fix and its own targeted test:

Zone What happened
Japan, GB, Singapore, US/Eastern IANA backward aliases. Silently downgraded to UTC, and separately treated as naming no place, which collapsed a Tokyo to Seoul flight into one zone
+02:00, CEST Wrote DTSTART;TZID=+02:00:..., where the unquoted colon truncates the property value, and a ctz Google ignores
EST, CET DateTimeZone::getTransitions() returns false, which raised a PHP warning and emitted BEGIN:VTIMEZONE / END:VTIMEZONE with no observance, invalid per RFC 5545 §3.6.5
Africa/Casablanca Two STANDARD transitions in one window (DST suspended for Ramadan, then restored). The emitted VTIMEZONE resolved the event's own start an hour late
gmt+0, gmt-0 Lowercase keeps the literal name where uppercase normalises to +00:00, so they were classified as places

Five separate discoveries, all the same question: does this class of zone name break something. That question should be asked by a test, not by whoever happens to be reading the diff.

Proposal

One test that walks DateTimeZone::listIdentifiers(DateTimeZone::ALL_WITH_BC), plus a fixed list of offset and abbreviation zones (+02:00, -05:00, Z, CEST, GMT, gmt+0, UTC), building a link in each zone and asserting invariants that must hold for every one of them:

  1. No emitted TZID parameter value contains a : (RFC 5545 §3.1 forbids it unquoted in a param-value).
  2. Every emitted VTIMEZONE contains at least one STANDARD or DAYLIGHT subcomponent (§3.6.5).
  3. For each endpoint written with a TZID, the VTIMEZONE in the file resolves that endpoint to the offset PHP reports for that zone at that instant. This is the one that catches Casablanca.
  4. A ctz, stz or etz parameter is either absent or a name accepted by Link::hasResolvableTimezones().
  5. Every generator emits no PHP warning, notice or deprecation for any zone.

Invariant 5 alone would have caught the getTransitions() bug, and invariant 3 the Casablanca one, before either reached review.

A second pass over pairs is worth having for the two-timezone path, but the pair space is too large to enumerate fully. A sample is enough: each zone against a fixed partner, plus the handful of pairs already known to be interesting (alias against its canonical name, two places sharing an offset, one resolvable end and one not).

Notes

  • Runtime matters. About 420 identifiers times a few generators is fine, but the pair pass needs sampling rather than a full cross product.
  • The tzdb ships with PHP and moves. An assertion tied to a specific transition date will break on a tzdata update, so the invariants should be derived from DateTimeZone at runtime rather than hardcoded, which is what invariant 3 does.
  • The CI matrix runs 8.3, 8.4 and 8.5, which may carry different tzdata. That is a feature here: it widens the search.
  • This replaces nothing. The targeted regression tests added with each fix stay, since they document why each rule exists.

Related, but deliberately out of scope

Invariant 4 checks that a ctz, stz or etz is a name Link::hasResolvableTimezones() accepts. That proves the generators obey our own rules. It does not prove the rules are right, because no offline test knows what Google actually resolves.

That ground truth has to come from live checks against the services, which need a logged in account and cannot run in CI, so it belongs in a separate issue rather than here. It matters because some of the current rules are stricter than reality may require: the whole Etc/ tree is refused including canonical Etc/UTC (see #248), while US/Pacific is accepted, and only some of those cases were ever confirmed against the live service.

If such a check is ever run, its results feed straight back into this test as the expected set for invariant 4.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions