>>> [!note] Migrated issue <!-- Drupal.org comment --> <!-- Migrated from issue #3620933. --> Reported by: [mably](https://www.drupal.org/user/3375160) Related to !60 >>> <p>A full read of the module from the first file rather than a diff against the last tag: all 133 tracked files, plus phpcs at CI's own ruleset and again with DrupalPractice, phpstan at level 5, cspell, a metrics regeneration, a clone detector, and a sweep that extracts every <code>Class::method()</code> from the docs and from every PHP comment block and greps the source for it. Twenty-four findings, in one issue and one merge request; the twenty-fifth is the CI fatal filed separately as #3620931, which this branch is built on.</p> <h3>A token charge records money it can never give back</h3> <p><code>chargeToken()</code> settles through <code>PaymentManager::settleOnGateway()</code>, which sets the state to captured, saves, and dispatches CAPTURED. It does not record a claim and it does not add to the captured total. <code>doCapture()</code>, the other path to the same state, does both.</p> <p>So a merchant-initiated charge lands as a captured payment whose captured total is 0.00. <code>getBalance()</code> is captured less refunded, so the balance is zero, and <code>refund()</code> refuses the payment outright with "a refund returns a positive amount". The claims view shows no row for money that moved, and kessai_audit_trail writes captured 0.00 into the chain for a charge that took the money. Reproduced against unmodified 1.x with a kernel probe:</p> <pre><pre>$charge = $manager-&gt;chargeToken('15.00', 'EUR', 'stub', 'fee', 'tok_abc', $subject);<br>state=captured&nbsp; amount=15.00&nbsp; captured=0.00&nbsp; balance=0.00&nbsp; claims=0<br><br>$manager-&gt;refund($charge);<br>InvalidArgumentException: Cannot refund 0.00 of payment 1: a refund returns a positive amount.</pre></pre><p>architecture.md states the broken invariant twice, in its own words: "A claim is recorded whichever path took the money, including a direct sale that never held anything and asked no gateway, so the total always has rows behind it", and captured is "the sum of the payment's completed claims". Fixed by recording the claim and the total on the token path too, so one rule covers every way money is taken. Nothing in the suite asserted a claim row or a total after <code>chargeToken()</code>, which is why four audit rounds passed over it; a test now pins both.</p> <p>Recording money that was going unrecorded costs queries, so each path was measured in its own process, one variable at a time: <code>chargeToken()</code> 11 to 20, <code>authorizeToken()</code> 11 to 11, the direct sale 20 down to 16, and a rejected checkout undone 23 down to 19. The two reductions are a second thing the audit turned up: a settled claim was written pending and then saved again completed on three paths where the provider had already taken the money, so no attempt could ever fail and the row was written twice to record a fact settled before the call was made. <code>recordClaim()</code> now takes the state, and those rows are written once. So one path costs more, because it was recording nothing, and two existing paths cost less.</p> <h3>The bcmath install gate is a hook core never calls</h3> <p><code>KessaiRequirements::install()</code> carries <code>#[Hook('install_requirements')]</code> under a docblock saying it "Refuses the install outright when bcmath is missing". Core dispatches no hook of that name, in 11.3 or in 12.0. Install-phase requirements reach core through <code>install_check_class_requirements()</code>, which scans <code>src/Install/Requirements/</code> for a class implementing <code>InstallRequirementsInterface</code>, or through procedural <code>hook_requirements()</code> in the .install file; <code>HookCollectorPass::checkForProceduralOnlyHooks()</code> rejects <code>#[Hook('requirements')]</code> outright.</p> <p>So a site without bcmath installs cleanly and then fatals on the first amount it reads, naming an undefined function rather than a missing extension, which is exactly what the hook exists to prevent. Moved to <code>src/Install/Requirements/</code>, the shape core actually discovers. The runtime half stays where it is: <code>hook_runtime_requirements()</code> is real and is dispatched. The hook class keeps the runtime half and is renamed <code>KessaiRuntimeRequirements</code>, because that is now all it is, and it returns the installer's own check rather than a second copy of it.</p> <h3>Duplication the byte-identical detector cannot see</h3> <p>The clone detector reports 151 exact five-line groups and every one of them is test boilerplate. These are semantic: one rule written out again under a different name.</p> <ul> <li>The guard that stops an open redirect on a payer-supplied URL has three implementations in three modules: <code>PaymentHandoffController::resolveDestination()</code>, <code>WorldlineReturnController::resolveDestination()</code>, the same name with a different signature, and <code>SimulatorCheckoutForm::resolveInternalUrl()</code>. Hardening it means remembering all three. Given one owner on the engine side that all three call.</li> <li><code>PaymentHandoffController::TOKEN_ARG</code> already declares the query argument kessai_pay_token, and it is written out as a literal six more times. Worldline mints it in <code>buildReturnUrl()</code> and reads it in the return controller; the simulator does the same in its own pair. Each pair has to agree or the payer meets a 403 coming back from paying, and nothing links them. Same shape as the REFERENCE_PREFIX finding #3620119 fixed.</li> <li>The two settings.php override keys have two owners: constants in <code>WorldlineSettingsForm</code> and literals in <code>WorldlineClientFactory</code>. The constants' own docblocks say "Kept in step with WorldlineClientFactory, which reads the same key", which is the finding written down rather than fixed. The factory owns them now: it is the reader, and it is the @api service a deployment swaps.</li> <li><code>WorldlineGateway</code> builds a CallContext carrying an idempotency key three ways: <code>buildRefundCallContext()</code> and <code>buildReversalCallContext()</code>, near-identical four-line methods differing only in which <code>PaymentOperationKey</code> static they call, and the same two lines inline in <code>capture()</code>.</li> <li>Two formatters render the same Worldline error: <code>WorldlineGateway::formatResponseErrors()</code> and <code>WorldlinePaymentHandler::formatWorldlineError()</code>, in different shapes, so an operator reading the log sees two formats for one provider failure.</li> <li>Each shipped default is written three times: 1800 and 20 appear in config/install, as <code>PaymentManager</code> constants, and again as fallbacks in <code>KessaiSettingsForm::buildForm()</code>. The form's copy is the one that would silently disagree, since it is what an operator reads.</li> <li><code>KessaiSchemasTrait</code> was created to own the four-schema install list and says so, "the list and the reason live here rather than in each setUp()". Four test classes use it; nine write the same four <code>installEntitySchema()</code> calls by hand.</li> </ul> <h3>Dead code a deletion left behind</h3> <p><code>WorldlineGateway::chargeStoredCard()</code> takes a <code>$persist_reference</code> parameter documented as "FALSE for a throwaway probe (account verification) that must not clobber a real reference". That probe was <code>verify()</code>, removed from the capability interfaces in #3615855. The method is private and neither caller passes FALSE, so the parameter and its branch are unreachable. docs/worldline.md still describes <code>verify()</code> as well, which is the third time a removal sweep has missed docs/, after the ones #3619100 and #3620119 caught.</p> <h3>Documentation</h3> <p>Nine of these are a rename or a deletion a sweep did not follow, which is what the check that finds them is for.</p> <ul> <li>docs/events.md ships two code samples that would fatal: <code>$payment_manager-&gt;refunds($payment)</code> and <code>-&gt;claims($payment)</code>, renamed to <code>getRefunds()</code> and <code>getClaims()</code> in #3619212. They are the only executable lines on the page a reader is meant to copy.</li> <li>Seven docblocks reason about methods under their old names. <code>PaymentManager</code> explains its own locking in terms of <code>lockedCreate()</code>, three times, and <code>lockedTransition()</code>, twice; they are <code>createUnderLock()</code> and <code>transitionUnderLock()</code>. <code>PaymentManagerInterface::getLatestMovement()</code> weighs itself against <code>self::claims()</code>, <code>self::refunds()</code> and <code>self::reversals()</code>. <code>WorldlineGateway</code> cites <code>sessionMinutes()</code>, now <code>getSessionMinutes()</code>.</li> <li><code>PaymentDeadline::seconds()</code> is named in three places and does not exist; the factory is <code>fromSeconds()</code>. In gateways.md the comment sits directly above a line of code that calls <code>fromSeconds()</code> correctly.</li> <li><code>RecordInterface</code> opens with "It deliberately declares no methods", then argues at length that hoisting the shared signatures would cost documentation. It declares four of them. The contradiction was there in the commit that created the file.</li> <li>architecture.md's "What is public" list omits <code>MovementInterface</code> and <code>RecordInterface</code>, both marked @api. #3620119 found the same omission in the README and fixed it there.</li> <li><code>hook_kessai_gateway_info_alter()</code> is declared by <code>PaymentGatewayManager::alterInfo()</code> and documented nowhere: no kessai.api.php, and the name appears in no doc page and no README, so a gateway author has no way to learn it exists. Given a kessai.api.php.</li> <li>Two sentences in worldline.md lose their verb mid-clause: "the payment handler deletes it, best effort, unneeded card is left on file", and "checks two things, and if fails it releases what was taken". Both are on the page describing what happens to a payer's money.</li> <li><code>chargeToken()</code> says it is "idempotent under concurrency the same way as <code>authorize()</code>". <code>authorize()</code> records an outcome on an existing payment and shares none of that machinery; the sibling is <code>authorizeToken()</code>, which says so correctly in the other direction.</li> <li><code>create()</code>'s $deadline parameter says "NULL uses the configured default". NULL asks the gateway, which is the point of <code>PaymentDeadline</code> and may answer never(). It also never mentions that 0 opts the payment out, which index.md, the settings form and the config schema all do.</li> <li><code>CheckoutSession</code>'s class docblock says an id, a URL and a lifetime "are required". $expires is nullable with a NULL default, and its own parameter doc explains what NULL means, so the next sentence about the return secret being optional reads as a contrast that is not there.</li> <li>"The payment payment handler", in three constructor docblocks, copied along with the parameter it describes.</li> <li><code>PaymentStorageSchema</code> describes its composite index as covering "(subject + subject + kind)". The columns are subject_type, subject_id and kind.</li> <li><code>SimulatorCheckoutForm::title()</code> is declared <code>: mixed</code> against its own @return of TranslatableMarkup, which is what it returns.</li> </ul> <h3>Audited and clean, recorded so the next round does not repeat it</h3> <p>Security: all nine entity queries carry an explicit accessCheck(FALSE); the only raw SQL is one parameterized SELECT ... FOR UPDATE; there is no #markup and no Markup::create anywhere. Every bearer token is an HMAC over the hash salt under its own purpose prefix, compared with hash_equals. The webhook verifies its signature over the raw body before trusting a byte of it, and answers 200 when unconfigured so the provider stops retrying. Both Worldline secrets take a settings.php override and the form never prefills them. All four views are gated on administer kessai and none exposes the card token or the checkout secret.</p> <p>Performance: no N+1; every hot query is served by kessai_payment__subject_kind, kessai_payment__state_expires or core's entity-reference target index; gateway plugins and the SDK client are memoized per request; the card-on-file anchor lookup is bounded at ten candidates and the cascading movement delete is chunked at fifty.</p> <p>Gates: phpcs clean at CI's ruleset and again with DrupalPractice; phpstan level 5 clean apart from the two errors that are the separate CI issue; cspell clean; docs/metrics.md regenerates to a fixed point, so it is not stale for once.</p> <h3>Found reviewing the branch, and fixed on it</h3> <p>Repeated passes over the merge request turned up five more, every one on the documentation surface rather than in the money paths. <code>MovementInterface</code> and its <code>STATE_PENDING</code> constant both promised "a movement row exists before the provider is asked", which three claim paths now contradict, and that is the contract a consumer reads; <code>ClaimInterface::isFinal()</code>, architecture.md and index.md said the same thing more narrowly. <code>kessai.api.php</code> called <code>t()</code> in its example, which potx extracts, so translators would have been offered a phrase this module never displays. And the prose on the new classes narrated the refactor that produced them rather than describing what they do, which is not what a reader six months from now needs.</p> <p>Two of the tests were checked by breaking what they protect rather than by reading them. Deleting <code>#[Hook('runtime_requirements')]</code> left the requirements test passing, because it called the method instead of asking whether Drupal dispatches it: the same defect as the install half, in the fix's own test. It now asks the module handler, and fails when the attribute is removed. The contract test that stops the fix over-reaching onto the authorization path was checked the same way, by making an authorization record a claim; it catches it.</p> <p>The consolidation of the open-redirect guard was measured rather than assumed: the three implementations it replaces were run beside <code>OnwardUrl::resolve()</code> over thirty adversarial values, including protocol-relative, <code>javascript:</code>, <code>data:</code>, backslash hosts and uri schemes. No decision differs. That corpus is now a test, since the rule went from three owners to one and the route tests each cover a single value.</p> <p>AI-Generated: Yes (Claude Code was used to help draft this issue summary and to write the code and tests on the merge request. I reviewed and ran the work myself before posting it.)</p>