Statically analyse MinervaNeue codebases with Phan
Open, LowPublic

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.

phuedx created this task.Apr 26 2016, 10:10 AM
Restricted Application added subscribers: TerraCodes, Aklapper. · View Herald TranscriptApr 26 2016, 10:10 AM
phuedx updated the task description. (Show Details)Apr 26 2016, 10:11 AM
jhobs triaged this task as Low priority.Apr 26 2016, 5:11 PM
phuedx closed this task as Declined.Nov 4 2016, 12:48 PM

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 added a project: Epic.
phuedx updated the task description. (Show Details)
phuedx reopened this task as Open.

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)Nov 6 2016, 8:07 AM
phuedx updated the task description. (Show Details)Nov 6 2016, 8:19 AM
phuedx claimed this task.
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

Jdlrobson moved this task from Backlog to Later on the Readers-Web-Backlog (Tracking) board.

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 goodfirstbug ?

@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 goodfirstbug; removed Epic.
phuedx updated the task description. (Show Details)
phuedx moved this task from To Triage to Needs Analysis on the Readers-Web-Backlog board.

@phuedx: As goodfirstbug 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 goodfirstbug 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 removed phuedx as the assignee of this task.Aug 15 2017, 9:07 AM
phuedx updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)Aug 15 2017, 2:45 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.

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 Normal.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?

Jdlrobson updated the task description. (Show Details)Aug 15 2017, 3:44 PM
phuedx updated the task description. (Show Details)Aug 23 2017, 8:41 AM
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 Normal to Low.Oct 11 2017, 10:05 PM
Bawolff added a subscriber: Bawolff.

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