DX review of v1.0.0: findings and follow-ups

[loevgaard]

A developer-experience review of setono/quickpay-php-sdk at v1.0.0, done with a "first-time consumer" lens: the public API surface, README/docblocks, examples, tests, and a cold composer require into a fresh project. Endpoint/parameter facts below were checked against Quickpay's machine-readable spec (https://api.quickpay.net/docs/v10/merchant/api/payments).

Verdict: the fundamentals (typing, safety, honesty about API quirks, tests, docblocks) are excellent. The gaps are mostly around the core — a few "obvious next things" a consumer reaches for, one real inconsistency in the security story, and a README that explains the SDK well but doesn't yet teach the Quickpay mental model or show framework-shaped code. Nothing needs a 2.0; almost everything is additive.

Each item below is fixed in its own PR referencing this issue.

What's already excellent (protect this)

• Required-vs-optional on request DTOs verified against the live API, not the docs.
• $raw escape hatch + non-exhaustive enums (state()/type() via tryFrom) — the SDK won't break on API evolution.
• Async operations modeled honestly (synchronized per call + client default; docblocks spell out what a 202 body does and doesn't tell you).
• Callback design: raw-body HMAC, hash_equals, strict resource-type validation, "don't assume it's a payment", the "don't mock the handler — forge signatures with sign()" testing section.
• Exception hierarchy under one marker interface, lazy-parsed Quickpay error body, sanitized URLs.
• Zero-mock test suite with a URI-keyed fake; e2e harness with a real callback loop.

P1 — fix soon

• 1. Client::request() bypasses the host-pinning guard. src/Client/Client.php request() stamps Authorization on any RequestInterface and sends it; only get()/send() go through resolveUrl(). Since request() is public on ClientInterface, a consumer using it to reach an unmodeled endpoint can leak the key to another host with no error, contradicting the README/release-notes claim ("credentials only ever go to api.quickpay.net"). Fix: run the same host/port check on $request->getUri() in request(), with a test.
• 2. Stale docblock: ClientInterface::put() says it is "Used for updating a payment" — update is PATCH now.

P2 — API ergonomics gaps (additive, 1.x-safe)

• 3. No way to find a payment by order_id. Quickpay rejects duplicate order_id ("has already been taken"), so every integration needs find-or-create — but CollectionRequestOptions only carries page/pageSize, while GET /payments supports order_id, state, accepted, min_time/max_time, acquirer, fraud_suspected, id, sort_by/sort_dir, operations_size. Add PaymentsEndpoint::findByOrderId(string): ?Payment plus a typed filter/query object, and a README recipe "create a payment idempotently".
• 4. Consumers hand-roll the same result helpers. "Was the capture approved?" = find the operation with type=capture, pending=false, qp_status_code='20000'. Add small, tested helpers: Operation::isApproved(), Payment::operationsOfType(), latestOperation(), operation(int $id), and derived amounts (authorizedAmount(), capturedAmount(), refundedAmount()).
• 5. Escape hatch is incomplete and undocumented. The SDK is deliberately narrow (no renew, session, DELETE …/link, operations sub-resource, subscriptions), so consumers will hit the boundary. There is no delete() helper, post()/put()/patch() accept only ?Payload (and every concrete Payload is final, so a consumer must subclass the abstract base), and the README never mentions get()/post() as the way to reach unmodeled endpoints. Add delete(), accept Payload|array|null bodies, and document "Calling endpoints the SDK doesn't model".
• 6. Testing story for consumers is missing. Client/PaymentsEndpoint are final (good) and the intended substitution point is the PSR-18 client — but ScriptedHttpClient lives in tests/ and is export-ignored, and the README's only testing guidance is about callbacks. Ship a Setono\Quickpay\Testing\ fake and add a README section "Testing code that uses the SDK" (also noting response DTOs have public constructors).
• 7. Cache wiring is a footgun. Passing a plain (new MapperBuilder())->withCache($cache) without wrapping it in Client::configureMapperBuilder() compiles fine and fails at runtime (dates, superfluous keys). Add a cache: constructor argument on Client and CallbackHandler; keep the builder params for power users.
• 8. handleRaw() drops headers and forces a superglobals dance. It can't populate accountId/apiVersion, and Symfony/Laravel Requests aren't PSR-7 without a bridge — so handleRaw() is what most people will actually use. Accept the optional headers and add handleGlobals() (php://input + $_SERVER), which examples/e2e/listen.php currently reimplements.
• 9. Transport errors escape the marker interface. catch (QuickpayException $e) misses PSR-18 ClientExceptionInterface (timeouts, DNS), although the QuickpayException docblock promises it nets "all SDK-thrown exceptions". Wrap in TransportException implements QuickpayException, ClientExceptionInterface (BC-safe for existing ClientExceptionInterface catches) and mention it in the README's error section.
• 10. Smaller additive items: memoize Callback::payment() (re-decodes/re-maps on every call); type Payment::$variables (the README shows setting variables but never reading them back — today that's $raw['variables']), deadlineAt, acquirer; add CreatePaymentRequest::$shopsystem (shopsystem[name|version], useful for a plugin to identify itself); include the SDK version in the User-Agent.

P3 — docs & examples

• 11. Install text is stale/misleading (README.md Installation). A cold install into a fresh project: with the discovery plugin allowed, composer require auto-installs symfony/http-client + nyholm/psr7; with it disallowed, install succeeds silently and new Client() throws php-http's NotFoundException (not a QuickpayException). Say both things.
• 12. Teach the mental model, not just the SDK. Add a "Concepts" section (the two keys, amounts as minor units, state + accepted + operations, sync vs async, test mode) and recipes: full checkout flow, Symfony and Laravel callback controllers, find-or-create by order id, testing your code.
• 13. Promote a critical warning from the e2e README to the main README: the continue_url redirect carries no data and can arrive before the callback — trust only the verified callback or a getById(). Plus callback best practices (respond 2xx fast, expect retries/out-of-order, key idempotency on operations[].id).
• 14. Docs polish: the README wants a TOC; createLink() returns a Link whose 5 non-url fields are always null (the endpoint returns only {url}) — document that on Link; the four near-identical 8-line paragraphs in PaymentsEndpoint could point to one shared explanation.
• 15. Packaging polish: composer.json has no keywords/support/homepage; README.md/UPGRADE.md are export-ignored so vendor/setono/quickpay-php-sdk/ ships no docs at all; consider a CHANGELOG.md mirroring the GitHub release notes.

2.0 candidates (don't break 1.x for these)

• updatePayment() → update() (create/getById/capture are otherwise noun-free).
• Drop Collection::$totalCount/$totalPages — fields documented as "should not be relied upon" shouldn't exist.
• Split the response Link (createLink() result vs the nested link on Payment), or return string $url.

[loevgaard]

All findings now have a PR — each is independent (based on 1.x) and self-contained with tests + its own README lines, except #20 which should be merged last (it links to the others' sections):

Findings	PR
1, 2 — request() host guard, put() docblock	#11
3 — findByOrderId() + typed PaymentsQuery filters	#12
4 — Payment/Operation result helpers	#13
5 — escape hatch: delete(), array bodies, deleteLink(), README section	#14 (BC note for ClientInterface implementors)
6 — shipped Testing\ScriptedHttpClient + "Testing code that uses the SDK"	#15
7 — cache: argument on Client / CallbackHandler	#16
8 — handleRaw() headers, handleGlobals(), framework snippets	#17
9 — TransportException	#18 (BC note for NetworkExceptionInterface catches)
10 — variables(), deadlineAt/acquirer, Shopsystem, memoized payment(), UA version	#19
11–14 — install text, TOC, Concepts, callback best practices, recipes, docblocks	#20 (merge last)
15 — composer metadata, CHANGELOG, docs in dist	#21 (dist change is a separate, droppable commit)

Verified against the live API along the way (read-only probes + a few throwaway unpaid test payments): order_id filter is exact/case-sensitive and order_id is unique per account; boolean/time filter encodings; the live shapes of variables / deadline_at / acquirer; and that Valinor's camelCaseKeys() recurses (which is why variables is a $raw-reading method, not a mapped property).

Since the PRs touch nearby README regions, a few may need a trivial conflict resolution depending on merge order — happy to rebase any of them.