Issue #3616760: Raise phpstan analysis from level 2 to 3

Running phpstan at level 3 against 4.x reported 52 errors in 24 files. 34 of them were real defects in this module and are fixed in the first commit; the remaining 18 are typing bugs upstream and are baselined in the second.

Wrong contracts (would mislead a caller)

  • DomainStorageInterface::loadDefaultId() documented @return int|bool, "FALSE if none is set", but the implementation returns NULL and the id of a config entity is a machine name. Every caller in the module already treats it as an id or NULL.
  • DomainStorageInterface::sort() documented @return bool. It is a uasort() comparator and returns an int; returning a bool there would break the sort.
  • DomainAliasInterface::getDomainId() documented @return int, "the numeric domain_id". The stored value is the parent domain's machine name: the property is @var string, the config schema says machine_name, and callers pass it straight to DomainStorage::load().
  • DomainLanguageConfigFactoryOverrideInterface::getDomainOverride() documented @return \Drupal\Core\Config\Config, but it returns a DomainLanguageConfigOverride, which extends StorableConfigBase and is not a Config.
  • Two test base classes declared @var \Drupal\domain\DomainInterface on a property holding loadMultiple() output. That single typo accounted for 19 of the 52 errors.
  • Three lazily initialized properties in DomainSourcePathProcessor and one in DomainConfigFactory are reset to NULL but were documented as non-nullable.

Narrowing that was assumed rather than checked

  • DomainStorage::loadDefaultDomain() and DomainAliasStorage::loadByPattern() returned current() on the result of loadByProperties() behind a count() guard. They now narrow with instanceof, which also removes the current()-returns-FALSE edge case.
  • DomainAliasForm needs DomainAccessControlHandler::checkAccess(), which the generic access handler interface does not expose, and DomainSourceActionBase needs ConfigEntityTypeInterface::getConfigPrefix(). Both now check the handler and the entity type they were silently assuming.

API use

  • DomainAccessEntityHooks appended to a field item list with $field[] = [...]. Replaced with the typed appendItem().
  • DeleteForm::buildForm() returned a RedirectResponse from a method contracted to return the form array. FormBuilder::retrieveForm() supports this by converting the response into an EnforcedResponseException, so the exception is now thrown directly. Same request outcome, same exception object, same subscriber.

What is baselined, and why it cannot be fixed here

The 12 remaining entries are not defects in this module. Each is a line in our code measured against a declared type that lives elsewhere:

  • 10 errors, KernelTestBase::$container. Core declares the property @var \Drupal\Core\DependencyInjection\ContainerBuilder, while \Drupal::getContainer() and DrupalKernelInterface::rebuildContainer() both document a container interface. Any test that reassigns the container after a rebuild puts those two core docblocks in contact.
  • 3 errors, BrowserTestBase::$loggedInUser. Same shape: @var \Drupal\user\UserInterface|false assigned from an AccountInterface.
  • 2 errors, StorableConfigBase::$schemaWrapper. Documented @var Element with no null, although getSchemaWrapper() guards with !isset() and so is built to start null. unset() instead of = NULL trades the error for unset.possiblyHookedProperty, and redeclaring the property nullable in the subclass trades it for property.phpDocType.
  • 1 error, getOverride(). LanguageConfigFactoryOverrideInterface documents @return Config; core's own implementation returns a LanguageConfigOverride, which is not one. Core would report this against itself.
  • 2 errors, DomainStorage. Not a wrong docblock anywhere. phpstan-drupal maps DomainStorage to the Domain class, so $storage->loadMultiple() resolves correctly from outside the class, but inside it the caller type is static and EntityDataRepository::resolveFromStorage() requires an exact supertype match.

Core runs phpstan at level 1, so assign.propertyType never fires against it: its 7192-entry baseline contains no entry with that identifier. Raising a contrib module above core's own level means the module is the first thing to audit core's docblocks, and it absorbs the findings at its own line numbers. The first four groups are worth filing upstream.

Note that with reportUnmatchedIgnoredErrors: false removed by #3616747, a baseline entry that matches nothing is now itself an error, so these entries cannot silently rot.

That immediately caught a real cross-lane problem. A generated baseline pins the exact message text, and on Drupal 12 core declares the property as ContainerBuilder|null rather than ContainerBuilder. The generated entries therefore matched on 11.4 and went unmatched on D12, so phpstan (next major) reported 5 ignore.unmatched on top of the 10 real errors. The five $container entries are consequently written as message regexes that accept either wording, with their paths and counts kept. Both phpstan and phpstan (next major) are green.

phpunit (next major) is red with 5 failures. They are pre-existing: the same five test cases fail on !422, diffed by test name from a run the same day.

Verified on Drupal 11.4.4 / PHP 8.4: level 3 clean, phpcs --standard=Drupal clean over the whole module, DrupalPractice reports nothing in any changed file, and the kernel and unit tests covering the changed code pass (42 tests).

Edited by Frank Mably

Merge request reports

Loading