2.x final review: stuck auto-capture, silent sync declines, per-operation callback url, docs and test gaps #71

Progress

Follow-up to #57 — a final review of 2.x (at 1a6cfc2 / v2.0.0-beta.1) for anything that review did not cover. Each finding that can be fixed here gets its own PR (stacked where they touch the same code); this table is updated as PRs open and land.

Verified against the code, the SDK 1.0.0 sources, payum/core 1.7.7, Quickpay's published swagger (api.quickpay.net/docs/v10/merchant/api/payments) and the Sylius plugin. R1 and R2 were confirmed with throwaway tests against the mock HTTP seam. Baseline: composer all green, infection MSI 89% (42 escaped mutants).

Robustness

R1 (high) — A rejected Quickpay-side auto-capture leaves the payment stuck: Capture is a permanent no-op

CaptureAction.php:61-69 returns unconditionally when the link carries auto_capture and an approved authorize exists. That covers "Quickpay captured" and "Quickpay's capture is queued" — but also "Quickpay's capture was rejected" (test card 1000 0000 0000 0032, listed in the harness). Then the state stays new, GetStatus → authorized, and every Capture from then on — with or without capture_amount — fetches and returns without issuing anything. Nothing in the gateway can ever capture that payment; only the Quickpay manager can. Confirmed offline: authorize 20000 + capture 40000 + link.auto_capture=true → Capture issues no request.

Sharpest consequence: setono/sylius-quickpay-plugin's PaymentProcessor runs GetStatus, sees not-captured, executes Capture, gets no exception → the Sylius payment transitions to completed with no money captured.

