PMK-2059: stop fatalling on valid API responses (v7.0.1)

[ewood-ac]

Jira: PMK-2059

Patch release for the two customer-reported defects on PMK-2059. Both are fatals on responses the API legitimately returns, and neither should require a major upgrade to escape.

Supersedes #166, which GitHub closed automatically when the branch was renamed to match the ticket. Same commit, no content change.

1. Six getters fatal when the API omits a field

Fatal error: Uncaught TypeError: Postmark\Models\PostmarkOpen::getGeo():
Return value must be of type Postmark\Models\PostmarkGeographyInfo, null returned

Each of these declares a non-nullable return while its own constructor explicitly assigns null when the key is absent — so the getter was guaranteed to fatal, not merely at risk of it:

PostmarkOpen::getGeo()	reported — broadcast-stream message with no geo data
PostmarkOpen::getClient()	same class, unreported
PostmarkOpen::getOS()	same class, unreported
PostmarkMessageBase::getMetadata()	base of the outbound send payload
PostmarkMessageBase::getMessageStream()	same
WebhookConfiguration::getHttpAuth()	any webhook without basic auth

The report was for getGeo(); scanning src/Postmark for the pattern found the other five. PostmarkClick already declares its three equivalents loosely, which is why clicks worked and opens didn't — that inconsistency is what marks this as an oversight rather than a design.

2. getBounces() can't filter by MessageID

Postmark\PostmarkClient::getBounces(): Argument #7 ($messageID) must be of type ?int, string given

c7a4371 ("Update to level 5 PHPStan") changed the docblock from @param string $messageID to @param int|null and added ?int to the signature. It shipped in v5.0.1 and every release since. Postmark MessageIDs are GUIDs, so the filter has been unusable for over two years — nobody can be passing a valid value as an int. Restored to ?string.

The reporter's diagnosis was exactly right, including the commit.

Why a patch and not the v8 branch

Widening a return type to nullable is covariant, so a subclass overriding one of these getters with the narrower type stays compatible — no class-load break. And callers weren't getting a wrong value, they were getting a fatal.

The one contravariant case is getBounces(): a subclass that overrides it with ?int $messageID will need to change. That's called out in the CHANGELOG, and no caller can currently be passing a usable value regardless.

Holding these for #164 would mean a customer with a production fatal has to accept a major upgrade — which also drops PHP 8.1 and tightens PostmarkAttachment signatures — to get a one-character fix. #164 keeps the genuine breaking changes and will rebase on this.

Verification

tests/NullableGetterRegressionTest.php covers all seven, needs no credentials, and fails on unfixed main (2 errors, 3 failures) — confirmed both directions.

• PHPStan: [OK] No errors
• New tests: 5 passing, 14 assertions

Tooling, included because it's what would have caught these

• .php-cs-fixer.dist.php built a Finder and never called setFinder(), so the fixer aborted with "You must call one of in() or append() methods". It has never run in this repo. Fixed.
• Added a credential-free static-analysis CI job. Every existing job is an integration suite against the live API that can't start without tokens, so this is the only check a fork PR can currently exercise. PHPStan gates; php-cs-fixer is advisory only, because 8 files are already non-conforming on main and a check that ships red just trains people to ignore it. Promote it after a formatting pass.

[ac-bgelfius]

Final review — postmark-php #167 (c2e1d3d6, base main)

Recommendation: approve-with-should-fix. The compatibility reasoning is right and the fix is
right. One thing to change before you tag: call it v7.1.0, not v7.0.1.

No blockers. Nothing in the code is wrong.

---

What's good, and it's most of the PR

• The variance argument holds. Widening those six returns to nullable is covariant, so a
  subclass keeping the narrower non-nullable return still loads. Two seats checked this by
  executing it, not by reading the spec, and both got the same answer. getBounces() is genuinely
  the only contravariant change in the diff — a script over every removed signature in the diff
  turned up nothing else.
• "Guaranteed to fatal" is accurate for five of the six. ?array $Metadata, ?string $MessageStream (PostmarkMessageBase.php:17-18,32-33), ?PostmarkGeographyInfo $Geo,
  ?PostmarkAgentInfo $Client, ?PostmarkAgentInfo $OS (PostmarkOpen.php:9-15) each get null
  from the constructor against a non-nullable getter. And PostmarkClick.php:85,118,139 really
  are already mixed in the same three places — the Open/Click asymmetry is the evidence that this
  was oversight, not design. That framing is correct.
