N°6644 - Add static analysis for PHP

[Molkobain]

Internal PR

Objective

Introduce static analysis in iTop and our modules to better detect code issues and compatibility with the PHP versions supported by each branches.

Framework choice

Initial choice

At first, I was really into Phan for the following reasons:

• Ability to run analysis for a range of PHP version at once (eg. 7.4 => 8.1)
• Configuration file made out of a PHP array which made it easy to craft depending on many conditions, which was perfect for the CI
• Community
• Ability to develop a plugin if the need comes

But it has a main drawback, its baseline.

The generated baseline only keeps track of the types of issues in a file but neither their counts nor their lines, which means that if an issue is already present for a file in the baseline, it won't detect a new occurrence of that issue.

Example of baseline

return [
    'file_suppressions' => [
        'foo/file-a.php' => ['PhanTypeMismatchArgumentInternal'],
        'foo/file-b.php' => ['PhanTypeUndeclaredClass'],
        'bar/file-c.php' => ['PhanTypeMismatchArgumentInternal', 'PhanTypeUndeclaredClass'],
        [...]
    ],
];

This is a no-go for us as we want to keep a accurate baseline so we can fix any issue from new pieces of code and leave existing (non critical) issues in baseline for correction over time.

New choice

Finally, I chose PHPStan because it was the closest contender to Phan with a good enough baseline. It offers almost everything we look for in Phan except:

• Ability to run analysis for a range of PHP version at once (eg. 7.4 => 8.1)
  => We will rely on the PHP versions rotation of the CI
• Configuration file made out of a PHP file
  => It's made out of a NEON file, but handles includes and PHP files to bootstrap the analysis, so we'll be able to do what we need

Also, PHPStan seems to offer better ways to suppress false positives due to polymorphism.

The baseline still isn't perfect as it doesn't keep track of the issues line numbers, so if we fix one but introduce another, the analysis result will see no change.
But keeping track of line numbers in a baseline is difficult as they change all the time due to codebase modifications. So we accept that, having the counts seems good enough.

Example of baseline

parameters:
    ignoreErrors:
        -
            message: "#^Call to static method GetSharedClassProperties\\(\\) on an unknown class SharedObject\\.$#"
            count: 1
            path: ../../../addons/userrights/userrightsprofile.class.inc.php

        -
            message: "#^Call to static method InitSharedClassProperties\\(\\) on an unknown class SharedObject\\.$#"
            count: 1
            path: ../../../addons/userrights/userrightsprofile.class.inc.php

        -
            message: "#^Call to static method GetSharedClassProperties\\(\\) on an unknown class SharedObject\\.$#"
            count: 1
            path: ../../../addons/userrights/userrightsprofile.db.class.inc.php

        [...]

How the PR works

This PR was designed to be very easy to use either for a developer on it's computer and in the CI.

• Everything lies in the <ITOP>/tests/php-static-analysis folder
• Installation is as easy as for unit tests (see included README.md) : composer install and the documented command line
• Comes with 2 configuration files and command lines:
  • 1 for analysing an iTop package (iTop core, included modules, third-party libs)
  • 1 for analysing 1 or more modules only
• Can also be configured to show as regular PHPStorm inspection warnings (Tutorial here) but it's pretty slow

If you are interested in testing this PR, you should take a look at the included README.md.

Proposed approach for static analysis deployment

• iTop
  • Support branches (2.7, 3.0, 3.1)
    • Generate baseline with basecode as-is and fix only issues found during new developments
    • Eventually fix existing PHP compatibilty issues after discussing it with the QA team
  • Develop branch (3.2) and future releases
    • Generate baseline with basecode as-is
    • Fix issues found during new developments
    • Fix existing PHP compatibility issues
    • Start fixing existing issues during the modernization phase, that way each version we could clean the basecode a bit more while keeping maximum stability
• Modules
  • TBD

As for the rules level we should start with, it as to be discussed to define what we want to try to match for new developments. I would go with level 5 for new developments and at least level 1 (or 3) for what we want to fix in the existing codebase.

What remains to be done in this PR

• Discuss if going with PHPStan is ok with the rest of the team
• Discuss which rules level we want to match with new developments
• Check if it can be added to the CI pipeline with the current implementation
  • PHPStan requires PHP 7.2+ to run, so it won't run on CI jobs with PHP 5.6-7.1. We have to find a solution. Olivier talked about running PHPStan in a separate container like it was done for Behat earlier.
  • Find best way to pass PHP version to check to the CI
    • First option, check if it works out of the box. As the docker image will "composer install" PHPStan, its "platform_check.php" file might contain the PHP version executed at that time, which will roll as expected
    • Second option, we could have a .neon file for each PHP version extending the for-package.dist.neon file, but seems rather heavy to maintain
    • Third option, add a PHP include in the base config file that sets PHP version from the PHP version running in the CLI to work around the current limitation.
      Main drawback is that it defines the PHP version running PHPStan as well and it won't allow to analyse PHP 5.6 => 7.1. So we might not consider it.
    • Make a PR on PHPStan to add the --php-version argument to the command line. Issue created to see if a PR would be considered.
      Issue rejected, they recommend option 2.
• Generating baseline for iTop repo and packages

[rquetiez]

Have you investigated means to ignore errors by the mean of PHP anotations?
Phan allows that : https://github.com/phan/phan/wiki/Annotating-Your-Source-Code#phan-suppress-next-line

Though it would be cumbersome to establish the first baseline, then the job would be far easier.

[Molkobain]

Indeed I did, here are some of the observations:

• With Phan it wasn't good enough IMHO because it required to add vendor-specific annotations (@phan-xxx) and I didn't wanted to pollute the codebase with something specific a particular lib that we might change.
  That being said, I thought that maybe the CI could do a global search/replace of the stanadard annotations (or combodo-specific ones) to replace them with the ones of the chosen vendor.
• With PHPStan, it seems that standard annotations can be used, we just have to move some of the existing to the right place (sometime a line or two below) so they are more relevant. For other false positives, we just need to add them.

That's why this wasn't a stopper at the time, but it can of course be discussed.