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 auasort()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 saysmachine_name, and callers pass it straight toDomainStorage::load().DomainLanguageConfigFactoryOverrideInterface::getDomainOverride()documented@return \Drupal\Core\Config\Config, but it returns aDomainLanguageConfigOverride, which extendsStorableConfigBaseand is not aConfig.- Two test base classes declared
@var \Drupal\domain\DomainInterfaceon a property holdingloadMultiple()output. That single typo accounted for 19 of the 52 errors. - Three lazily initialized properties in
DomainSourcePathProcessorand one inDomainConfigFactoryare reset to NULL but were documented as non-nullable.
Narrowing that was assumed rather than checked
DomainStorage::loadDefaultDomain()andDomainAliasStorage::loadByPattern()returnedcurrent()on the result ofloadByProperties()behind acount()guard. They now narrow withinstanceof, which also removes thecurrent()-returns-FALSE edge case.DomainAliasFormneedsDomainAccessControlHandler::checkAccess(), which the generic access handler interface does not expose, andDomainSourceActionBaseneedsConfigEntityTypeInterface::getConfigPrefix(). Both now check the handler and the entity type they were silently assuming.
API use
DomainAccessEntityHooksappended to a field item list with$field[] = [...]. Replaced with the typedappendItem().DeleteForm::buildForm()returned aRedirectResponsefrom a method contracted to return the form array.FormBuilder::retrieveForm()supports this by converting the response into anEnforcedResponseException, 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()andDrupalKernelInterface::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|falseassigned from anAccountInterface. - 2 errors,
StorableConfigBase::$schemaWrapper. Documented@var Elementwith no null, althoughgetSchemaWrapper()guards with!isset()and so is built to start null.unset()instead of= NULLtrades the error forunset.possiblyHookedProperty, and redeclaring the property nullable in the subclass trades it forproperty.phpDocType. - 1 error,
getOverride().LanguageConfigFactoryOverrideInterfacedocuments@return Config; core's own implementation returns aLanguageConfigOverride, which is not one. Core would report this against itself. - 2 errors,
DomainStorage. Not a wrong docblock anywhere. phpstan-drupal mapsDomainStorageto theDomainclass, so$storage->loadMultiple()resolves correctly from outside the class, but inside it the caller type isstaticandEntityDataRepository::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).