Make unconditionally-required request fields required constructor params - #2
Merged
Conversation
Verified against the live Quickpay API (not just the docs) with empty-body
probes — the API validates body params before transaction state, so an
unauthorized probe payment sufficed:
- POST /payments without order_id -> 400 order_id length error;
with a valid order_id but no currency -> 400 'currency is missing'
- PUT /payments/{id}/link without amount -> 400 'amount is missing';
with only amount -> 2xx
- capture/refund/authorize with an empty body -> 400 'body is invalid'
(with amount they proceed to state/acquirer checks)
CreatePaymentRequest::$orderId/$currency and $amount on CreateLinkRequest,
CaptureRequest, RefundRequest and AuthorizePaymentRequest are now required,
non-nullable constructor parameters, so a missing one fails at the call
site (and in static analysis) instead of as a ValidationException after a
network round-trip. A bodyless authorize is never valid, so
PaymentsEndpoint::authorize() now requires its request too.
All required fields were already the first constructor parameters, so
positional callers are unaffected. Conditionally-required fields stay
optional; there is still no construction-time validation logic.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 1.x #2 +/- ##
============================================
+ Coverage 92.05% 92.30% +0.24%
- Complexity 147 148 +1
============================================
Files 24 24
Lines 403 403
============================================
+ Hits 371 372 +1
+ Misses 32 31 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The first probe run sent an empty Payload, which the normalizer encodes
as [] (a JSON array) — the API's 'body is invalid' was about that shape,
not the missing amount. Re-probed with proper {} bodies and a test-card
authorization (test_mode, charges nothing):
- validation order is body shape -> transaction state -> params
- capture without amount on an AUTHORIZED payment: 400 'amount is missing'
- refund without amount on a CAPTURED payment: 400 'amount is missing'
(the API does not fall back to capturing/refunding the remaining balance)
- authorize with card data but no amount: 400 'amount is missing'
So amount is genuinely required for all three operations and the required
constructor params stand; docblocks now cite the real evidence.
The probe also exposed an SDK bug: any Payload with no set fields was
sent as [] and rejected by the API. Client::send() now rewrites the
empty-array encoding to {}, with a regression test.
This was referenced Aug 6, 2026
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.
Why
The request DTOs were all-optional by design ("the server is the source of truth"). That rule is right as a default, but for fields the API unconditionally requires it trades a call-site/static-analysis error for a
ValidationExceptionafter a network round-trip. Tightening now is cheap (alpha) and BC-safe to relax later; the reverse is a BC break.Verified against the live API
Every field was probed with real requests. The API's validation order is body shape → transaction state → params, so the operation params were verified on payments in the right state, obtained by authorizing with a Quickpay test card via the API (
test_mode, charges nothing):POST /paymentswithoutorder_idorder_id: must have length between 4 and 20POST /paymentsvalidorder_id, nocurrencycurrency: is missingPUT /payments/{id}/linkbody{}amount: is missingPUT /payments/{id}/linkonlyamountauthorizewith card data, noamountamount: [is missing, is empty]capture {}on an authorized paymentamount: [is missing, is empty]refund {}on a captured payment (balance 1000)amount: [is missing, is empty]Notably the API does not fall back to capturing/refunding the remaining balance when
amountis omitted — it rejects the operation outright.What
Required, non-nullable constructor parameters (all were already the first parameters, so positional callers are unaffected):
CreatePaymentRequest::$orderId,::$currencyCreateLinkRequest::$amountCaptureRequest::$amountRefundRequest::$amountAuthorizePaymentRequest::$amountAdditionally
PaymentsEndpoint::authorize()now requires its$request— the API validatesamountas required, so a request-less authorize can never succeed.Bug fix found by the probing: a
Payloadwith no set fields normalizes to an empty PHP array, which JSON-encodes as[]— the API rejects that shape withbody: "is invalid".Client::send()now rewrites the empty-array encoding to{}(with a regression test). Before this PR, e.g.updatePayment($id, new UpdatePaymentRequest())sent a body the API always rejected.Conditionally-required fields (acquirer/method-dependent) stay optional,
UpdatePaymentRequeststays all-optional (PATCH semantics), and there is still no construction-time validation logic — this is types only.Payloadand CLAUDE.md document the refined rule, including the live-probe methodology (validation ordering + test-card authorization).BC
Breaking for consumers who constructed these DTOs without the required fields (such requests were always rejected by the API) or called
authorize($id)without a request. Pre-1.0 alpha, so no major-version concern — should land before 1.0.0 stable.Tests
it_authorizes_without_a_body→it_authorizes_with_an_amount_body(the old test encoded a request the API always rejects).it_sends_an_empty_json_object_for_an_empty_payload.php -lon the e2e scripts, and Infection all green (covered MSI 76% ≥ 70 gate; no escaped mutants in the changed code).