2.x review: robustness findings, gaps vs other Payum integrations, and test coverage

[loevgaard]

Progress

Each finding is being fixed in its own PR (stacked where they touch the same files). This checklist is updated as PRs open.

#	Finding	PR
R1	ConfirmPaymentAction auto-captures rejected/pending authorize → 500-retry loop	#58 ✅ merged
R2	StatusAction degrades on rejected/pending trailing operations	#65 (supersedes #59)
R3	Missing quickpayPaymentId guards in Authorize/Capture/Refund/Cancel	#60 ✅ merged
R4	Link-level vs callback-level auto-capture overlap	needs live verification first — no PR yet
R5	Factory option coercion sharp edges ('true' → false etc.)	#61 ✅ merged
R6	Amounts silently truncates decimals	#62
R7	Currency drift after creation unreconciled	#63
G1	Capture is not the entry point; README example misleading	#66 (docs)
G2	Ship HeaderAwareGetHttpRequestAction in src/	#64
G3	Two-URL callback reality not in README	#66 (docs)
G4	Sylius wiring section	#66 (docs)
G5	Scope statement (no card API / subscriptions / payouts)	#66 (docs)
T1–T5	Test gaps (rejected authorize, checksum header shapes, AuthorizeAction, integration test)	#58, #65, #67
T6	PHPUnit attribute migration	deferred until the PRs above land (would conflict with all of them)

---

A thorough review of the 2.x branch: architecture, robustness, gaps compared to other Payum integrations, and test coverage. Overall the gateway is in very good shape — thin actions over a typed SDK, scalar-only details contract, balance-driven status mapping, HMAC-verified callbacks. The findings below are ordered by severity; the first one is a real production bug.

Robustness

1. Bug: a declined payment with auto_capture on puts the notify endpoint into a 500-retry loop

ConfirmPaymentAction::execute() gates the auto-capture on OperationType::Authorize === $latestOperation->type() — it never checks that the authorize was approved. A rejected authorize (declined card — a routine event in the payment window, and Quickpay sends a callback for it) has type authorize but qp_status_code 40000, so Operations::authorizedAmount() returns 0, the amount comparison fails, and the action throws LogicException. That 500s the notify endpoint, and Quickpay retries a callback that can never succeed. The same happens for a pending authorize (qp_status_code null).

The guard should be Operations::isApprovedOfType($latestOperation, OperationType::Authorize) — rejected/pending authorizes should be a silent return; the amount mismatch (a genuine anomaly) is arguably the only case worth throwing for.

There is no test for the rejected-authorize callback; shouldThrowWhenAuthorizedAmountDoesNotMatch only covers the approved-but-wrong-amount case.

2. StatusAction degrades when the trailing operation is rejected or pending

The Processed branch decides from Operations::latest() alone. Capture 1000, then a rejected refund attempt → the latest operation is an unapproved refund → markUnknown(), although 1000 is demonstrably still held (balance > 0, already in hand). Similarly an async refund still pending flips a captured payment to unknown until the callback lands. markUnknown is at least honest, but downstream consumers (Sylius state machines) treat unknown as a dead end.

Deciding from the last approved operation — or falling back to "balance > 0 → captured" — would keep the status truthful through failed/pending trailing operations. The New-state branch has the same shape: any latest operation that is not an approved authorize is markFailed, even if an approved authorize exists earlier in the list.

3. Missing quickpayPaymentId guards are inconsistent

StatusAction, SyncAction and ConfirmPaymentAction guard the missing-id case explicitly. AuthorizeAction, CaptureAction, RefundAction and CancelAction do not — (int) $model['quickpayPaymentId'] on a missing key is (int) null = 0, so you get a NotFoundException from GET /payments/0 after a network round trip instead of the clear "payment has not been created" LogicException the package already knows how to throw. AuthorizeAction even runs validateNotEmpty([...]) on four other keys but not this one.

4. The two auto-capture mechanisms overlap

