Static Analysis with Psalm PHP

October 16, 2019
Written by

Copy of Generic Blog Header 4-10.png

PHP is great, but its loosely-typed goodness is a double-edged sword. On the one hand, it gives PHP the legendary low barrier to entry, but on the other, magical type juggling inevitably means bugs.

Because PHP is an interpreted rather than compiled language, catching these type problems is tricky, and often they'll go unnoticed for a long time before they catch up with us somewhere down the road. Problems sit and fester in our code base waiting to strike. Unused code can clutter our application, and there is a myriad of other ways in which we write PHP that won't produce errors but are bugs.

There are some excellent tools to help us catch errors and find bugs before we even run our code. IDEs like PhpStorm can inspect our code and give us information on potential problems before a script is ever executed.

Screenshot of PhpStorm IDE showing an inspection finding an unused variable

Catching problems with our code before we run it is delightful. Bugs not making it out of our editor is a productive state to be in, but what if we're not using an expensive IDE, or we ignore the warnings and commit the code regardless? In an ideal world, we want to add the code inspection step into our continuous integration system so that we can reject buggy code before it makes it to production.

On this week's stream, we looked at Psalm, a PHP static analysis tool from the fine video hosting company Vimeo, that can find bugs in your code from the command line. It's handy in a couple of situations, bringing an unwieldy code base into line, and keeping it clean.

Finding Problems

We're going to look at the open-source smoke testing tool Cigar. It's a cool project to investigate as it's not too large, which makes dipping our toes into static analysis much more manageable. Once we've cloned the project from GitHub and installed the dependencies with Composer, we can run the test suite to make sure everything is passing as a baseline. Cigar uses the atypical Kahlan test runner, so to run the tests we call:

vendor/bin/kahlan --reporter=verbose

Animated GIF showing the the tests passing

I ran this on a Mac development environment, but everything should work just fine on Windows or Linux

The tests pass, so we can assume that this code is bug-free, right?

Let's install Psalm and see if it has any opinions on the quality of this code. We install Psalm using Composer and declare it as a development dependency.

composer require --dev vimeo/psalm

We can tell Psalm to generate a default config file for us, but first, we need to talk about strictness levels. Psalm can work at a variety of levels to let you solve the most critical problems first, working your way up to the most strict level. Currently, 8 is the loosest level, and 1 is the strictest. Level 1 expects everything to be type-defined and can be a struggle for even a medium-size codebase, so as a start, let's get Psalm to create a psalm.xml config file at level 8 and see what it has to offer us.

php vendor/bin/psalm --init src/ 8

The first parameter tells Psalm where to find our source code, and the second is the strictness level.

Now, we can run Psalm, cross our fingers, and hope it's not too bad.

php vendor/bin/psalm 

Scanning files...
Analyzing files...

IIIIIIII

INFO: PossiblyNullPropertyAssignmentValue - src/AsyncChecker.php:26:38 - $this->authorizationHeader with non-nullable declared type 'string' cannot be assigned nullable type 'string|null'
        $this->authorizationHeader = $authorizationHeader;


INFO: NullArgument - src/AsyncChecker.php:76:34 - Argument 2 of curl_multi_exec cannot be null, null value provided to parameter with type int
            curl_multi_exec($mh, $running);


INFO: MissingReturnType - src/EchoWriter.php:14:21 - Method Brunty\Cigar\EchoWriter::writeErrorLine does not have a return type, expecting void
    public function writeErrorLine(string $message)


INFO: MissingReturnType - src/EchoWriter.php:19:21 - Method Brunty\Cigar\EchoWriter::writeResults does not have a return type, expecting void
    public function writeResults(int $numberOfPassedResults, int $numberOfResults, bool $passed, float $timeDiff, Result ...$results)


INFO: MissingReturnType - src/EchoWriter.php:33:22 - Method Brunty\Cigar\EchoWriter::writeLine does not have a return type, expecting void
    private function writeLine(Result $result)


INFO: MissingReturnType - src/EchoWriter.php:44:22 - Method Brunty\Cigar\EchoWriter::writeStats does not have a return type, expecting void
    private function writeStats(int $numberOfPassedResults, int $numberOfResults, bool $passed, float $timeDiff)


INFO: MissingReturnType - src/JsonWriter.php:7:21 - Method Brunty\Cigar\JsonWriter::writeErrorLine does not have a return type, expecting void
    public function writeErrorLine(string $message)


