>>> [!note] Migrated issue <!-- Drupal.org comment --> <!-- Migrated from issue #3619892. --> Reported by: [mably](https://www.drupal.org/user/3375160) Related to !57 >>> <p>kessai has run phpstan at level 3 since #3619757, the same level as orchestra, yoyaku, audit_trail, pdv and domain. Level 4 adds dead-code detection: branches phpstan can prove unreachable, comparisons the declared types have already settled, and assertions that can only pass. Level 5 adds argument-type checking on every call.</p> <p>Measured on 1.x at b017760 with the shipped phpstan.neon and entity_mapping.neon, level 4 and level 5 produce identical output: 16 errors across 9 files locally, every one of them a level 4 finding, and a seventeenth that only the CI runner reports, because its phpstan-drupal types one more service from the map than the local one does. Level 5 has nothing to add, because the module already types every parameter natively. So the target is 5 rather than 4: it costs nothing today and starts guarding argument types as the module grows.</p> <h3>Seven findings are dead code, or a comparison the types have already answered</h3> <ul> <li><code>PaymentManager::deleteStoredCard()</code> re-tests every element with <code>instanceof PaymentInterface</code>, on a parameter its interface documents as <code>PaymentInterface[]</code>.</li> <li><code>WorldlineWebhookController</code> wraps each header value in <code>is_array()</code>, on a Symfony <code>HeaderBag</code> whose values are always lists.</li> <li><code>scripts/generate-metrics.php</code> coalesces offset 0 of a list that <code>explode()</code> guarantees is non-empty.</li> <li><code>WorldlineReturnTest</code> asserts four times that a controller declared to return a <code>RedirectResponse</code> returned a <code>RedirectResponse</code>.</li> </ul> <p>These are what level 4 exists for: each one is a guard a reader has to stop and think about, that the type system already made unreachable.</p> <h3>Four are guards that must stay, because the annotations phpstan trusts are optimistic</h3> <p><code>WebhooksEvent::getPayment()</code> is annotated <code>@return PaymentResponse</code>, but the property is simply unset when the notification carries no payment, so it returns NULL. The identity check against NULL in <code>WorldlineWebhookController</code> is what stands between a notification with no payment and a fatal error, and the two nullsafe calls on <code>AmountOfMoney</code> in <code>WorldlineGatewayTest</code> are the same shape. Deleting these three to satisfy phpstan would be a regression, so the fix is to describe the SDK's real types to phpstan in a stub file rather than trust its annotations or reach for an ignore.</p> <p>The fourth is the same problem from inside the tooling. <code>MovementBase::getPaymentId()</code> reads the magic <code>target_id</code> property, which phpstan-drupal types as a plain string, with a @todo in its own source saying it should be a union; core's <code>FieldItemList::__get()</code> returns NULL for an empty field and carries a comment saying so. The NULL branch is therefore live and the analyser is wrong. No stub can reach a hardcoded property reflection, so the answer here is to ask the field itself: an <code>isEmpty()</code> check before reading the property is the idiomatic form, and it sidesteps the bad inference instead of arguing with it.</p> <h3>Three are phpstan's service map disagreeing with what a test registered</h3> <p><code>PaymentOperationLockTest</code> registers <code>logger.channel.kessai</code> as a <code>BufferingLogger</code> in its own <code>register()</code>, but phpstan-drupal types that service id from the real container, so the drain guard reads as always false and the assertion as impossible. #3619757 hit the mirror image of this with the audit_trail recorder and settled the recipe: construct the double in the test, hand it to the container with set() after parent::setUp(), and keep it in a typed property. The same recipe applies here.</p> <p>The third is the reason to read the CI job rather than trust a local run: CI's phpstan-drupal types the <code>lock</code> service as the <code>DatabaseLockBackend</code> the production map names, so <code>testLockBackendIsReal()</code>, which exists to prove KernelTestBase's null backend was swapped out, reads as an assertion that can only pass. It now asserts the property the class actually rests on instead of a class name: a null backend answers TRUE to everyone, so the container backend taking a free lock and a second owner then being refused is what a vacuous lock would fail, and it is not something the service map can settle.</p> <h3>Three are implementations that never use the nullable return their interface allows</h3> <p><code>SimulatorGateway::initiate()</code> and the test <code>SessionGateway::initiate()</code> both declare <code>?CheckoutSession</code> and always build one, so narrowing the implementation is both legal and truer. The third is an anonymous SDK double in <code>WorldlinePaymentHandlerTest</code> whose <code>getPropertyName()</code> declares <code>?string</code> and only ever returns NULL, which wants a different answer: either the double names a real property in one test, or its declaration says what it does.</p> <h3>Scope</h3> <p>One line in <code>phpstan.neon</code>, and seventeen findings fixed rather than suppressed, across production code, tests and one script. No behavior change is expected, and the SDK stub file, if that is the route taken, would be the only new file.</p> <p>AI-Generated: Yes (Claude Code was used to measure the levels, work out what each finding is, draft this issue summary, and write the changes on the merge request. I reviewed and ran the work myself before posting it.)</p>