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
->dataoffclaimItem()without narrowing, andclaimItem()is declaredobject|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
@varcarried the escaping of the string it was copied from; two@returntags 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 nomethod().
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 aPlaceInterfacecould not ask a place for thegetChangedTime()it has. OrderOverviewFormmemoized its extra columns under a docblock naming two of the three keys it builds, andbuildRow()reads the third.SelectionBookerdescribed its result in prose. Saying it as a shape is what letsOfferClaimmerge the errors into its own by-reference array without widening it past what it promised its caller.TicketBuilderhanded a readonly property toalter()'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.CancelOrderConfirmFormcast fourTranslatableMarkupvalues to string to satisfy astringreturn 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.