Issue #3619890: Raise PHPStan to level 5 and remove the dead code it finds

Raises the project's PHPStan configuration from level 3 to level 5.

Level 5 reported 160 findings. They resolved into four groups.

Dead code removed (130)

  • 43 instanceof guards that can never be false, each after a loadMultiple() or loadByProperties() whose elements already are the entity type asked for. The three guards in InteractionResolver that read $tokens[$id] ?? NULL first are kept: those subjects really can be NULL, and the analysis does not report them, which is what makes the distinction reliable.
  • 25 null-coalesce operators on keys the node, flow and variable shapes declare required. The config schema requires the same keys, so the shape and the stored data agree.
  • 14 test assertions whose value already carries the asserted type.
  • Two always-true assert() calls in TaskActions::getFamily(), a dead catch with its unreachable tail in TenantDeletionTest, and two is_array() calls on values already known to be arrays.

Defects the run surfaced (4)

  • OrchestraCmFeatureAuthoringTest asserted assertNotNull() on waitForText(), which returns FALSE on timeout. The test passed whether or not the save landed.
  • RolesVariableAudienceTest declared ?int on a closure whose (int) cast could never produce NULL, defeating the ?-> it was written around.
  • ApiController forwarded limit and offset as strings where the client's declared shape asks for int.
  • Three kernel tests passed a method return value to reset(), which PHP takes by reference.

Return types narrowed (23)

Six are methods a class declares for itself and that promised more than they returned. The other 17 implement a wider interface. Narrowing an implementation is covariance, so a caller holding the interface still sees the wider type, and each one records in its docblock why this implementation is narrower.

AccountRecipient is deliberately left wider. It carries @api, and docs/extending.md promises that @api signatures do not change breakingly within 1.x, so a narrowing that costs nothing to write now would be a breaking change to undo. RecipientInterface keeps getAccount() and label() nullable and the concrete class follows it.

What is still ignored (7), and why

Every entry is scoped to the files it applies to, so each rule keeps working everywhere else.

  • FormStateInterface::getUserInput() is documented as returning an array and returns NULL on a FormState nothing has populated yet. Removing the coalesce errors three kernel classes with "must be of type array, null given".
  • Sql::addWhere() documents $group as a string while its own body maps '', 0 and NULL onto the default group, and core passes the integer 0 to it in nine places.
  • Three assertions the analysis folds to a constant because it cannot model what changes between them: a service resolved from the container, config read back after a form submit, and an event collector a dispatch appends to. Each is a real regression guard at run time.
  • The two AccountRecipient signatures above.

reportUnmatchedIgnoredErrors is left at its default, so an entry that stops matching is reported rather than sitting stale.

Verification

  • phpstan clean at level 5 with the project's own phpstan.neon.
  • phpcs clean over the whole module against Drupal and DrupalPractice.
  • Kernel suite: 182 of 183 classes pass. PaymentWorkflowSubscriberTest::testInstanceDeletionDeletesPinnedPayments fails identically on unmodified 1.x, so it is not from this branch.
Edited by Frank Mably

Merge request reports

Loading
Loading