Page MenuHomePhabricator

Add phan to PageForms
Open, Needs TriagePublic

Description

It will help to catch simple errors during the development phase, instead of at deployment time, and hopefully avoid things like T228049.

This is blocked on the subtask for the BC policy, and we'd also need SMW to be loaded for testing. In case this isn't possible (and I suspect it isn't), then I'll just stub whatever classes and methods PageForms is using.

Event Timeline

Daimona created this task.Jul 16 2019, 11:55 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 16 2019, 11:55 AM

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

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

@Daimona - thanks for putting this together, and of course for your work on the actual patch, which looks like it will make a lot of improvements to the code. Just to clarify one thing: Page Forms doesn't require Semantic MediaWiki, and is often used without SMW, so SMW doesn't need to be loaded for any testing.

@Daimona - thanks for putting this together, and of course for your work on the actual patch, which looks like it will make a lot of improvements to the code. Just to clarify one thing: Page Forms doesn't require Semantic MediaWiki, and is often used without SMW, so SMW doesn't need to be loaded for any testing.

Yeah, I saw that. Unfortunately, phan will complain about undefined classes if SMW is not installed in the test environment, so it needs a way to know about the classes/methods. And that also goes for optional dependencies. There are usually 3 choices:

  1. Install the required extension in the docker image - This is the safest choice, and what we did for most MW extensions
  2. Add stubs for phan, like here - This is lighter for CI and easier to implement, although it introduces some duplications, and stubs will have to be updated if the repo they're mocking is updated. This is probably what we'll have to do with SMW here.
  3. Suppress phan errors related to undeclared classes etc. - If done on a per-issue base, this could add tons of extra annotations and could possibly clog the codebase. Suppressing errors at file- or repo-level, instead, would make phan miss those kinds of errors even when legit, so this is discouraged.

Unless someone more familiar with CI proves me wrong, stubs are probably our best option here.

Hi,

Unfortunately, phan will complain about undefined classes if SMW is not installed in the test environment,

Are you sure about this? Page Forms always checks whether SMW classes exist before using any of them, as far as I know.

Hi,

Unfortunately, phan will complain about undefined classes if SMW is not installed in the test environment,

Are you sure about this? Page Forms always checks whether SMW classes exist before using any of them, as far as I know.

Yes, phan only performs static analysis and won't read class_exists or similar checks. See here, we have several warnings containing "SMW".

Oh, okay. That's too bad.

Oh, okay. That's too bad.

Well, not necessarily bad :-) I guess that helps to ensure the extension remains compatible with SMW without breaking anything, even if it's just an optional dependency. Using stubs sort of weakens this purpose, but IMHO it's still helpful.

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

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

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

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

Mentioned in SAL (#wikimedia-releng) [2019-07-17T16:49:09Z] <James_F> Zuul: [PageForms] Add phan dependencies on AdminLinks, Cargo, PageSchemas T228155

hashar added a subscriber: hashar.Jul 18 2019, 7:57 AM

Given PageForms is supposedly usable without SMW, would it makes sense to have run TWO phan analysis? One with SMW and another one without it?

I thought about that for testing extension, ie to test with only the required dependencies and run tests again with the optional dependencies. But there is no way to differentiate between required and optional deps yet.

@hashar Being a static analysis tool, it wouldn't make any difference. We only need SMW to provide phan with all of the classes/methods definitions, as it would otherwise fail. My actual doubt is whether we can install SMW before doing the analysis, since AFAIK it's not hosted on gerrit.