Fix: no-op only while a capture is approved/pending or not yet recorded (the race window right after authorization); when the latest capture attempt is rejected, Quickpay tried once and will not retry. The return trip (a token-carrying Capture) should still not move money — the customer has to land somewhere and the shop sees authorized — but a programmatic Capture (the merchant settling; the plugin's complete transition) captures through the API. Needs the live check with card …0032 that #57 never ran.

R2 (high) — In synchronized mode a rejected capture/refund/cancel returns as success

?synchronized makes Quickpay answer 200 with the completed operation — completed, not approved: a decline is a 2xx with qp_status_code ≠ 20000. The SDK maps only the HTTP status to exceptions (Client.php:353-376), and CaptureAction.php:71-77 / RefundAction.php:37-43 / CancelAction.php:36 ignore the returned Payment entirely (right for the async snapshot, wrong for the sync outcome). Confirmed offline: synchronized capture, response operation 40000 → no exception, capture_amount consumed. README:42/155 and UPGRADE:146 present synchronized as "block until settled" — a caller (again the plugin: complete transition → Capture → no exception → completed) reasonably reads that as "and tell me if it failed".

Fix: after the call, look at the returned payment's newest operation of that type; if it is not pending and not approved → throw a typed OperationRejectedException (Quickpay status code/message on it) before Amounts::consume(). Cheap to do unconditionally: async responses carry the operation as pending, so nothing changes there.

R3 (medium, policy call) — No in-flight guard on the money operations

AuthorizeAction.php:53-57 refuses to act while a capture is pending, but on the plain-link path CaptureAction issues a capture with a capture already pending, RefundAction refunds the (pre-operation!) balance with a refund already pending, and CancelAction cancels with a cancel pending. A retry after a timeout, or a double click, queues a second operation; whether Quickpay rejects the duplicate at queue time or only at processing time is unverified. Suggest hasPending(<same type>) → LogicException ("still pending — wait for the callback or use synchronized"). It also blocks a legitimate second instalment issued within seconds of the first, and it costs the explicit-amount refund and the cancel a fetch they do not make today — hence a policy call.

R4 (medium, verify live) — Does state become pending during an async operation?

StatusAction.php:64-66 maps pending → markPending(). If Quickpay flips a captured payment to pending while a refund is in flight (the SDK docs only say the 202 snapshot keeps the pre-operation state), then #65's last-approved-operation fix — only reached in processed — does not apply, and a captured payment reports pending mid-refund. The suite assumes both shapes (pending for a queued capture in CaptureActionTest::linkAutoCapturesProvider, processed for a pending refund in StatusActionTest:202). One e2e:operate refund + immediate status settles it. If pending does occur, decide it like processed: last approved operation, then balance.

R5 (low) — quickpayPaymentId => null throws instead of markNew

StatusAction.php:35 / SyncAction.php:44 guard with offsetExists(), which is true for a null value (verified against payum's ArrayObject), so Details::paymentId() throws "execute Convert first" where GetStatus should answer new (and Sylius would then Convert). ConvertPaymentAction uses isset(). Align on "null means absent".

R6 (low) — CreatePaymentLinkAction.php:59 casts amount with a bare (int)

The link amount is what Quickpay authorizes; Amounts was made strict for exactly this (#62), but the link path still truncates '249.99' to 249. Reuse the strict read. Also worth a doc line: capture_amount is ignored on the interactive Capture (the link auto-captures the full link amount).

R7 (low) — payum/core ^1.6 allows versions where GetHttpRequest::$headers is a dynamic property

#[\AllowDynamicProperties] landed in payum/core 1.7.5 (2024-09). Below that, on PHP ≥ 8.2, HeaderAwareGetHttpRequestAction (and payum's own Symfony bridge) emit a deprecation per callback, and PHP 9 makes it fatal. phpstan.neon.dist already assumes ≥ 1.7.5. Bump to ^1.7.5.

Architecture

A1 (high) — The "two callback urls" problem has an API-level fix the gateway is not using

Quickpay's swagger for POST /payments/{id}/capture|refund|cancel lists a request header QuickPay-Callback-Url — "Specify the callback url (overrides merchant default callback-url)" (also on learn.quickpay.net/tech-talk/api/callback). Sending the payment's callback_url (the notify token url already in the details) on every operation would route capture/refund/cancel callbacks to the per-payment Payum notify token — no account-wide url, no order_id-resolving endpoint (README:146-155, UPGRADE:123-148, examples/e2e/listen.php and the plugin's NotifyAction controller all exist to work around this). SDK 1.0 has no way to set a per-request header (ResourceEndpoint::postOperation), so this is: SDK minor (optional callbackUrl on capture/refund/cancel) → gateway passes $model['callback_url'] when present → the docs shrink to a footnote.

Verify live first, e.g. curl -u ":$KEY" -H 'Accept-Version: v10' -H "QuickPay-Callback-Url: <notify token url>" -d amount=1000 https://api.quickpay.net/payments/<id>/capture on an authorized e2e payment and watch the listener; the response operation's own callback_url field also shows where it went.

A2 (medium) — auto_capture deprecation vs. the Sylius plugin, and README's Sylius section

setono/sylius-quickpay-plugin's GatewayConfigurationType forces use_authorize = true (a HiddenType) and exposes auto_capture as its "capture at authorization" checkbox — i.e. the primary consumer's only way to express a sale is the option 2.0 deprecates, and README:270-274 ("Sylius drives its checkout through Capture by default … Nothing to configure") is not what a plugin-based shop gets. Either soften the deprecation until the plugin derives use_authorize from the checkbox (use_authorize = !auto_capture), or coordinate that plugin change with 2.0. Either way the README's Sylius section should point at the plugin (admin form, notify endpoint, reconcile command) rather than describe a bare GatewayFactoryBuilder service as if the admin form existed.

A3 (medium, optional) — Register HeaderAwareGetHttpRequestAction automatically

payum's core config sets payum.action.get_http_request to a plain-PHP GetHttpRequestAction instance before populateConfig() runs, so QuickpayGatewayFactory can detect get_class(...) === Payum\Core\Bridge\PlainPhp\Action\GetHttpRequestAction::class and swap in the header-aware subclass. Symfony's bridge (a different class) and any consumer override are left alone. Removes the "every callback is a 400 unless you read the README" failure mode entirely; the README snippet becomes optional.

A4 (low) — Vestigial public helpers

Operations::latest(), isLatestApproved(), authorizedAmount() (Operations.php:31-38, 110-129) have no callers in src/ since #65/#69. Beta is the time to drop them (or accept them as public API deliberately).

A5 (info) — The plugin overrides ConvertPaymentAction and sets continue_url = after url

So under the plugin the #69 return-trip design (re-run Capture/Authorize) is never exercised; the outcome comes from Sylius's after-pay GetStatus. Fine — but its Convert also lacks the currency-drift guard (#63) and the order-id length guard is its own. Worth knowing when reasoning about "what Sylius does".

Docs

• D1 README:58-74 configuration example builds a gateway with no api_key/private_key → throws at getGateway(); $defaultConfig = []; is unused.
• D2 README:177-187 details table: balance is also written by Authorize/Capture; state also by Notify (ConfirmPaymentAction:51). UPGRADE:96-99 same.
• D3 synchronized (README:42,155; UPGRADE:146) implies the outcome is surfaced — see R2.
• D4 Callbacks: mention that Quickpay retries up to 24 times with backoff (so a 400 on a bad checksum is retried), and A1 once verified.
• D5 Stale: examples/e2e/listen.php:107 "notify token url AuthorizeAction built", CLAUDE.md "the notify token AuthorizeAction mints" (it is CreatePaymentLinkAction), CLAUDE.md's e2e section still presents HeaderAwareGetHttpRequestAction as harness-encoded (it ships in src/Bridge).
• D6 .gitattributes: CLAUDE.md and composer-dependency-analyser.php are exported in the dist.

Tests

• T1 R1 / R2 / R3 scenarios (none exist).
• T2 QuickpayGatewayFactoryTest: only "missing private_key" is pinned (:364-378) — dropping api_key from the required options survives mutation; nothing asserts the apikey alias / api_key reaches the client (send one request through the mock and check Authorization); nothing asserts order_prefix/language/private_key land on Api from a full config (4 escaped CastString mutants, factory :105-108).
• T3 GatewayIntegrationTest:96-99 presets callback_url "since no token factory is wired" — so the one aware interface that matters most (GenericTokenFactoryAwareInterface on CreatePaymentLinkAction) is never exercised through the factory-built gateway. Pass 'payum.extension.token_factory' => new GenericTokenFactoryExtension(new StubTokenFactory()) (what PayumBuilder does) and drop the preset.
• T4 infection.json.dist gates 65/70 vs. an actual 89/89 — raise to ~85 so a regression fails CI.
• T5 Small: Api's default autoCapture=false is unpinned; the two markUnknown() calls are undetectable (GetHumanStatus starts unknown — markNew() before executing kills both).
• T6 Local note: composer infection on PHP 8.4 dies in the initial run (exit 143 under infection's own thecodingmachine/safe deprecation flood); --initial-tests-php-options='-d error_reporting=24575' works.

Cross-repo (for the SDK / plugin trackers)

• SDK: per-operation QuickPay-Callback-Url (A1); model Link::$autoCapture (the swagger has it; the gateway reads raw['link']['auto_capture']); document/handle sync-mode rejected operations (R2) — a latestOperation(type) helper would serve both.
• Plugin: forces use_authorize and depends on the deprecated auto_capture (A2); its NotifyAction controller does not catch Payum replies (Reply\Base extends LogicException), so an invalid checksum surfaces as a 500 rather than the gateway's 400; PaymentProcessor completes on "no exception" — R1/R2 make that unsafe.

Suggested order

R2 → R1 (with the …0032 live check) → A1 live check (decides SDK work + docs) → R3/R7/D1–D6/T2–T4 (small, independent) → A2 decision with the plugin → A3/R5/R6/A4 optional polish.