MJ Convento

/

PHP 7.1 in a codebase that still remembers 5.3

1,906 words, about 8 min read

The oldest file I touched last month has a docblock dated 2011 and a function in it that takes six arguments, four of which can be null, and none of which are declared. It was written for PHP 5.3 and it still runs. In December I put it on PHP 7.1 and it still ran, which is the part of this job nobody writes blog posts about.

PHP 7.1.0 shipped on 1 December, six release candidates after the first one. The reporting worker went to it on 19 December, came back off the next evening, and went on again on 4 January, where it has stayed. The web tier is still on 7.0.14. 7.1.1 arrived on Thursday, so the plan is 7.1.1 everywhere in February. The back office here is two codebases sharing a document root: a framework era half with type declarations and tests, and an older half that predates all of it and knows about $_REQUEST.

What follows is sorted by what it actually did to that older half. Some of 7.1 let me delete code. One part of it cost a full day and a rollback.

Nullable types delete guard clauses, not null

The nullable types RFC adds a leading ? to any type, in parameters and in return types. The return position is the one that matters, because before 7.1 there was no way to say “a string or nothing” on the way out, which meant every such function had an untyped return and a docblock nobody enforced.

Here is a real shape from the statements module, before:

<?php

namespace AppStatement;

class PeriodFormatter
{
    /**
     * @param string|null $label
     * @return string|null
     */
    public function normalise($label)
    {
        if ($label === null) {
            return null;
        }

        if (!is_string($label)) {
            throw new InvalidArgumentException('label must be a string or null');
        }

        return trim($label) === '' ? null : trim($label);
    }
}

After:

<?php

namespace AppStatement;

class PeriodFormatter
{
    public function normalise(?string $label): ?string
    {
        if ($label === null) {
            return null;
        }

        return trim($label) === '' ? null : trim($label);
    }
}

The is_string check and the docblock both go. That was 31 functions across the statements and export modules, and it is the single largest deletion 7.1 bought us.

One thing that catches people: ?string $label makes the parameter nullable, not optional. The RFC spells it out. foo_nullable(?Bar $bar) called as foo_nullable() is an error, while the old foo_default(Bar $bar = null) accepts zero arguments. They are not synonyms, and on 7.1 the difference is louder than it used to be, for reasons I will get to.

void and iterable are mostly documentation, and that is fine

The void return type is checked at compile time, not on call. A return 1; or even a return null; inside a function x(): void is a fatal error before the function is ever invoked, and the message is direct: “A void function must not return a value”. return; on its own is legal. void also cannot be widened by a subclass, because nothing is a subtype of nothing.

Where this earned its place for us was the command and handler classes. About 20 methods in the import pipeline exist purely for their side effects, and at least three callers were treating the returned null as a result and branching on it. Declaring : void did not stop them (using a void function’s return value is still legal and evaluates to null, by design) but it did make the reviews easy to write.

iterable is the other one I use daily now. It accepts an array or anything implementing Traversable, and there is a matching is_iterable(). The variance rules are the useful part: a child may broaden a parent’s array or Traversable parameter to iterable, and may narrow a parent’s iterable return to array. So the row source interface in the importer went from an untyped parameter with a hand written check:

<?php

namespace AppImport;

class LedgerWriter
{
    public function write($rows)
    {
        if (!is_array($rows) && !$rows instanceof Traversable) {
            throw new InvalidArgumentException('rows must be iterable');
        }

        foreach ($rows as $row) {
            // ...
        }
    }
}

to public function write(iterable $rows): void, and the four implementations that return arrays kept returning arrays. Eleven of those is_array plus instanceof pairs came out of the codebase. None of them had ever fired in production. All of them had to be read by somebody.

Destructuring with keys, which is the one the importer wanted

Two features here. Square bracket syntax as an alternative to list(), and keys in list(). The second is the one that changes code, because our importers all deal with associative rows out of PDO:

<?php

// 7.0 and earlier
foreach ($rows as $row) {
    $reference = $row['reference'];
    $amount    = $row['amount_minor'];
    $valueDate = $row['value_date'];
    // ...
}

// 7.1
foreach ($rows as ['reference' => $reference, 'amount_minor' => $amount, 'value_date' => $valueDate]) {
    // ...
}

The reconciliation importer lost 46 lines to this and reads better, with one caveat I hit immediately: a key that is missing from the row destructures to null with an undefined index notice, exactly like an array read, so this is not validation and I stopped pretending it was after the first null value_date reached a date parser.

Class constant visibility and multi-catch

Two small ones I would have paid for separately, both on the new features list. Class constants take public, protected and private now. The legacy half of the codebase has classes with 30 or 40 constants where perhaps six are meant for outside use, and private const finally says so in a way that is enforced rather than implied by a naming convention.

Multi-catch collapses the duplicated handler blocks:

<?php

try {
    $response = $this->gateway->send($message);
} catch (ConnectionException | TimeoutException $e) {
    $this->queue->retry($message, $e);
} catch (RejectedException $e) {
    $this->queue->park($message, $e);
}

