Page MenuHomePhabricator

Phan should warn if wfDeprecated() is called with non-string parameters
Closed, DuplicatePublic

Description

https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/535901/

- wfDeprecated( __METHOD__ . " with a string for the title", 1.34 );
+ wfDeprecated( __METHOD__ . " with a string for the title", '1.34' );

This was accidentally passing a float instead of a string. This is something I'd expect Phan to (be able to) catch.

Event Timeline

Hopefully that is PhanTypeMismatchArgument, which is worked on: https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/517740/

It is, but it won't catch the error even if enabled. The reason is that our base phan config allows implicit scalar casts. Hence, it won't complain about strings vs numbers (for instance).
"MediaWiki is pretty lax and uses many scalar types interchangably." -> if we want stricter validation, we need to have stricter code.

I propose to disable implicit casts in the base config, and re-enable it only when needed on a per-repo basis. And ideally, we should do the same for null_casts_as_any_type, but that's another story.

I tried to disable scalar_implicit_casts and then I ran phan on mw core, current master [1]. Result: 1218 issues detected, see P9092 for a list. That's a huge list, so we cannot take it lightly. Although as I said, we can choose to disable it or not depending on the repo. For instance, disabling the option for AbuseFilter yields "just" 36 errors, which is a reasonable amount to fix in a relatively short time.

Interchangeability between scalar types, like number/string is handy, but it can lead to subtle bugs. It also forces us to keep weak comparisons (== and !=) to avoid breaking stuff, and it would prevent us from using PHP7's strict types. Hence, IMHO it's really worth it to consider strengthening the analysis and enable the check.

[1] - I don't know if it's due to that option, but phan took roughly 3GB of memory to run, and my 16GB machine wasn't happy.