Page MenuHomePhabricator

Statically analyse MinervaNeue codebases with Phan
Closed, ResolvedPublic

Description

Static analysis can help us to catch errors and (potentially) bugs before they're merged into the codebase.

Static analysis with Phan was enabled for MediaWiki extensions on our CI infrastructure as part of T153039: CI: Support running phan for extensions. We should enable/configure the tool like other extensions and reap the rewards!

We'll start by looking at a single extension/skin to get familiar with the tooling and get experience with fixing Phan bugs. Long term if we find it useful we'll apply Phan to all our codebases.

Acceptance criteria

  • Make the extension-unittest-generic job run for the MinervaNeue skin.
  • Copy the Phan config to the MinervaNeue skin.
    • We'll start with that given it's small and not an extension (so we can see if there are any issues with skins using it).
  • Review and group the errors by their difficulty to fix 'em.
  • Fix the easy to fix errors (and get those merged minus the config change)
  • Create tasks for errors that require further discussion (e.g. references to LqtDispatch).

Sign off steps

  • Open tasks for another reading web extension e.g. MobileFrontend or Popups. Copy this line to sign off steps for the new task - we'll do one at a time until reading web projects are using phan.

Questions

  • Do we need to enable any config changes e.g. check experimental?

@phuedx: Yes. Per T133664#3526092, we'll need to enable the extension-unittest-generic job for the MinervaNeue skin at least.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I'll submit a new task about running Phan against MobileFrontend.

phuedx renamed this task from Review and fix phan errors to [EPIC] Fix Phan errors.Nov 6 2016, 6:23 AM
phuedx reopened this task as Open.
phuedx added a project: Epic.
phuedx updated the task description. (Show Details)

Change 320077 had a related patch set uploaded (by Phuedx):
POC: Add Phan config file

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

phuedx updated the task description. (Show Details)
phuedx renamed this task from [EPIC] Fix Phan errors to [EPIC] Statically analyse codebase with Etsy's Phan.Nov 7 2016, 11:55 AM
phuedx updated the task description. (Show Details)

Change 320077 abandoned by Phuedx:
POC: Add Phan config file

Reason:
This change was only submitted for posterity.

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

Looks like etsy phan is running on RelatedArticles when I do check experimental.
Do we want to add a phan file there or do we want to remove this from the experimental build? It confused me while working on https://gerrit.wikimedia.org/r/#/c/347116/

It looks like we're running Phan against any extension that uses the extension-unittest-generic template. The mwext-php70-phan-jessie job was added to the template in rCICF150f06c41479: Add mwext phan job to ext-unittest-generic as part of T153039: CI: Support running phan for extensions. It's unlikely that you'll remove it from the experimental build.

It looks like this task could be resolved if we were to copy https://gerrit.wikimedia.org/r/#/c/330257/ for the MobileFrontend extension (and RelatedArticles and Popups too).

Not sure if this should be changed from an Epic to an good first task ?

@Addshore: Probably a bit of both, right? Getting Phan running against the codebase is now pretty darn easy – it already is! – but fixing the errors… ;)

Wait… I take that back. This task just covers getting it running. I'll update the description!

Do you know about T110617? WMF is currently using Veracode. Whatever the outcome, please mention any ongoing static analysis on the wiki, at a minimum on https://www.mediawiki.org/wiki/Core_Infrastructure_Initiative_Best_Practices_badge#Static_code_analysis so that I can update our best practices information.

phuedx renamed this task from [EPIC] Statically analyse codebase with Etsy's Phan to Statically analyse Readers Web codebases with Phan.Aug 15 2017, 8:55 AM
phuedx edited projects, added good first task; removed Epic.
phuedx updated the task description. (Show Details)
phuedx edited projects, added Web-Team-Backlog; removed Web-Team-Backlog (Tracking).
phuedx moved this task from Incoming to Needs Prioritization on the Web-Team-Backlog board.