Our gateway client had three exception types handled identically, in two places, with the bodies having drifted apart. That is the actual value: not fewer lines, but one body that cannot drift.

The day it cost: too few arguments

Now the expensive part. In 7.1, calling a user defined function with fewer arguments than it declares throws instead of warning. It is an ArgumentCountError, a subclass of TypeError, and it does not apply to internal functions. Under 5.6 and 7.0 the missing parameter was a warning and the variable was null, and code written in 2011 relied on that more often than I would have guessed.

This is what came out of the worker log at 09:12 on 20 December, the morning after that deploy:

PHP Fatal error:  Uncaught ArgumentCountError: Too few arguments to function AppStatementPeriodFormatter::forRange(),
 1 passed in /var/www/app/src/Report/StatementBuilder.php on line 84 and exactly 2 expected
 in /var/www/app/src/Statement/PeriodFormatter.php:31

One call site, written by somebody who knew the second argument defaulted to null in practice because the warning was suppressed in production. There were 14 more like it, and finding them is the problem: a warning in a log you have already learned to ignore becomes a fatal error under load. I grepped, which found maybe half, then ran the suite with error_reporting(E_ALL) and a handler that threw on E_WARNING, which found the rest of the covered paths. A day gone, a rollback that evening, and a second attempt two weeks later that held.

The nullable types RFC carries a related break that hit one interface. Parameter covariance for nullable types is now rejected, so a parent declaring foo(array $f = null) and a child declaring foo(array $f = []) no longer validates, because the parent accepts null and the child must too. The fix in the RFC is foo(?array $f = []). Our audit log interface was exactly this, and the error arrives at compile time, which at least means you cannot ship it.

Three more from the same list, in the order they annoyed me:

  • Arithmetic on invalid strings now warns. '1b' + 'something' gives a notice (“A non well formed numeric value encountered”) for the leading numeric string and a warning (“A non-numeric value encountered”) for the non-numeric one. Our overnight batch went from about 400 log lines to 9,000 on the first night, all of them real, most of them a CSV column arriving as an empty string and being added to a total.
  • String offsets. $str[] = $x is a fatal error now rather than silently turning the string into an array, and assigning to an index of an empty string pads with spaces instead of creating an array, so $a = ''; $a[10] = 'foo'; gives you a string of ten spaces and an f. Two functions in the older half were accumulating into a variable initialised as '' instead of [].
  • rand() and srand() are now aliases of mt_rand() and mt_srand(), and the Mersenne Twister implementation itself was fixed, so rand(), shuffle(), str_shuffle() and array_rand() all produce different output than before. Our fixture builder seeded with srand(4) to get a stable set of test accounts, and every assertion against those accounts failed at once. mt_srand($seed, MT_RAND_PHP) restores the old sequence if you need it. We rewrote the fixtures instead.

mcrypt is deprecated and that is not a find and replace

The mcrypt extension is fully deprecated in 7.1, in favour of OpenSSL, and the plan on record is that it leaves core for PECL in the feature release after this one. Every call logs:

PHP Deprecated:  Function mcrypt_encrypt() is deprecated in /var/www/app/legacy/lib/Crypto.php on line 44

Two columns in this system are encrypted with mcrypt_encrypt() and rijndael-128 in CBC mode, by code from 2011. Swapping in openssl_encrypt() is a two line change for new writes and does nothing for the rows already there, because the padding is not the same: what mcrypt wrote back then does not come out of openssl_decrypt() without me handling the trailing bytes myself. So this is a migration with a backfill, a dual read path while the backfill runs, and a key handling review with people who are not developers. It is on the plan for March, which is early only if you have never watched a key rotation get scheduled.

What I have not resolved: the web tier is still on 7.0.14 because two of the client facing forms run through that older half and I have no coverage on them worth the name. I am not moving them on the strength of a grep. Next week is writing enough of a smoke suite to make the February deploy something other than a guess, and if the mcrypt backfill slips past March we will be running a deprecated extension into a version that does not have it.

Sources

  • PHP news archive, 2016: the 1 December 2016 release date for 7.1.0 and the release candidate schedule before it.
  • PHP news archive, 2017: 7.1.1 released 19 January 2017, alongside 7.0.15 and 5.6.30.
  • PHP 7.1 new features: nullable types, void, iterable, class constant visibility, symmetric array destructuring, keys in list(), and multi catch.
  • PHP 7.1 backward incompatible changes: the ArgumentCountError on too few arguments with the error message format, the string offset changes, and rand() and srand() becoming aliases.
  • PHP 7.1 other changes: the exact notice and warning text for arithmetic on invalid strings.
  • PHP 7.1 deprecated features: ext/mcrypt fully deprecated in favour of OpenSSL, and leaving core for PECL in the next feature release.
  • Nullable Types RFC: nullable is not optional, the inheritance rules, and the rejected parameter covariance that breaks array $f = null in a parent against array $f = [] in a child.
  • Void Return Type RFC: compile time checking, why return null; is refused, and that void cannot be changed during inheritance.