Issue #3618610: Ship a phpstan config with an entity mapping, raise the gate to level 3, and fix what the levels above 0 could see

Implements [#3618610]. The project ships a phpstan.neon and the gate is level 3.

The gate was level 0. The templates fetch their own asset when a project ships no config, and that asset is level: 0. So the project ships phpstan.neon now, carrying the level, the file extensions the asset scanned (so shipping a config does not silently narrow the job), the one new static ignore Drupal needs, and the entity mapping. The level lives there rather than in _PHPSTAN_LEVEL, which this removes: one answer, and a contributor's own run gives it too. No baseline and no ignoreErrors beyond that one, because a baseline is a level nobody meets, written down.

The entity mapping is the lever, and it is the Domain project's trick. Without it every $storage->load() is an EntityInterface, so every call to a booking's own method reads as undefined: 219 of the 264 findings at level 2 were exactly that and none were defects. Mapping the entity types took level 2 from 264 findings to 152 and level 3 from 356 to 217 with no call site touched. The mapping also names the orchestra and kessai types the bridge modules read: those belong to dependencies rather than to this project, but its tests load and drive them, and both bridges declare the dependency already, so the mapping says no more than their code does.

One of the mapped types is not the class its attribute declares. yoyaku_order swaps yoyaku_transaction to its own Order in hook_entity_type_alter. Mapping the attribute's class made phpstan report every order method as undefined, which is how the swap was found. Worth stating in the file, and it is: a mapping is a claim about the class a site HAS, not the one the attribute names.

The defects, which are the argument for raising a gate at all. From levels 1 and 2:

  • A test passed a message as a third argument to responseHeaderEquals(), which takes two, so the explanation was discarded in silence.
  • The event dispatcher was typed as PSR-14, whose dispatch() takes one argument, in two services that both pass an event AND its name. Now Symfony's contract, which is what Drupal's service implements.
  • Four queue loops read ->data off claimItem() without narrowing, and claimItem() is declared object|bool.
  • A test called setArchived() on a class that does not declare it.
  • Three array shapes in doc comments never parsed, so phpstan was ignoring those types outright. They are now a type both tools accept, with the prose that already described the keys doing that work.
  • A @var carried the escaping of the string it was copied from; two @return tags named a class that does not exist or contradicted the native type; and a mock was typed as the interface it stands in for, which has no method().

And from level 3, which adds return.type, offsetAccess.notFound and method.childReturnType, the categories that catch a docblock promising what the code cannot produce:

  • Thirteen entity interfaces did not declare the changed time their entity tracks. Every one of them uses EntityChangedTrait, and nine of the module's twenty-two already said so, so these were the omission: code holding a PlaceInterface could not ask a place for the getChangedTime() it has.
  • OrderOverviewForm memoized its extra columns under a docblock naming two of the three keys it builds, and buildRow() reads the third.
  • SelectionBooker described its result in prose. Saying it as a shape is what lets OfferClaim merge the errors into its own by-reference array without widening it past what it promised its caller.
  • TicketBuilder handed a readonly property to alter()'s by-reference context argument. PHP passes a temporary there, so anything a hook wrote back to it would have been dropped without a word.
  • FieldRuleRegistry::options() said its keys were strings. A boolean rule is keyed 0 and 1, which PHP holds as ints, so no caller could reach either.
  • PlacePositions::renumberedRow() said its second key held places. It holds one boolean per place: whether a gap falls before it.
  • CancelOrderConfirmForm cast four TranslatableMarkup values to string to satisfy a string return type, throwing away markup the page title wants.

And the narrowing the rest asked for, each said once rather than at every call site: the table mapping's base table (one line in one trait, reported 27 times, because getBaseTable() is on DefaultTableMapping and not on the interface the getter returns); the weighted order forms' records, a family of six, which are content entities carrying a weight field and are now typed as such; the entity reference items behind ->entity and ->target_id; the access results that carry cacheability; the aggregate queries, which answer one row per group and not the id list QueryInterface::execute() is declared to return; and a query helper that hands back whatever query it was given, which a @template can say and a signature cannot. In tests, the entity a reference field's ->entity really answers is now checked before it is used, so a row that does not carry one fails saying so rather than on a null.

Checked: phpstan at level 3 reports [OK] No errors. phpcs over the module root under Drupal,DrupalPractice: no error and no warning in any file this touches. The translation check is clean in both directions, and this adds no string. The fifteen kernel classes whose tests this changes: 15/15.

Edited by Frank Mably

Merge request reports

Loading
Loading