PMK-2059: stop fatalling on valid API responses (v7.0.1) - #167
Open
ewood-ac wants to merge 1 commit into
Open
Conversation
Two production reports, one defect class. Six getters promised a non-nullable return while their constructor explicitly assigns null when the API omits the field, so they were guaranteed to throw TypeError on a perfectly valid response. Reported against PostmarkOpen::getGeo() for a broadcast-stream message with no geo data; a scan of src/Postmark found five more of the same shape. PostmarkClick already declared its equivalents loosely, which is why clicks worked and opens did not -- the inconsistency is the tell that this was an oversight rather than a design. Separately, getBounces()'s $messageID filter became ?int in c7a4371 and shipped from v5.0.1 onward. Postmark MessageIDs are GUIDs, so the filter has been unusable for two years: passing one throws "Argument #7 ($messageID) must be of type ?int, string given". Restored to ?string, matching the pre-v5.0.1 documented type. Shipped as a patch rather than held for 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 were receiving a fatal rather than a value. The one contravariant case is getBounces(), where a subclass override declaring ?int must change; that is noted in the CHANGELOG, and no caller can currently be passing a usable value anyway. Customers hitting a fatal should not have to take a major upgrade (which also drops PHP 8.1) to get the fix. NullableGetterRegressionTest covers all seven and needs no credentials. Verified it fails on unfixed main: 2 errors, 3 failures. Also fixed .php-cs-fixer.dist.php, which built a Finder and never called setFinder(), so the fixer aborted with "You must call one of in() or append() methods" and had never run at all. Added a credential-free static-analysis job to CI -- PHPStan gates, php-cs-fixer is advisory only because 8 files are already non-conforming on main and a check that ships red just teaches people to ignore 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
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.
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.
1. Six getters fatal when the API omits a field
Each of these declares a non-nullable return while its own constructor explicitly assigns
nullwhen the key is absent — so the getter was guaranteed to fatal, not merely at risk of it:PostmarkOpen::getGeo()PostmarkOpen::getClient()PostmarkOpen::getOS()PostmarkMessageBase::getMetadata()PostmarkMessageBase::getMessageStream()WebhookConfiguration::getHttpAuth()The report was for
getGeo(); scanningsrc/Postmarkfor the pattern found the other five.PostmarkClickalready 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 MessageIDc7a4371("Update to level 5 PHPStan") changed the docblock from@param string $messageIDto@param int|nulland added?intto 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 $messageIDwill 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
PostmarkAttachmentsignatures — to get a one-character fix. #164 keeps the genuine breaking changes and will rebase on this.Verification
tests/NullableGetterRegressionTest.phpcovers all seven, needs no credentials, and fails on unfixedmain(2 errors, 3 failures) — confirmed both directions.[OK] No errorsTooling, included because it's what would have caught these
.php-cs-fixer.dist.phpbuilt aFinderand never calledsetFinder(), so the fixer aborted with "You must call one of in() or append() methods". It has never run in this repo. Fixed.static-analysisCI 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 onmainand a check that ships red just trains people to ignore it. Promote it after a formatting pass.