N°6644 - Add static analysis for PHP #536
Open
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
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
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:
=> We will rely on the PHP versions rotation of the CI
=> 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
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.
<ITOP>/tests/php-static-analysis
folderIf you are interested in testing this PR, you should take a look at the included README.md.
Proposed approach for static analysis deployment
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
for-package.dist.neon
file, but seems rather heavy to maintainMain 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.