>>> [!note] Migrated issue <!-- Drupal.org comment --> <!-- Migrated from issue #3619740. --> Reported by: [mably](https://www.drupal.org/user/3375160) Related to !54 >>> <p>Another session found a large amount of duplication in yoyaku that several audits had missed, so this is the same question asked of kessai. It has the same answer: a clean phpcs, a clean phpstan and four naming audits had all passed over <strong>26 duplicate groups worth 308 removable lines</strong>.</p> <h3>The detector, and the bug in it that matters more than the findings</h3> <p>A windowed-hash clone detector: extract every method body, strip comments and blank lines, hash it three ways. Once as written, once with variables, strings and numbers blinded, and once with class and method names blinded as well, which is what catches a copy that differs only by type.</p> <p><strong>The first run was wrong and its numbers were useless.</strong> The body being hashed included the method's own declaration line, so three methods differing only by name hashed differently and were invisible. Fixing that took the production groups from 3 to 7 and the removable lines from 77 to 308. It was caught only because a second, independent pass, grouping every entity query by its normalized condition shape, found a triplicate the clone detector had not reported. <strong>Two independent methods disagreeing is what exposes a broken detector; one method agreeing with itself proves nothing.</strong></p> <p>The other thing worth writing down: the detector must run over the tests. Scoping it to production code hides the bigger half.</p> <p><strong>A second blind spot, found the same way.</strong> The first attempt at that bullet did not reuse <code class="language-php">runUnderLock()</code>; it extracted a near-copy, adding a third instance of the primitive rather than removing one. Thirteen of fourteen lines were identical and the detector reported nothing, because the single differing line was <code class="language-php">return $payment;</code> against <code class="language-php">return NULL;</code>. <code class="language-php">NULL</code> is a keyword, so it survives the pass that blinds variables, strings and numbers, and the later pass that blinds class names then turns it into the same token as a class. A one-token difference hid a thirteen-line clone at every level. The detector now blinds <code class="language-php">NULL</code>, <code class="language-php">TRUE</code> and <code class="language-php">FALSE</code> before the class-name pass, and gains a level that blinds the returned expression so two bodies that are the same algorithm with different sentinels collide. Checked against the commit that still contained the clone: the first three levels miss it, the new one catches it. The lesson is not about the tool: <strong>a clone detector finds duplication that exists, and cannot tell you that the helper you are about to write already exists.</strong> Extraction has to start with a grep for the shape.</p> <p><strong>A third blind spot, in the same instrument.</strong> The detector compares method BODIES, and an interface has none, so it could not see that <code class="language-php">getCurrency()</code> was declared in two interfaces with a byte-identical summary, nor the five other signatures those two shared. Interface declarations need their own comparison, on the signature and the docblock summary rather than a body. Added and re-run over the module: after the change above, zero duplicated declarations remain across its ten interfaces. Three blind spots in one audit, all of the same kind, is the point worth keeping: <strong>each time the detector said nothing, it was because the thing it cannot represent was exactly where the duplication was.</strong></p> <h3>Production</h3> <ul> <li><strong>The row lock had no owner.</strong> <code class="language-php">startTransaction()</code>, <code class="language-php">SELECT ... FOR UPDATE</code> on the payment row and the fresh reload were written out in both <code class="language-php">accumulate()</code> and <code class="language-php">transitionUnderLock()</code>. This is the concurrency guarantee, and two copies is how one gets fixed and the other does not. Now <code class="language-php">loadPaymentForUpdate()</code>, and <code class="language-php">forUpdate()</code> appears exactly once, next to the note that it is a no-op on SQLite so the guarantee is only real on MySQL.</li> <li><strong>The four transitions off pending</strong> (<code class="language-php">authorize</code>, <code class="language-php">fail</code>, <code class="language-php">cancelPending</code>, <code class="language-php">expire</code>) each restated the same rule: announce only when the compare-and-set actually won the row. A fifth transition that forgot the guard would announce an event for something that never happened. Now <code class="language-php">transitionFromPendingAndAnnounce()</code>.</li> <li><strong><code class="language-php">getClaims()</code>, <code class="language-php">getRefunds()</code> and <code class="language-php">getReversals()</code></strong> were the same eleven-line query three times, differing only in the storage and the interface filtered on. <code class="language-php">getLatestMovement($payment, $type)</code> already existed as the parameterised form, so the pattern was established and simply not applied to the list case. Now <code class="language-php">getMovements()</code>.</li> <li><strong><code class="language-php">authorizeToken()</code> and <code class="language-php">chargeToken()</code> were the same 32 lines twice</strong>, differing in six values. Split into <code class="language-php">createTokenPayment()</code> for the create-or-reuse-under-lock skeleton and <code class="language-php">settleOnGateway()</code> for the ask-and-record sequence, which is the part that decides whether the engine believes it holds money it does not. The capability interface is checked in the helper, but the gateway call stays a closure typed to that same interface, so it is still resolved statically instead of through a string.</li> <li><strong><code class="language-php">refundUnderLock()</code> and <code class="language-php">reverseUnderLock()</code></strong> differed only in the operation the lock was scoped to, and the class already had <code class="language-php">runUnderLock()</code> doing exactly that sequence: the same unsaved-id bypass, the same acquire, the same refresh, the same body call, the same release in a finally. <code class="language-php">runUnderLock()</code> now takes the value to answer with when the lock is refused, and capture, refund and cancel all go through it, so three copies of the primitive became one.</li> <li><strong><code class="language-php">WorldlineGateway::refund()</code> and <code class="language-php">::cancel()</code></strong> shared an eight-line preamble, extractable now that both movements share <code class="language-php">MovementInterface</code>: <code class="language-php">getReferenceOrDecline()</code> and <code class="language-php">buildAmountOfMoney()</code>.</li> <li><strong>A payment and a movement had no name for what they share.</strong> <code class="language-php">runUnderLock()</code> could only say it returned <code class="language-php">mixed</code>, because the three operations answer with different things. They were not that different: <code class="language-php">PaymentInterface</code> and <code class="language-php">MovementInterface</code> both extended the identical pair, <code class="language-php">ContentEntityInterface</code> and <code class="language-php">EntityChangedInterface</code>, and shared seven method names, six of them with byte-identical signatures. <code class="language-php">RecordInterface</code> now names what the engine writes down and hands back, so the signature reads <code class="language-php">?RecordInterface</code>. <code class="language-php">?ContentEntityInterface</code> would have typed it too, but a node satisfies that, and a kessai record is the actual contract. It carries the four questions both answer the same way: <code class="language-php">getAmount()</code>, <code class="language-php">getCurrency()</code>, <code class="language-php">getReference()</code> and <code class="language-php">setReference()</code>. <code class="language-php">getState()</code>, <code class="language-php">setState()</code> and <code class="language-php">getKind()</code> stay on the two sub-interfaces, because there the shared signature hides different meanings: a payment state is one of seven and a movement state one of three, and a payment must name what it is for while a movement need not, which is why <code class="language-php">getKind()</code> returns <code class="language-php">string</code> on one and <code class="language-php">?string</code> on the other.</li> </ul> <h3>Tests, which were the bigger half</h3> <ul> <li><strong>Thirteen test classes had each written the same reload.</strong> Eleven were byte-identical; one added an assertion and one also set a captured total. Now <code class="language-php">PaymentReloadTrait</code>, with the odd one delegating to it.</li> <li>Three worldline kernel tests had byte-identical <code class="language-php">setUp()</code> bodies, including the comment explaining which schemas a gateway path needs and why. Now <code class="language-php">KessaiSchemasTrait</code>, where that reason lives once.</li> <li><code class="language-php">createHeldPayment()</code> stood twice, now <code class="language-php">PaymentFixturesTrait</code>.</li> <li>Four provider-response builders collapsed to two parameterised ones: two differed only in a status category, two only in a status string.</li> <li>The client-factory stub wiring was extracted where all seven lines matched exactly, which covered four sites in one class. The eight other stub sites genuinely differ and were left alone.</li> </ul> <h3>What was deliberately not changed</h3> <p>Reporting these would make the next audit worthless, so they are recorded rather than churned.</p> <ul> <li><code class="language-php">getFieldDescriptions()</code> in the three movement entities: same shape by design, since each subclass exists to supply its own content.</li> <li>Two-line entity setters such as <code class="language-php">setReference()</code>: that is Drupal's entity idiom, not duplication, and they belong in the detector's exclusion list beside <code class="language-php">create()</code>.</li> <li>Parallel test methods that assert the same behavior for a refund and for a reversal. Each documents its own case, and a shared helper or a data provider would hide what each one is actually checking.</li> <li>The <code class="language-php">$modules</code> arrays: per-class configuration, not logic.</li> <li><code class="language-php">authorizeToken()</code> and <code class="language-php">chargeToken()</code> still read alike at fourteen lines, and <code class="language-php">refund()</code> and <code class="language-php">cancel()</code> at six, because what remains in each is a parameterised call rather than shared logic. Collapsing those further would need a helper taking a dozen arguments, which is worse than what it replaces.</li> </ul> <h3>Result</h3> <p>Byte-identical duplicate method bodies: <strong>zero</strong>. Quasi-duplicate groups down from 26 to 16, removable lines from 308 to 160, and what remains is the deliberate list above. 34 files, 684 insertions, 721 deletions, no behavior change and nothing stored moved.</p> <p>AI-Generated: Yes (Claude Code was used to write the detector, run the audit, draft this summary and make the changes on the merge request. I reviewed and ran the work myself before posting it.)</p>