DEV Community

Revin
Revin

Posted on Originally published at revin.com.br

PHPCompatibility flagged 217 errors in a PHP 5.6 app. The one that cost money wasn't in the report

Last month someone handed me read access to a PHP 5.6 codebase and asked whether going to PHP 8 was the same size of job as dropping Laravel on top of what already existed. Roughly 90k lines, procedural, a homemade router, MySQL 5.7 underneath, running order capture and shipping rules for a distributor. Three developers in its history, none of them still at the company.

The team had been arguing about it for two weeks with zero numbers on the table. Instead of joining the argument I ran a scanner, and then I did the boring thing nobody wants to do first: I froze the current behavior of the money flows before touching the version.

Good thing, because the change that would have cost real money never showed up in the scanner report.

The linter counts the damage in about half an hour 

composer require --dev \
  squizlabs/php_codesniffer \
  phpcompatibility/php-compatibility

vendor/bin/phpcs -p ./src \
  --standard=PHPCompatibility \
  --runtime-set testVersion 8.2 \
  --report=summary
Enter fullscreen mode Exit fullscreen mode

Output, trimmed:

PHP CODE SNIFFER REPORT SUMMARY
--------------------------------------------------------------
FILE                                        ERRORS  WARNINGS
--------------------------------------------------------------
src/legacy/order_functions.php                  58        11
src/legacy/db.php                               41         3
src/legacy/mailer.php                           22         9
src/shipping/tier.php                            9         2
... (7 more files)
--------------------------------------------------------------
A TOTAL OF 217 ERRORS AND 89 WARNINGS WERE FOUND IN 11 FILES
--------------------------------------------------------------
Enter fullscreen mode Exit fullscreen mode

217 errors across 11 files, in a codebase that everybody in the room described as "unfixable". Almost all of it mechanical: each() removed, create_function() gone, mysql_* calls that already died back in 7.0, arguments passed by reference where that is no longer allowed. Rector cleared a large slice of it unattended:

vendor/bin/rector process src --set php82 --dry-run
Enter fullscreen mode Exit fullscreen mode

What was left after the dry run was a long afternoon of manual edits. Annoying, not a quarter of work.

The change that does not raise an error

Loose comparison between a string and a number changed in PHP 8. On 5.6, 0 == "abc" returned true. On 8, it returns false. Nothing throws, no log line appears, the if simply picks the other branch.

In this codebase it lived inside a switch, which is loose comparison wearing a costume:

function shippingTier($code) {
    switch ($code) {
        case 0:            // matches ANY non-numeric string on 5.6
            return 'free';
        case 1:
            return 'flat';
        default:
            return 'standard';
    }
}

echo shippingTier('EXPRESS');
// PHP 5.6 -> free
// PHP 8.2 -> standard
Enter fullscreen mode Exit fullscreen mode

Same with in_array(0, ['EXPRESS', 'ECONOMY']), true on 5.6 and false on 8. A business rule flips and nobody signs off on it. The customer finds out from an invoice total three weeks later.

Grep does not save you here. I tried: searching for == in 90k lines returns thousands of hits, and reading them one by one is how you convince yourself you read them all.

Freeze the behavior, then bump the version

What worked was characterization tests over the four flows that generate revenue. Not unit tests of what the code should do. A recording of what it does today, ugly parts included.

// tests/Characterization/ShippingTierTest.php
$inputs = ['EXPRESS', 'ECONOMY', '0', '', '0.0', 0, 1, '1abc', null, false];

$fh = fopen(__DIR__ . '/golden.txt', 'w');
foreach ($inputs as $in) {
    fwrite($fh, sprintf("%-10s => %s\n", var_export($in, true), shippingTier($in)));
}
fclose($fh);
Enter fullscreen mode Exit fullscreen mode

Run it on the old runtime, keep the file, run it on the new one, diff:

php5.6 tests/Characterization/ShippingTierTest.php && mv tests/Characterization/golden.txt golden-5.6.txt
php8.2 tests/Characterization/ShippingTierTest.php && mv tests/Characterization/golden.txt golden-8.2.txt
diff golden-5.6.txt golden-8.2.txt
Enter fullscreen mode Exit fullscreen mode 
1c1
< 'EXPRESS'  => free
---
> 'EXPRESS'  => standard
2c2
< 'ECONOMY'  => free
---
> 'ECONOMY'  => standard
5c5
< '1abc'     => flat
---
> '1abc'     => flat
Enter fullscreen mode Exit fullscreen mode

Three lines of diff on the flow that decides shipping cost. That is the whole return on the exercise. Across the four money flows I ended up with 3 behavior changes: two harmless, one that would have handed free shipping to nobody who had it before, or the reverse, depending on which way the data leaned that month.

Two things I tried that did not work

First attempt was to lean on the existing suite. The repo reported 34% coverage, which sounded like something. Opening the tests, a good chunk of them called the method and asserted nothing at all, no assert* anywhere in the body. The metric existed, the guarantee did not. Coverage told me nothing about whether the version bump changed an outcome.

Second attempt was worse and I am glad it was a branch. I ran a sweep turning == into === in the shipping and pricing files, on the theory that strict is safer. It is safer in a language where types are stable, and this app reads everything from mysqli in the old procedural style, which hands back numeric columns as strings. Half the id comparisons started returning false. Reverted in twenty minutes. Strict comparison is a refactor with its own test bill, not a migration step you sneak in.

Then check what nobody maintains anymore

Half a day of work and the answer changes the plan more than any framework debate:

composer outdated --direct
composer audit
php -m | sort   # native extensions the server actually loads
Enter fullscreen mode Exit fullscreen mode

Watch for mcrypt, which left core in 7.2 and is still glued to homegrown crypto in plenty of shops. If every direct dependency has an 8-compatible release, even one that needs work, the upgrade path is short. If the system is welded to a framework that stopped shipping fixes years ago, part of it gets rewritten either way, and now you know which part: the entry layer, not ten years of business rules.

One detail that shortens meetings: 5.6 has been out of security support since the end of 2018. That does not choose between upgrading and rewriting. It removes the option everybody secretly prefers, which is looking at this next year.

The order that ended up working

  1. Characterization tests over the revenue flows, on the old runtime, golden files committed.
  2. Rector plus manual cleanup for the 217 errors, then diff the golden files and explain every line that moved.
  3. Housekeeping without changing the shape of the thing: Composer with PSR-4, a single front controller, config out of the code, real logging.

Only after that does standing a framework beside it on the same database make sense, one route at a time, new endpoints born there and old checkout left alone until tests can back the move. The system runs split for a while and that bothers everybody who likes clean things. It is also reversible on any given day, which a nine-month rewrite is not.

Measured on this one: the scan took under an hour, the characterization tests took about two days, the actual upgrade landed in a bit over three weeks. The rewrite proposal on the table had been sized at a quarter, and every rewrite I have watched from close up ran past its number.

The part I still do not have a clean answer for: flows that only prove themselves against a third party. Billing, tax invoices, the payment gateway. You cannot golden-file a webhook that only fires when a real customer pays. How do you snapshot behavior on those before a version jump? Recorded HTTP fixtures, a sandbox, or do you just ship it and watch the logs?

Originally published on the Revin blog: https://revin.com.br/en/blog/php-5-6-to-php-8-or-rewrite

Top comments (0)