• Splitting the patch off the major is the right customer call. Somebody with a production
  fatal shouldn't have to take a PHP-8.2 floor and tightened PostmarkAttachment signatures to
  escape it. That asymmetry — fix the thing that's breaking them fast and small, put the thing that
  changes their code behind a major — is what you want here.
• The new static-analysis job is well-built and its comment is true. It is the only job in
  the workflow needing no Postmark credentials, so it's the only one that gives a fork PR any
  signal at all. That works because CircleCI withholds project env vars from forked-PR builds —
  the design leans on the platform's own protection rather than routing secrets around it.

---

Should-fix

1. Ship it as v7.1.0, not v7.0.1. CHANGELOG.md:8

Your argument is good, and it defends value safety: nobody can pass a working int MessageID
today — they're GUIDs, and the signature has demanded ?int since v5.0.1. Agreed, no working
caller breaks.

The break isn't about values. It's about declarations. A subclass overriding
getBounces(?int $messageID) fatals at class load the moment the parent widens to ?string —
whether or not anyone ever calls it. Those are independent facts, and the second one isn't
addressed by the first, because the fatal fires at autoload time before any call happens.

Verified by execution, and this is the part worth knowing: there is no signature for $messageID
that keeps an existing ?int subclass loading. Not ?string, not int|string|null, not
untyped. "Fix it without breaking subclasses" was never on the table — which is fine, the fix is
still right. It just means the release carries a load-compat break.

So who's affected by the digit? Almost nobody, and that's the point of asking. ^7.0 — what
composer require writes — takes 7.0.1 and 7.1.0 identically. The only cohort that gets 7.0.1 but
not 7.1.0 is one pinned 7.0.*: people who explicitly asked for bugfixes and nothing else. They're
exactly the people who shouldn't silently receive a load-compat signature change. There's no
release, tag, or publish automation in .github/ or .circleci/config.yml, so the version number
is a pure editorial choice — a CHANGELOG heading and a git tag. One digit, no cost, and the
contract stops lying.

2. "None should require a change on your side" isn't true for PHPStan ≥8 or Psalm users.
CHANGELOG.md:26-31

Covariance protects subclasses. It says nothing about callers' static analysis. Verified against a
two-line consumer calling getGeo()->getCountry(): PHPStan level 5 is clean, level 8 reports
Cannot call method getCountry() on Geo|null on consumer code that didn't change, after a
composer update inside ^7.0.

Their code still runs. Their pipeline stops. This is the surface most upgraders will actually hit,
and it's the one the notes don't mention. One sentence fixes it — something like: "if you run
PHPStan ≥8 or Psalm, these getters are now nullable and your existing ->getGeo()->… chains will
need a null check."

3. .php-cs-fixer.cache is committed and not gitignored.

It's added as a new tracked file by this PR (+1 line), and .gitignore covers only
.phpunit.cache/ and .phpunit.result.cache. It bakes in the PHP runtime and fixer version of
whoever generated it ("php":"8.4.4","version":"3.82.2") plus a hash per file — so it goes stale
immediately and produces noise diffs on unrelated PRs. Worse: running the advisory fixer step this
PR adds rewrites it (verified — php-cs-fixer fix --dry-run at this head leaves
M .php-cs-fixer.cache), so every contributor and every CI run gets a dirty tree.
git rm --cached .php-cs-fixer.cache plus a .gitignore line.

---

Nits

