Fix PHP issues in MediaWiki core found by Phan (static code analysis)
Open, LowPublic

Description

MediaWiki continuous integration now runs Phan.
Phan is a static code analyser for PHP.
Phan highlighted a lot of issues.
Until those issues are fixed in the MediaWiki core code repository, these issues have been temporarily suppressed (so they will not be shown) in the suppress_issue_types array in Phan's config.php file.

Technical requirements:

You are expected to

  • have some basic understanding of PHP
  • install Phan locally
  • manually remove some issue types from the suppress_issue_types array
  • run Phan locally
  • check Phan's output
  • fix 10-20 of the errors in the output
  • submit your patch in Wikimedia Gerrit. Make sure the commit message has the line "Bug: T153252"

If you run into any problems with Phan, please ask on Phan's Discussion page on the wiki (as there will be more people seeing it than on the GCI site.)

Reedy created this task.Dec 14 2016, 9:08 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 14 2016, 9:08 PM
Reedy updated the task description. (Show Details)Dec 14 2016, 9:13 PM
Aklapper updated the task description. (Show Details)Dec 14 2016, 9:19 PM
Reedy updated the task description. (Show Details)Dec 14 2016, 9:33 PM
Legoktm updated the task description. (Show Details)Dec 14 2016, 9:51 PM

To turn this into a GCI task I guess I'd have to edit config.php and remove the comments on certain types before running Phan? If so, the task description should say so. (With the default config.php on a Fedora 25 machine I only get PhanUndeclared* output for MediaWiki core.)

Aklapper renamed this task from Fixup Phan issues to Fix PHP issues in MediaWiki core found by Phan (static code analysis).Dec 21 2016, 4:13 AM
Aklapper updated the task description. (Show Details)
Aklapper updated the task description. (Show Details)

For the start (and to see if our instructions and the scope / conditions are clear enough), I've created four GCI tasks about this:

Happy to iterate and create more.

Aklapper updated the task description. (Show Details)Dec 21 2016, 10:22 AM
Aklapper triaged this task as Low priority.

Change 328679 had a related patch set uploaded (by Filip):
Fixed PHP issues found by phan

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

Florian added a subscriber: Florian.

As a result of a discussion with @Legoktm (one mentor of the tasks created by @Aklapper), I now unpublished all remaining GCI tasks, as we come to the conclusion, that they don't fit into the GCI. Some errors can be fixed with workarounds, however, we mostly don't want to merge such workarounds just to make Phan happy. Therefore, some of the found problems are errors in phan and should be fixed upstream. However, this can be a difficult and time-intensive task, and therefore not suitable for GCI, unfortunately.

Change 328679 abandoned by Filip:
Fixed PHP issues found by phan

Reason:
Abadoned due to: https://phabricator.wikimedia.org/T153252#2898821

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

I'll add that phan has been pretty good at reviewing patches I've sent upstream to fix analysis problems. Their release schedule is a bit random, so when we can actually use those upstream changes in CI is debatable, but overall I've enjoyed working with them. I have to agree that the level of effort needed to make upstream Phan changes is much too large for a GCI project though.

To be clear (probably what I wrote before wasn't clear enough or could give a false feeling): What I wrote wasn't meant to be "against" Phan, or something. I meant, that upstream changes, at least from my point of view, aren't well suited for GCI, no matter if Phan or any other projects we use in MediaWiki (and/or extensions) :) So, if what I wrote before was understood to be "against" Phan: Sorry, it wasn't meant to be! :]

Mentioned in SAL (#wikimedia-releng) [2017-01-26T13:49:38Z] <hashar> Gerrit creating mediawiki/libs/phpstorm-stubs to fork https://github.com/JetBrains/phpstorm-stubs for T153252

Change 334345 had a related patch set uploaded (by Hashar):
wmf: add composer.json to fork JetBrains/phpstorm-stubs

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

Change 334347 had a related patch set uploaded (by Hashar):
composer validate mediawiki/libs/phpstorm-stubs

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

Change 334347 merged by jenkins-bot:
composer validate mediawiki/libs/phpstorm-stubs

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

Change 334345 merged by jenkins-bot:
wmf: add composer.json to fork JetBrains/phpstorm-stubs

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

Change 335410 had a related patch set uploaded (by Hashar):
Drop phpstorm-stubs

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

It would be nice to have a non voting job running phan which would not have the list of suppressed issues.
This way CI would list all issues for every patch.
I guess if this is done in a separate job however it might put too much strain on CI :/

Change 370942 had a related patch set uploaded (by WMDE-Fisch; owner: WMDE-Fisch):
[mediawiki/core@master] Fix minor issues found with phan

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

Change 370942 merged by jenkins-bot:
[mediawiki/core@master] Fix minor issues found with phan

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