Page MenuHomePhabricator

Phan throws false positive when checking SpecialElectronPDF file
Closed, ResolvedPublic

Description

ElectronPDF extension uses mwext-php70-phan-jessie to verify code standards, now it started to fail with an error:

18:47:00 + PHAN=/home/jenkins/workspace/mwext-php70-phan-jessie/src/vendor/bin/phan
18:47:00 + ./tests/phan/bin/phan ./extensions/ElectronPdfService -m checkstyle
18:47:54 <?xml version="1.0" encoding="ISO-8859-15"?>
18:47:54 <checkstyle version="6.5">
18:47:54   <file name="src/specials/SpecialElectronPdf.php">
18:47:54     <error line="19" severity="error" message="Call to method __construct from undeclared class \MessageLocalizer" source="PhanUndeclaredClassMethod"/>
18:47:54     <error line="203" severity="error" message="Call to method setHeaders from undeclared class \MessageLocalizer" source="PhanUndeclaredClassMethod"/>
18:47:54   </file>
18:47:54 </checkstyle>
18:47:54 Build step 'Execute shell' marked build as failure

After quick check I found that SpecialElectronPdf extends SpecialPage which implements new MessageLocalizer interface. Now, when analyzing SpecialElectronPDF file Phan because of some reason throws an error. I assume that issue was introduced in rMW3e2824679694

The most likely reason is Global classes aren't found when in a namespace (SpecialElectronPDF is namespaced), but it looks like it should be fixed from Dec 10th 2015 and version 0.7.0 was released in Dec 2016. There might be an issue in Phan anyway as we're using old Phan (version 0.7.0, latest stable is 0.9.2 which requires PHP7.1, the 0.8.4 works on PHP7.0).

This issue is blocking T167272. Link to patch: https://gerrit.wikimedia.org/r/#/c/359011

Event Timeline

@pmiazga hey, the patch seems to be merged now. Is the issue resolved or Did I miss anything? Also, did it break CI tests (phpunit for example) or is it just phan?

Patch rMW3e2824679694 causes Phan tests to fail, which causes CI tests to fail, which ends up with -1 from jenkinsbot.

Jdlrobson moved this task from Upcoming to Design on the Readers-Web-Backlog board.
Jdlrobson added a subscriber: Jdlrobson.

It's likely to hit any extension that is subclassing SpecialPage. @Ladsgroup only reason we merged it was because we disabled phan for that line.

@EBernhardson and @Addshore might be able to help since they have introduced Phan ?

TLDR is:

src/specials/SpecialElectronPdf.php
Call to method __construct from undeclared class \MessageLocalizer

Probably due to global classes not being found in a namespaced class https://github.com/etsy/phan/issues/28

Change 359684 had a related patch set uploaded (by EBernhardson; owner: EBernhardson):
[mediawiki/extensions/ElectronPdfService@master] Add core languages directory to phan analysis

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

The phan config was just not loading this class, because some of the language classes are in a top level language directory instead of includes. Attached patch adds the appropriate configuration.

Change 359684 merged by jenkins-bot:
[mediawiki/extensions/ElectronPdfService@master] Add core languages directory to phan analysis

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

Thanks @EBernhardson. To me the tests directory would be the last place to look for. Lesson learned - phan has configs in every project.

Change 359939 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/ElectronPdfService@master] Remove phan supress checks annotation

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

Change 359939 merged by jenkins-bot:
[mediawiki/extensions/ElectronPdfService@master] Remove phan supress checks annotation

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