Phan throws false positive when checking SpecialElectronPDF file
Closed, ResolvedPublic
Actions

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

Details

	Project	Branch	Lines +/-	Subject
	mediawiki/extensions/ElectronPdfService	master	+0 -2	Remove phan supress checks annotation
	mediawiki/extensions/ElectronPdfService	master	+3 -1	Add core languages directory to phan analysis

Customize query in gerrit

Related Objects

Mentioned In: rEEPSab695652f90d: Update patch set 5
rEEPS69f7a6d13fc6: Final NoteDb migration updates
rEEPS0d79b5ae6be7: Remove phan supress checks annotation
rEEPS7cdcd4906a3f: Add core languages directory to phan analysis
rEEPSe9563425876c: Add core languages directory to phan analysis
rEEPS16fd98537180: Add core languages directory to phan analysis
T167272: Incorrect alignment on download as PDF page
Mentioned Here: rMW3e2824679694: Require $key in msg functions
T167272: Incorrect alignment on download as PDF page

Event Timeline

• pmiazga created this task.Jun 15 2017, 6:04 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 15 2017, 6:04 PM

• pmiazga updated the task description. (Show Details)Jun 15 2017, 6:04 PM

@Ladsgroup - any idea what can be wrong?

• pmiazga mentioned this in T167272: Incorrect alignment on download as PDF page .Jun 15 2017, 6:19 PM

@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.

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.

Jdlrobson moved this task from Design to 2014-15 Q4 on the Readers-Web-Backlog board.Jun 16 2017, 7:16 PM

Jdlrobson edited projects, added Readers-Web-Backlog; removed Readers-Web-Backlog (Design).

@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

gerritbot added a project: Patch-For-Review.Jun 17 2017, 7:11 PM

EBernhardson mentioned this in rEEPS16fd98537180: Add core languages directory to phan analysis.Jun 17 2017, 7:13 PM

EBernhardson mentioned this in rEEPSe9563425876c: Add core languages directory to phan analysis.Jun 17 2017, 7:16 PM

EBernhardson mentioned this in rEEPS7cdcd4906a3f: Add core languages directory to phan analysis.Jun 17 2017, 7:21 PM

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

ReleaseTaggerBot added a project: MW-1.30-release-notes (WMF-deploy-2017-06-20_(1.30.0-wmf.6)).Jun 19 2017, 8:00 AM

Thank you @EBernhardson 8-]

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

• pmiazga mentioned this in rEEPS0d79b5ae6be7: Remove phan supress checks annotation.Jun 19 2017, 1:42 PM

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

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