Page MenuHomePhabricator

Phan broken due to ResourceLoader namespace move
Closed, ResolvedPublic

Description

example console

src/MediaWiki/Config/LexemeLanguageCodePropertyIdConfig.php:19 PhanParamSignatureRealMismatchParamType Declaration of function getScript(\ResourceLoaderContext $context) should be compatible with function getScript(\MediaWiki\ResourceLoader\ResourceLoaderContext $context) (parameter #1 of real signature type '\ResourceLoaderContext' cannot replace original parameter of real signature type '\MediaWiki\ResourceLoader\ResourceLoaderContext') defined in ../../includes/ResourceLoader/Module.php:234
src/Presentation/View/TemplateModule.php:20 PhanParamSignatureRealMismatchParamType Declaration of function getScript(\ResourceLoaderContext $context) should be compatible with function getScript(\MediaWiki\ResourceLoader\ResourceLoaderContext $context) (parameter #1 of real signature type '\ResourceLoaderContext' cannot replace original parameter of real signature type '\MediaWiki\ResourceLoader\ResourceLoaderContext') defined in ../../includes/ResourceLoader/FileModule.php:377
src/Presentation/View/TemplateModule.php:55 PhanParamSignatureRealMismatchParamType Declaration of function getDefinitionSummary(\ResourceLoaderContext $context) should be compatible with function getDefinitionSummary(\MediaWiki\ResourceLoader\ResourceLoaderContext $context) (parameter #1 of real signature type '\ResourceLoaderContext' cannot replace original parameter of real signature type '\MediaWiki\ResourceLoader\ResourceLoaderContext') defined in ../../includes/ResourceLoader/FileModule.php:623

Affects:

  • GrowthExperiments
  • WikibaseLexeme
  • probably others

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Namespaces introduced in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/789789 (forgot to link that earlier).

Is it worth trying to figure out why Phan doesn’t work with the class aliases that the patch adds (correctly, at a glance at least), or should we just update all affected extensions to use the new name right away (and hope the namespaceization isn’t reverted for other reasons)?

The patch includes a “ResourceLoaderContext72Hack” hack class, which is apparently supposed to work around T166010#5962098. Perhaps that hack, whatever it is, doesn’t work in Phan?

I was under the impression that T271736: Migrate WMF production from PHP 7.2 to PHP 7.4 is making steady progress – maybe it would be best to delay the namespaceization until we’re firmly on PHP 7.4 (as the linked comment proposed)?

Change 792137 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/core@master] Revert "ResourceLoader namespace"

https://gerrit.wikimedia.org/r/792137

Change 792137 merged by jenkins-bot:

[mediawiki/core@master] Revert "ResourceLoader namespace"

https://gerrit.wikimedia.org/r/792137

Lucas_Werkmeister_WMDE lowered the priority of this task from Unbreak Now! to Needs Triage.May 16 2022, 4:00 PM
Lucas_Werkmeister_WMDE added a subscriber: Reedy.

CI should be working again (I’ll verify in WikibaseLexeme in a moment); leaving it up to @tstarling and @Reedy whether this task should stay open (for repeating the namespace move with fewer problems) or can be closed already.

CI should be working again (I’ll verify in WikibaseLexeme in a moment)

Phan is green again

This comment was removed by tstarling.

When phan is checking for a signature match, it first checks whether the names match:

if (!$overridden_parameter_union_type->isEqualTo($parameter_union_type) &&

That always fails in the case of an alias. Then if the target PHP version is 7.4+, it checks for what the PHP manual calls contravariance:

$is_exception_to_rule = (Config::get_closest_minimum_target_php_version_id() >= 70400 && $overridden_parameter_union_type->isStrictSubtypeOf($code_base, $parameter_union_type)) ||

That check passes for an alias. So Phan does not properly implement alias checks for class overrides, but it accidentally works for --target-php-version=7.4 due to the contravariance feature.

Basically it looks like at least a day of my time wasted debugging issues specific to the dinosaur version of PHP we are running in production. Potential fixes:

  • Patch all affected extensions to use the new class name. The core patch would be reapplied and the extension patches would be immediately merged. Would break anything using phan without release branches.
  • Add @phan-suppress to all affected methods in extensions.
  • Patch phan. Wastes upstream's time with ancient PHP version issues, and probably incurs a rebase cost when reapplying the core patch, due to phan deployment delay.
  • Temporarily have the core class use the old name in its signatures, and suppress the phan issues which will occur due to core having enable_class_alias_support=false
  • Forget it for now. Rebasing in future will be complex, so this option probably adds half a day to the project.
  • Upgrade to PHP 7.4 in production. Could be done in a few days and we need to do it anyway.

FWIW the current blocker of php7.4 migration work seems to be T295578

For configuration of changeprop, there is https://wikitech.wikimedia.org/wiki/Changeprop#To_deployment-prep but the steps described there don't seem to generate a valid configuration currently.

  • Patch phan. Wastes upstream's time with ancient PHP version issues, and probably incurs a rebase cost when reapplying the core patch, due to phan deployment delay.

I think creating an issue upstream would make sense; I'm not sure what the PHP compat policy for phan is, but I'm not aware of any plans about dropping support for older PHP versions. I do agree that this would take a while though, because of the slow rollout of the new version.

I think my preferred solution is to combine options 1, 2: update extensions to use the new class name, but if any affected extensions need to support old versions of core, use @phan-suppress.

I've uploaded updates for 35 extensions, to be merged shortly after the core patch is merged. I linked them to T308718 so that there wouldn't be too much bot noise here.

Change 793713 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/extensions/UniversalLanguageSelector@master] Suppress PhanParamSignatureRealMismatchParamType on classes affected by new RL namespace

https://gerrit.wikimedia.org/r/793713

Change 793713 merged by jenkins-bot:

[mediawiki/extensions/UniversalLanguageSelector@master] Suppress PhanParamSignatureRealMismatchParamType on classes affected by new RL namespace

https://gerrit.wikimedia.org/r/793713

Lucas_Werkmeister_WMDE claimed this task.

leaving it up to @tstarling and @Reedy whether this task should stay open (for repeating the namespace move with fewer problems) or can be closed already.

Let’s close this task, since T308718 was opened in the meantime.