• CHANGELOG.md:11-16 — the getHttpAuth() justification is overstated. "Its constructor
  explicitly assigns null when the API omits the field" is true of five of the six, not of this
  one: WebhookConfiguration.php:24 and WebhookConfigurationListingResponse.php:26 both
  unconditionally substitute a real HttpAuth object on every API-response path. Null is only
  reachable through the public constructor or Build() — which is what your new test actually
  exercises. The fix is right and consumers null-check the same way either way; this is
  documentation precision. The test docblock ("a webhook configured without basic auth — the common
  case — returned null") says the same wrong thing.
• TemplateValidationResponse.php:70 — the sweep missed one. getSuggestedTemplateModel(): array over public ?array $SuggestedTemplateModel, same defect class. A sweep of all 75 src/
  files found 7 instances on main; this PR fixes 6 and this is the survivor. Lower risk — the
  constructor assigns [], not null, so it's only reachable by setting the public property
  directly. Cheap to close while you're here.
• Getter and setter now disagree — PostmarkMessageBase.php:189/194 and
  WebhookConfiguration.php:119/124. getMessageStream(): ?string vs setMessageStream(string),
  so $m->setMessageStream($m->getMessageStream()) now fatals. Widening the setters is
  contravariant, so v8 is the right place for it — but it belongs in the upgrader note rather than
  being discovered later.
• strict_types is worth a clause next to the subclass note. No src/ file declares it, so
  coercion mode is set by the caller's file: a weak-mode caller passing getBounces(…, 12345)
  silently coerces to '12345', while a caller under declare(strict_types=1) gets a TypeError.
  Same break reaching plain callers, not just subclassers.
• tests/NullableGetterRegressionTest.php:24 is missing the @internal / @coversNothing
  docblock every other test class carries — and the advisory fixer job this PR adds flags this file.

---

About the red php81 check

It is not caused by this PR, and the review confirmed that rather than assuming it.

php81 has been red on main since 2025-10-24; this branch inherited it. Two independent
confirmations: the actual CircleCI build output for this head was pulled via the public v1.1 build
API (postmark-php is public, no token needed), and the suite was reproduced locally in a
php:8.1-cli container. Both show the same thing — 79 identical
PostmarkClient::__construct(): Argument #1 ($serverToken) must be of type string, null given
errors, plus sender-signature and API-token failures. That's account state and credentials, not a
PHP 8.1 incompatibility. At this head the local run was Tests: 80, Assertions: 15, Errors: 79 —
the same 79, plus your five new passing regression tests.

Worth one line in the PR body, though: it currently doesn't mention the red check at all, and a
reviewer skimming CI would reasonably read it as caused by this diff.

---

Open questions

• Any support or crash-report signal on how many customers are hitting these fatals beyond the one
  reporter? Doesn't change whether to ship — it would tell you whether this needs outreach beyond
  the CHANGELOG.
• "MessageIDs are GUIDs" is load-bearing for the no-working-callers argument, and its only source
  in this review chain is the new docblock at PostmarkClient.php:309 — i.e. the diff itself.
  [NEEDS VERIFICATION] as independent provenance. Low stakes; it's a well-established product fact,
  not a surprising claim.

Sequencing note

#168's base is this branch, so #167's content ships with #168 either way; v7.0.1 (v7.1.0) exists so
PHP 8.1 consumers can take the fatal fix without the 8.2 floor. Stated as fact, no ask.

---

Coverage note

Six seats ran, all full passes, none abstained: php, software-architect, circleci,
product, engineering-manager, cost, plus a devil's-advocate adjudication pass.

• The php seat is normally calibrated for Postmark's Craft CMS 4 / Yii2 estate — none of which
  applies to a standalone SDK. It was re-aimed at the transferable axes: PHP type-system
  correctness, null handling, composer supply chain, static-analysis config, with published-library
  API compatibility and SemVer as the governing concern. That's the right frame for this repo.
• No local PHP or composer on the review machine. Everything marked verified was executed in
  php:8.1-cli / php:8.2-cli / composer:2 containers, network-disabled for anything touching
  the test suite. The live Postmark API was never called.
• Not applicable to this PR, so not covered: no AWS, no Bedrock, no per-message cost surface, no
  Rails/Ruby, no Terraform, no production infrastructure. Cost review was confined to CircleCI
  credits, where #167's delta is one added job.
• Resolved reviewer conflict: the architect returned request-changes on this PR while capping
  its own finding at should-fix and writing that nothing in the code is wrong. The devil caught the
  mismatch; I've adopted its call. A zero-code, one-line CHANGELOG edit doesn't need a formal gate.
  On the substance of v7.1.0 the architect is upheld over the php seat, for the declaration-vs-value
  reason above.
• #168 is reviewed separately. Findings against its commits are not in this document.