PHPStan Code Review Comments Against a PR

Basic overview on getting PHPStan to post errors as code review comments on a pull request.

PHPStan

GitHub link: https://github.com/phpstan/phpstan
“Static code analysis helps find errors in your code without actually running it. It catches whole classes of bugs even before you write tests for the code.“

Here are a few examples of some of the types of errors it will pick up on.

SARB – Static Analysis Results Baseliner

GitHub link: https://github.com/DaveLiddament/sarb

Introducing static code analysis into your process should improve the quality of your code and make the QA process more efficient due to finding errors earlier on.

However running any static code analysis on an existing project is likely to raise hundreds if not thousands of errors, one particular project I was helping to maintain had in excess of twenty thousand errors. The time involved in fixing all these issues wasn’t a viable option and yet changes being made to the application contained errors that could have been picked up by static code analysis. These would normally be caught by automated/manual testing or by users of the application once it had been released.

SARB lets you take a baseline reading of your code so that you can use static analysis to identify issues exclusively in your changes.

“SARB is used to create a baseline of these results. As work on the project progresses SARB can takes the latest static analysis results, removes those issues in the baseline and report the issues raised since the baseline. SARB does this, in conjunction with git, by tracking lines of code between commits.“

CI Process

New or updated pull requests will trigger a webhook from BitBucket to tell Jenkins to start running static code analysis on the branch. This will use the baseline that exists in the current branch.

High level overview diagram:

Updating the baseline

I’m sure the intention of the author of SARB is that once the baseline has been created it probably shouldn’t be done again. It could be argued that why would you run a new baseline to effectively ignore/hide errors that have been introduced since introducing static code analysis to the process.

My intention is to only see errors related to the changed code for the pull request so to keep code review focused on the specific changes and restrict the scope of the change increasing. If we want to stop code with errors being merged then a pre-merge check could be run which fails on errors. I haven’t done that as I want the decision to remain with those responsible for merging the pull request. That should mean they don’t merge if there is an outstanding error, if they did decide to merge then the error would be treated exactly the same as any other pre-baseline error in the context of any future branches.

Diagram of actions when a pull request is merged:

Code Review Preview

Once the errors have been sent as comments via the API this is how they look against a pull request in code review. Tasks against each comment could be included which would force the errors to be resolved or for someone to check a box next to each outstanding error before a merge could be performed.

Example video of a new pull request being created and comments appearing against it: