Page MenuHomePhabricator

Upgrade phan to 5.3.2 or later
Open, Needs TriagePublic

Event Timeline

Reedy renamed this task from Upgrade phan to 5.3.1 to Upgrade phan to 5.3.1 or later.Jan 5 2022, 4:50 PM
Umherirrender added subscribers: Daimona, Umherirrender.

Updates of phan in mediawiki/mediawiki-phan-config needs also update of phan in taint-plugin

For 5.2.1 there is a bump already - https://gerrit.wikimedia.org/r/c/mediawiki/tools/phan/SecurityCheckPlugin/+/721411, but no release

Reedy renamed this task from Upgrade phan to 5.3.1 or later to Upgrade phan to 5.3.2 or later.Feb 3 2022, 5:36 PM
Reedy updated the task description. (Show Details)
Reedy updated the task description. (Show Details)

Change 760569 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/tools/phan/SecurityCheckPlugin@master] Bump phan to 5.3.2

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

Change 760569 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/tools/phan/SecurityCheckPlugin@master] Bump phan to 5.3.2

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

I tried... CI is now failing with a mysterious message when running PHPUnit. It doesn't seem to fail for other patches in the same repo, but there shouldn't be any difference. Also, the disabling of xdebug inevitably clutters the output and it's unclear whether it might be related. The relevant task is T269489, which requires T280170, which iscurrently blocked on T243847 for the 7.2 package.

I've confirmed locally that T269489 is indeed the issue -- if I run the taint-check PHPUnit suite with xdebug enabled, it tries to restart itself but for some reason it fails. Likely, the xdebug-checking logic changed in phan > 5.2.1.

For now I'm setting the env var which bypasses the xdebug check, but that's not something I'd like to keep in the long term, since phan is terribly slow if xdebug is enabled (I think the docs used to say 5x slow, I personally saw it run like 20x slower with taint-check enabled). xdebug should really really really not be enabled in the base CI images.

Change 760569 merged by jenkins-bot:

[mediawiki/tools/phan/SecurityCheckPlugin@master] Bump phan to 5.3.2

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

Reedy assigned this task to Daimona.

I'd rather close this after we tag a new version of taint-check, use it in mw-phan-config and then tag a new version of that. Upgrading phan is a bit annoying, unfortunately...

Just as note:
When using the new version (5.3.2) one of the most (new) showing up issue is PhanParamTooFewUnpack/PhanParamTooFewInternalUnpack which warns when ... is used to unpack arguments to a function and that array may is empty.

Possible some false positives as well.

Example:

includes\api\ApiBase.php:1227 PhanParamTooFewUnpack Call with 0 or more arg(s) to \wfMessage($key, ...$params) which requires 1 arg(s) defined at includes\GlobalFunctions.php:1180. This may throw an ArgumentCountError if there are too few args at runtime.

for

$msg = wfMessage( ...$msg );

Phan sees a possible empty $msg array

Example:

includes\api\ApiBase.php:1227 PhanParamTooFewUnpack Call with 0 or more arg(s) to \wfMessage($key, ...$params) which requires 1 arg(s) defined at includes\GlobalFunctions.php:1180. This may throw an ArgumentCountError if there are too few args at runtime.

for

$msg = wfMessage( ...$msg );

Phan sees a possible empty $msg array

This one (and possibly others) could also be fixed by annotating the parameter as non-empty-array. I'm not sure how "official" that is, and whether it's fine to have it in the standard @param annotation or whether it should be in @phan-param.