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.

In T228155#5337535, @Yaron_Koren wrote:

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

In T228155#5337679, @Yaron_Koren wrote:

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.

In T228155#5337705, @Yaron_Koren wrote:

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

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.

Change 523162 abandoned by Daimona Eaytoy:

[mediawiki/extensions/PageForms@master] [WIP] Add phan

Reason:

It's easier to start over

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

There's a bunch of errors: https://integration.wikimedia.org/ci/job/mwext-php72-phan-docker/136559/console

Some are due to SMW not being cloned, but I'm not even sure if our CI infrastructure allows that (@hashar?).

Change 721873 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/extensions/PageForms@master] Fix some issues spotted by phan

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

Change 721873 merged by jenkins-bot:

[mediawiki/extensions/PageForms@master] Fix some issues spotted by phan

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

Change 722637 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/extensions/PageForms@master] Fix more issues spotted by phan

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

Change 722644 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/extensions/PageForms@master] Fix even more issues spotted by phan

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

Change 722637 merged by jenkins-bot:

[mediawiki/extensions/PageForms@master] Fix more issues spotted by phan

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

Change 721865 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/extensions/PageForms@master] Enable phan

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

Change 722644 merged by jenkins-bot:

[mediawiki/extensions/PageForms@master] Fix even more issues spotted by phan

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

Change 721865 merged by jenkins-bot:

[mediawiki/extensions/PageForms@master] Enable phan

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