INFO: MissingReturnType - src/JsonWriter.php:15:21 - Method Brunty\Cigar\JsonWriter::writeResults does not have a return type, expecting void
    public function writeResults(int $numberOfPassedResults, int $numberOfResults, bool $passed, float $timeDiff, Result ...$results)


INFO: MissingReturnType - src/Outputter.php:23:21 - Method Brunty\Cigar\Outputter::writeErrorLine does not have a return type, expecting void
    public function writeErrorLine(string $message)


INFO: MissingReturnType - src/Outputter.php:32:21 - Method Brunty\Cigar\Outputter::outputResults does not have a return type, expecting void
    public function outputResults(array $passedResults, array $results, float $startTime)


INFO: PossiblyNullArgument - src/Parser.php:24:32 - Argument 1 of rtrim cannot be null, possibly null value provided
        $this->baseUrl = rtrim($baseUrl, '/');


INFO: MissingClosureParamType - src/Parser.php:43:35 - Parameter $value has no provided type
        return array_map(function($value) {


INFO: RedundantConditionGivenDocblockType - src/Parser.php:71:13 - Docblock-asserted type string can never contain null
        if ($this->baseUrl !== null && ! isset($urlParts['host'])) {


INFO: PossiblyNullPropertyAssignmentValue - src/Result.php:31:27 - $this->contents with non-nullable declared type 'string' cannot be assigned nullable type 'string|null'
        $this->contents = $contents;


INFO: MissingReturnType - src/Result.php:40:21 - Method Brunty\Cigar\Result::getContentType does not have a return type, expecting null|string
    public function getContentType()


INFO: MissingReturnType - src/Result.php:55:21 - Method Brunty\Cigar\Result::getContents does not have a return type, expecting string
    public function getContents()


INFO: MissingReturnType - src/Url.php:65:21 - Method Brunty\Cigar\Url::getContent does not have a return type, expecting null|string
    public function getContent()


INFO: MissingReturnType - src/Url.php:70:21 - Method Brunty\Cigar\Url::getContentType does not have a return type, expecting null|string
    public function getContentType()


INFO: MissingReturnType - src/Url.php:75:21 - Method Brunty\Cigar\Url::getConnectTimeout does not have a return type, expecting null|int
    public function getConnectTimeout()


INFO: MissingReturnType - src/Url.php:80:21 - Method Brunty\Cigar\Url::getTimeout does not have a return type, expecting null|int
    public function getTimeout()


INFO: MissingReturnType - src/WriterInterface.php:7:21 - Method Brunty\Cigar\WriterInterface::writeErrorLine does not have a return type
    public function writeErrorLine(string $message);


INFO: MissingReturnType - src/WriterInterface.php:9:21 - Method Brunty\Cigar\WriterInterface::writeResults does not have a return type
    public function writeResults(int $numberOfPassedResults, int $numberOfResults, bool $passed, float $timeDiff, Result ...$results);


------------------------------
No errors found!
------------------------------
22 other issues found.
You can hide them with --show-info=false
------------------------------

Checks took 0.38 seconds and used 44.300MB of memory
Psalm was able to infer types for 93.0796% of the codebase

Interestingly, Psalm found no errors, which is a very encouraging start. It did find 22 other issues, reported as INFO lines in the output. Anything reported as INFO at a lower strictness level gets reported as an error at higher strictness levels, because we plan on working up through the strictness its safe for us to ignore these INFO issues.

php vendor/bin/psalm --show-info=false

Scanning files...
Analyzing files...

░░░░░░░░

------------------------------
No errors found!
------------------------------

Checks took 0.24 seconds and used 53.803MB of memory
Psalm was able to infer types for 93.0796% of the codebase

Hooray, Psalm says we have no errors at the lowest strictness level. Lets ramp it up a little and see how Psalm fairs at strictness level 3 which is the default. You can manually edit the generated psalm.xml file, but I find it easier to delete the file, and re-initialise with the new level.

rm psalm.xml && php vendor/bin/psalm -i src 3

php vendor/bin/psalm --show-info=false

Scanning files...
Analyzing files...

E░E░░░E░

ERROR: PossiblyNullPropertyAssignmentValue - src/AsyncChecker.php:26:38 - $this->authorizationHeader with non-nullable declared type 'string' cannot be assigned nullable type 'string|null'
        $this->authorizationHeader = $authorizationHeader;


ERROR: NullArgument - src/AsyncChecker.php:76:34 - Argument 2 of curl_multi_exec cannot be null, null value provided to parameter with type int
            curl_multi_exec($mh, $running);


ERROR: PossiblyNullArgument - src/Parser.php:24:32 - Argument 1 of rtrim cannot be null, possibly null value provided
        $this->baseUrl = rtrim($baseUrl, '/');


ERROR: PossiblyNullPropertyAssignmentValue - src/Result.php:31:27 - $this->contents with non-nullable declared type 'string' cannot be assigned nullable type 'string|null'
        $this->contents = $contents;


------------------------------
4 errors found
------------------------------

Checks took 0.37 seconds and used 44.284MB of memory
Psalm was able to infer types for 93.0796% of the codebase

Now we have 4 errors to get our teeth into. All four of these errors are to do with nullable types. Let's take a look at the code around the first error.

class AsyncChecker
{

    /**
     * @var bool
     */
    private $checkSsl;

    /**
     * @var string
     */
    private $authorizationHeader;

    /**
     * @var array
     */
    private $headers;

    public function __construct(bool $checkSsl = true, string $authorizationHeader = null, array $headers = [])
    {
        $this->checkSsl = $checkSsl;
        $this->authorizationHeader = $authorizationHeader;
        $this->headers = $headers;
    }

Psalm has noticed that while we are doing some standard dependency injection property setting, our types don't match. The second parameter of the constructor tells us to expect a string as the second $authorizationHeader with a default value of null, yet the docblock annotation for the $authorizationHeader property of our class states that the property will always be a string. Which is correct? If we think the string should be able to be null, we can fix it by updating the docblock to make it a string or null.

class AsyncChecker
{

    /**
     * @var bool
     */
    private $checkSsl;

    /**
     * @var string|null
     */
    private $authorizationHeader;

    /**
     * @var array
     */
    private $headers;

    public function __construct(bool $checkSsl = true, string $authorizationHeader = null, array $headers = [])
    {
        $this->checkSsl = $checkSsl;
        $this->authorizationHeader = $authorizationHeader;
        $this->headers = $headers;
    }

But should this string ever be null? Fixing these problems makes us consider how types are working across our code. It might be more reasonable to never expect that string to be null and to set it to an empty string by default. We need to consider how these values are used across the code and consider reasonable types and values.

Running Psalm again shows us only 3 errors, so we've fixed one problem.

It may seem petty to flag an incorrect type annotation as a problem, but these things are essential in a project, and become more critical the more extensive a project or team becomes. We rely on type annotations when upgrading applications to the typed properties that ship in PHP 7.4. Annotations that are currently informational will become code that’s enforced. Let's get things right now, ready for the exciting new shiny tools of the future.

Let's fix one more issue, the next one in the list.

ERROR: NullArgument - src/AsyncChecker.php:76:34 - Argument 2 of curl_multi_exec cannot be null, null value provided to parameter with type int
            curl_multi_exec($mh, $running);

It seems like we're passing in a null value into curl_multi_exec when it's expecting an int.

$running = null;
do {
    curl_multi_exec($mh, $running);
} while ($running);

We're setting $running to null, and as curl_multi_exec executes all the HTTP requests we've asked it to, it updates the value of $running to 1. Once it's finished, it sets $running to 0. It’s a wonderful example of PHP's Fractal of Bad Design, but it is also a genuine bug in our code. We're expecting $running to be null if the code isn't running, but the function we're using is expecting it to be 0. It is a prime example of where PHP's magical type coercion is harming us hugely. In an ideal world Curl would throw an error here telling us exactly what Psalm is telling us because this type mismatch will hurt us somewhere down the line.

To fix the problem, we change the initial value of $running from null to 0.

$running = 0;
do {
    curl_multi_exec($mh, $running);
} while ($running);

When we rerun Psalm, we've fixed this problem. If you're interested in how we fixed the problems up to maximum strictness, take a look at pull requests #41 and #42 that fix all the Psalm issues.

Running as part of the Build

Now that we have our code ship-shape and Bristol fashion it must stay that way. We want our continuous integration system to fail the build if we introduce any Psalm detected problems. If we commit our new composer.json with Psalm as a dev-dependency, it's as straightforward as adding a new step to the script: block if we're using Travis.

script:
  - vendor/bin/kahlan --reporter=verbose --clover=coverage.xml
  - vendor/bin/psalm

Psalm fails with an exit code of 1 if it finds errors, just like our test runners and code quality tools. We can run it as part of the build, and the build fails if Psalm finds any problems.

Quality is hugely important in modern software development, and static analysis tools like Pslam are essential in producing quality maintainable applications. Let me know on Twitter or in the comments how you find Psalm, and what tools or libraries you'd like me to investigate in the future. I can't wait to see what you build.