When auto_capture is on, AuthorizeAction sets autoCapture: true on the payment link (Quickpay's own window-side auto-capture) and ConfirmPaymentAction issues its own capture when the authorize callback arrives. If Quickpay's link-level capture works, the callback that still shows only [authorize] will trigger a second, redundant capture that Quickpay rejects (→ 500 → retry until the operations list shows the capture). It self-heals but is noisy, and the interplay is undocumented. Worth verifying live which of the two is intended; if the gateway-side capture is the deliberate one (e.g. because link auto-capture is acquirer-dependent), the link flag should probably be false.

5. Coercion sharp edges in the factory closure

• (bool) (int) $config['auto_capture'] turns the string 'true' into false ((int) 'true' is 0).
• (bool) $config['synchronized'] turns 'false' into true.
• agreement_id => 'abc' becomes agreement id 0 rather than an error.

YAML-sourced configs that stringify booleans get silently inverted behavior. Given how carefully payment_methods rejects wrong shapes, these deserve the same strictness.

6. Amounts::forOperation() silently truncates decimals

'249.99' passes is_numeric() and becomes 249. Since everything is minor units, a fractional value is always a caller bug; rejecting non-integers would catch a "sent kroner instead of øre" mistake instead of underquoting it.

7. Currency drift after creation is unreconciled

ConvertPaymentAction refreshes amount/currency in the details on every conversion, but if the currency changes after the Quickpay payment exists, the real payment keeps its original currency and nothing notices. The SDK has updatePayment(); even just throwing on a mismatch would be safer than the silent divergence. Edge case, admittedly.

Missing compared to other Payum integrations

1. Capture is not the interactive entry point — and the README's Usage example is misleading

In most Payum gateways (paypal-express, stripe, mollie bridges…), executing Capture against a fresh payment drives the whole flow, because Payum's stock capture controller — and Sylius's default checkout — executes Capture. Here Capture is strictly the money-movement call: on a fresh payment it produces a Quickpay ValidationException. That is 1.x-compatible (1.x did the same), and Authorize-as-entry is defensible for an auth/capture PSP — but nothing in the README says so, and its Usage section literally demonstrates $quickpay->execute(new Capture($model)) on a bare model.

Either document "the checkout entry point is Authorize (Sylius: use_authorize: true)" prominently, or consider having CaptureAction delegate to Authorize when there's no approved authorization yet (the common Payum pattern). At minimum this is a docs bug.

2. The header-aware GetHttpRequest action ships only as an example

payum/core's plain-PHP bridge never populates GetHttpRequest::$headers, so every non-Symfony consumer has all callbacks rejected as unsigned (400) unless they find and copy examples/e2e/HeaderAwareGetHttpRequestAction.php. That's a silent, total failure of the notify path for a whole class of consumers. The class is small and dependency-free — ship it in src/ (a Bridge/ namespace) and either register it via the core-gateway config defaults or document it in the README's installation section, not just in the upgrade guide.

3. The two-URL callback reality isn't in the README

The fact that capture/refund/cancel confirmations go to the account-wide URL — which is empty by default, so those callbacks silently vanish — lives only in docs/UPGRADE-2.0.md. That's operational knowledge every new consumer needs, not just upgraders.

4. No Sylius wiring section

Sylius shops are the stated main consumer, but the README has no "gateway config in Sylius admin + which option names to use + use_authorize" section. That would prevent the most common integration mistakes and surface the deprecated-alias story to the people it exists for.

5. Deliberate scope gaps worth stating once in the README

No card/API authorize (hosted window only — good for SAQ-A), no subscriptions/recurring (Quickpay supports them; the SDK doesn't model them yet), no payouts. Fine to omit — but saying so saves consumers a search.

Test gaps

The suite is genuinely good: offline, deterministic, asserts both Payum marks and the wire-level requests, and covers the subtle stuff (alias precedence, override consumption on failure, partial-refund status). Gaps, in order of value:

1. No rejected/pending-authorize callback test — the exact case behind bug Fix bug on callback when autocapture is 0 #1. Fixtures with pending: true / qp_status_code: null are entirely absent from the suite, even though async operations are the documented default.
2. NotifyAction::extractChecksum()'s hard-won paths are untested: lowercased header names (Symfony's HeaderBag lowercases them — that's the real production shape!), list-valued headers, non-array headers. Only the exact-case string form is exercised. Prime surviving-mutant candidates.
3. AuthorizeAction has one happy-path test. Untested: the no-token path (pre-set callback_url), the validateNotEmpty failure, the null-link-URL LogicException, and branding_id propagation into the link body (agreement_id is asserted; branding_id never is — precisely the "optional option silently stops being read" failure mode the factory tests guard agreement against).
4. No factory-wired integration test. shouldAllowCreateGateway asserts non-emptiness via reflection; nothing builds a real gateway through QuickpayGatewayFactory (with quickpay.client pointing at the mock) and runs Convert → Authorize → Notify through it. That's the test that would catch a mis-registered action or an aware-interface regression.
5. Smaller: StatusAction on an unmodeled state string (→ unknown), New with an empty operations list, and Infection's MSI gates at 65/70 are modest — the gaps above are likely where the surviving mutants live.
6. Housekeeping: PHPUnit 10 reports 21 deprecations (docblock @test/@dataProvider metadata) — relevant for the move to PHPUnit 11/12.

Suggested priorities

1. Fix the ConfirmPaymentAction approved-authorize guard (robustness Fix bug on callback when autocapture is 0 #1) with the rejected-callback test.
2. Add the missing quickpayPaymentId guards (robustness Payum\Core\Security\GenericTokenFactoryInterface is deprecated #3).
3. Promote HeaderAwareGetHttpRequestAction into the package (gaps added types, static analysis, code style analysis #2).
4. Fix the README's Capture example and add a "checkout entry point is Authorize" section (gaps Fix bug on callback when autocapture is 0 #1).
5. Think longest about the StatusAction last-approved-operation change (robustness added types, static analysis, code style analysis #2) — it changes observable status behavior.

[loevgaard]

All findings that can be fixed without live-account verification now have PRs:

PR	Fixes	Status
#58	R1 — auto-capture guard for rejected/pending authorize (+ T1 tests)	✅ merged
#65	R2 — status decided from the last approved operation (supersedes #59, which GitHub auto-closed when #58's branch was deleted)	open
#60	R3 — quickpayPaymentId guards via the new Details helper	open
#61	R5 — strict boolean/id option normalization in the factory	open
#62	R6 — Amounts rejects non-integer amounts	open
#63	R7 — currency-drift guard in ConvertPaymentAction	open
#64	G2 — HeaderAwareGetHttpRequestAction shipped in the package	open
#66	G1/G3/G4/G5 — README: flow & entry point, callbacks, Sylius, scope (docs; stacked on #64)	open
#67	T2/T3/T4 — notify header shapes, AuthorizeAction paths, factory-wired integration test (contains #60; merges cleanly in either order)	open

Suggested merge order: #65, #60, #61, #62, #63 (independent, any order) → #64 → #66 → #67.

Two items deliberately have no PR:

• R4 (auto-capture overlap) — AuthorizeAction sets autoCapture: true on the payment link and ConfirmPaymentAction captures on the authorize callback. Which of the two should win needs a live check (does the window-level auto-capture actually fire for your acquirer, and does the callback then still show only the authorize?). Only auto-capture an approved authorize in ConfirmPaymentAction #58 narrows the window for the race but does not remove it. Once verified, the fix is one line either way: drop the link flag, or drop the gateway-side capture.
• T6 (PHPUnit attribute migration) — mechanical change across every test file; doing it now would conflict with all nine open PRs. Worth a follow-up PR once the above have landed.

[loevgaard]

Status check (2026-08-10, later)

Merged: #58 (R1), #60 (R3), #61 (R5).

Found and fixed while checking CI:

• 🔴 2.x itself was red since Guard every action against a missing quickpayPaymentId #60 merged: the Dependency Analysis job flags ext-ctype (ctype_digit() in Details, from Guard every action against a missing quickpayPaymentId #60) and ext-filter (filter_var() in the factory, from Normalize the boolean and id options strictly #61) as undeclared shadow deps. Both are default-on but can be compiled out, so they belong in composer.json next to ext-hash/ext-json. → Declare ext-ctype and ext-filter #68, a two-line fix. Every open PR inherits this red check from 2.x, so Declare ext-ctype and ext-filter #68 should go in first.
• Ship HeaderAwareGetHttpRequestAction in the package #64 codecov was a genuine finding, not noise: HeaderAwareGetHttpRequestAction was 62% covered because Guzzle's getallheaders() polyfill is loaded under PHPUnit, so the $_SERVER fallback — the path every polyfill-less SAPI relies on — was dead code in CI. Pushed a follow-up commit making the header source injectable and exposing the reconstruction as headersFromServer(), so it's tested deterministically. 62% → 91% (the rest is the getallheaders-absent branch, unreachable while the polyfill is on the include path). Cascades to the stacked Document the flow, the callback reality, Sylius wiring and scope #66.
• Cover the notify header shapes, AuthorizeAction paths and factory wiring #67 codecov/patch was stale: its diff still carried Guard every action against a missing quickpayPaymentId #60's src/ changes from before Guard every action against a missing quickpayPaymentId #60 merged. Rebased onto 2.x — the diff is now the three test files only.

Open, in suggested merge order: #68 → #65, #62, #63 (independent) → #64 → #66 (stacked on #64) → #67.

Still deliberately without a PR: R4 (auto-capture overlap — needs a live-account check first) and T6 (PHPUnit attribute migration — after the above land, to avoid conflicting with all of them).

[loevgaard]

#68 merged — 2.x is green again. Rebased all six open PRs (#65, #62, #63, #64, #66, #67) onto current 2.x (#66 onto the updated #64 branch) so they pick up the fix and CI re-runs; every one passed composer all and the dependency analyser locally before push.

One more real finding surfaced on #64 while doing that: the analyser resolves getallheaders() to guzzle's dev-only ralouphie/getallheaders polyfill and reports it as a shadow dependency. That's a false positive — it's a PHP-internal function on Apache/FPM, and HeaderAwareGetHttpRequestAction guards it with function_exists() + a $_SERVER fallback precisely so it works with or without the polyfill; depending on the polyfill would be the wrong fix. Added an ignore in composer-dependency-analyser.php (with the reasoning) alongside the existing php-http/message-factory one. Pushed to #64, cascades to #66.

Merge order unchanged: #65, #62, #63 (any order) → #64 → #66 → #67.

[loevgaard]

All green ✅

Every open PR now passes all 21 checks (tests PHP 8.1–8.5 × lowest/highest, style, PHPStan, dependency analysis, mutation, codecov) and is mergeable; 2.x is green at 4aeed1e.

The last holdout was #64's codecov, which turned out to be two genuine test gaps in HeaderAwareGetHttpRequestAction rather than threshold noise: the unsupported-request guard in execute() was only reached via supports(), and the getallheaders-exists-but-empty fall-through was never driven. Both are covered now (the latter deterministically, by clearing every HTTP_* entry so the polyfill has nothing to read), and defaultHeaderSource() collapses its nested ifs into the single decision they are — which also removes the one branch that was structurally unreachable while a polyfill is autoloaded. Class at 100% lines/methods; cascaded to #66.

Merged (4): #58, #60, #61, #68
Ready to merge (6), suggested order: #65, #62, #63 (independent, any order) → #64 → #66 (stacked on #64) → #67

Deliberately still without a PR: R4 (auto-capture overlap — needs a live-account check of which mechanism actually fires before choosing which to drop) and T6 (PHPUnit attribute migration — after the above land, to avoid conflicting with all of them).