Fix the four regressions this campaign's own fixes introduced - #8
Merged
Conversation
Found by the re-review, which is what it is for. Each is a case where a fix added a
call that can fail into a path whose surrounding design assumes it cannot.
1. A GATEWAY OUTAGE ON RENEWAL KILLED THE JOB INSTEAD OF OPENING DUNNING (P1).
The off-session fix added `customers->resolve()` and `gateway->paymentMethods()`
to PaymentService::pay(). Both talk to the gateway and both throw; nothing further
up catches — not chargeRenewal(), not RenewSubscriptionJob. charge() itself
catches precisely so dunning opens, so the new calls sat UPSTREAM of the only
protection. A brief incident during the monthly run killed the job after the
period had advanced and the invoice was issued: invoice left Open, dunning never
opened, subscription still serving, nobody notified. Both now degrade to a FAILED
result, which is the recoverable outcome.
2. GET /billing/portal/{token} MINTED A GATEWAY CUSTOMER (P2). gatewayAccountFor()
resolves through a seam that CREATES on miss, so merely loading the portal page
wrote a Stripe customer and a gateway_customers row — a non-idempotent side effect
on a GET — and a gateway outage 500'd the whole page including the invoice history
and cancel flow, which need no gateway at all. Now memoised per request (it was
re-resolving up to three times in removeMethod()) and the page renders an empty
saved-cards panel rather than failing.
3. THE ORG-REF MIDDLEWARE HYDRATED A MODEL TO RETURN ITS OWN ARGUMENT (P2). It ran
`find()` — a full Eloquent model, every column — to return the string it was
handed, on every authenticated request including /reserve and /usage at 600/min
per token, and resolved twice when `org` arrived in both path and body. Now one
non-hydrating `value('id')` with an orWhere, memoised per request.
4. ONE GARBLED CURRENCY CELL ABORTED AN ENTIRE IMPORT RUN (P2). MinorUnits::parse()
catches MathException, but Currency::of() throws UnknownCurrencyException — an
unrelated class — so the throw escaped through parse() with no handler and lost a
10 000-record dry-run to one bad cell. The importer's whole design is per-record
outcomes; the adapter now returns null and BillingImporter lands the record as the
Conflict it already knows how to report.
Regression tests cover the two that are observable end to end: a gateway that throws
on listing methods still yields Failed, and an export carrying an unknown currency
still parses.
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.
Found by the re-review pass. Each is the same shape: a fix added a call that can fail, into a path whose surrounding design assumes it cannot.
GET /portal/{token}minted a gateway customer, and resolved up to 3×The P1 is the one that matters.
PaymentService::pay()gained two gateway calls, andcharge()catches exceptions precisely so dunning opens — the new calls sat upstream of the only protection. A brief Stripe incident during the monthly run killed the job after the period had advanced and the invoice was issued: invoice leftOpen, dunning never opened, subscription still serving, nobody notified. Strictly worse than the decline it replaced, because a decline is visible.#4 is a nice illustration of an exception-type assumption.
MinorUnits::parse()catchesMathException;Currency::of()throwsUnknownCurrencyException, which is a plainRuntimeException. Unrelated hierarchies, so the catch never fired.Gate
pint · PHPStan level max (0 errors) · 1034 tests ·
composer audit— green.