@phuedx: As good first task tasks are self-contained, non-controversial issues with a clear approach and should be well-described with pointers to help the new contributor, I'm removing the good first task tag. I have no idea which code base(s) this is about, and "review and group the errors by their difficulty to fix 'em" requires knowledge I don't have (how to know which error is how difficult to fix"), and no explanation how to "run" Phan either.
Please re-add the tag once the task description has been polished and provides sufficient information for a new contributor. Thanks!

Do you know about T110617? WMF is currently using Veracode. Whatever the outcome, please mention any ongoing static analysis on the wiki, at a minimum on https://www.mediawiki.org/wiki/Core_Infrastructure_Initiative_Best_Practices_badge#Static_code_analysis so that I can update our best practices information.

@Nemo_bis: Thanks for the ping. I'll be sure to read up on the Veracode project!

AFAICT WMF is currently using both Phan and Veracode for two different types of analysis:

  • Phan's being used to check for program incorrectness: correct operations on types, methods are accessible, valid return types of functions etc. – for a full list see Phan's README.md.
  • Veracode is being used to detect security vulnerabilities.
phuedx updated the task description. (Show Details)

@Jdlrobson: Regarding the part 1/part 2 split: Which of the Readers Web extensions don't run the extension-unittest-generic job on a per-commit basis.

Change 372061 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Add phan config to MinervaNeue

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

Jdlrobson renamed this task from Statically analyse Readers Web codebases with Phan to Statically analyse MinervaNeue codebases with Phan.Aug 15 2017, 3:22 PM
Jdlrobson updated the task description. (Show Details)

Change 372062 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Popups@master] Add phan config to Popups

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

Jdlrobson raised the priority of this task from Low to Medium.Aug 15 2017, 3:43 PM

@Jdlrobson: Regarding the part 1/part 2 split: Which of the Readers Web extensions don't run the extension-unittest-generic job on a per-commit basis.

Per https://gerrit.wikimedia.org/r/#/c/372061/ it seems skins do not... and I'm not seeing it here: https://gerrit.wikimedia.org/r/#/c/372062/ ?
Am I missing something?

phuedx updated the task description. (Show Details)

@phuedx it doesn't seem to run on Popups either so my guess is we need config for that too (https://gerrit.wikimedia.org/r/#/c/372062/) but since this task is about MinervaNeue this would be a good opportunity to explore it.

Change 372062 abandoned by Jdlrobson:
Add phan config to Popups

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

Change 372061 abandoned by Jdlrobson:
Add phan config to MinervaNeue

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

Jdlrobson lowered the priority of this task from Medium to Low.Oct 11 2017, 10:05 PM
Bawolff subscribed.

It appears that phan-taint-check is voting on MinervaNeue, so removing that project from this bug.

Change 522953 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/skins/MinervaNeue@master] [WIP] Add phan

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

Change 522958 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[integration/config@master] Add phan dependencies for MinervaNeue

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

Change 523210 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[integration/config@master] layout: [skins/MinervaNeue] Add phan in non-voting mode

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

Change 523211 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[integration/config@master] layout: [skins/MinervaNeue] Make phan voting

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

Change 522958 merged by jenkins-bot:
[integration/config@master] Add phan dependencies for MinervaNeue

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

Mentioned in SAL (#wikimedia-releng) [2019-07-15T15:59:30Z] <James_F> Zuul: Adding Echo and MobileFrontend dependencies to skins/MinervaNeue T133664

Change 523210 merged by jenkins-bot:
[integration/config@master] layout: [skins/MinervaNeue] Add phan in non-voting mode

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

Mentioned in SAL (#wikimedia-releng) [2019-07-15T16:49:10Z] <James_F> Zuul: [MinervaNeue] Add phan as non-voting T133664

Change 523211 merged by jenkins-bot:
[integration/config@master] layout: [skins/MinervaNeue] Make phan voting

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

Mentioned in SAL (#wikimedia-releng) [2019-07-15T18:05:04Z] <James_F> Zuul: [MinervaNeue] Make phan voting T133664

Jdforrester-WMF subscribed.

Thank you for all your work on this, Daimona.

Change 522953 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Add phan

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