Legacy PHP Application: PHP Code Sniffer Compatibility Standard

Part 1 of this series covers a tour of my 14+ year old legacy CMS called LampLight and it continues with my use of command line code analysis tools to migrate it from PHP v 5.6 to 7.3. In this part I'll use PHP_CodeSniffer to check my applications compatibility with PHP v 7.3 and beyond.

Let's not fuss around, and instead dive right into the meat of this post. At the root of my project I run:

phpcs --standard=PHPCompatibility --extensions=php,inc,lib -s --runtime-set testVersion 7.3- . 

Hot damn it's not too bad! A couple pages of errors flagged, mostly boiling down to these two:

Line Ending Madness

Good grief, I can't seem to escape this mixed line endings issue. It's been flagged and fixed repeatedly. Opening the file in VSCode, reveals down in the bottom right corner an indicator of CRLF meaning Windows style endings. I want everything to use Linux style endings (LF), I work on a Linux machine, this shouldn't be complicated. Unfortunately a lot of my old code was written on Windows.

The odd thing is that PHP_CodeSniffer didn't even flag all the files that were problematic. I suppose it's only noticing files where there are both types of endings.

At this point I ran through several attempts to fix it using tools like dos2unix and other scripts off of StackOverflow, my usual git reset tricks, and more. It was an hour-long tangent and nothing seemed to work.

I discovered that most of these tools, including Git's methods, can convert files with consistent line endings... but once a file has MIXED endings, they aren't automatically converted. Though I can't figure out why my fixers weren't doing it. UG.

Ultimately I ended up opening every single file in VSCode and manually select-all and convert to LF endings. It took me about 10 minutes which is way less time than I spent researching it. I'm now fairly certain that all text files use LF line endings. (Knock on wood.)

Running the PHP_CodeSniffer command again and... AAARRRGGGH! It's flagging one file as still having mixed line endings. Opening it in VSCode indicates LF endings. Using my bash cat command replacement bat on the file, shows mixed endings indeed.

So in VSCode I select all and convert to CRLF and then back again to LF endings. Drum roll... hallelujah PHP_CodeSniffer no longer reporting mixed line ending issues. VSCode is lacking in this area. It needs a 'mixed' indicator.

Let's move on to the real issues that PHP_CodeSniffer revealed.

Farewell get_magic_quotes_gpc()

Well, well, I'll try not to blush as I write this, but PHP_CodeSniffer is flagging a few uses of get_magic_quotes_gpc() in my code. A dig into the PHP docs reveals that this oft-maligned feature of PHP was removed as of PHP 5.4.0! So, this is clearly a hold-over of some very, very, very old code. Let's look at what it's doing:

function stripSlashesDeep($value)
{
    $value = is_array($value) ? array_map('stripSlashesDeep', $value) : stripslashes($value);
    return $value;
}
if (get_magic_quotes_gpc()) {
    $_GET    = stripSlashesDeep($_GET);
    $_POST   = stripSlashesDeep($_POST);
    $_COOKIE = stripSlashesDeep($_COOKIE);
}

This first instance is in my bootstrap file and is just cleaning the automatically inserted slashes from the request globals. I'll just remove this completely.

The final instance of magic quotes is in a string escaping method in my database class/facade which wraps the mysqli functions.

public function escapeStr($inStr, $stripSlashes=false)
    {
        $this->connect(); // If not already connected.

        if ($stripSlashes) {            
            if (get_magic_quotes_gpc()) {
                $inStr = stripslashes($inStr);                
            }
        }

        $inStr = mysqli_real_escape_string($this->dbc, $inStr);
        return $inStr;
    }

Since it's no longer needed I'll just remove the section resulting in:

public function escapeStr($inStr, $stripSlashes=false)
    {
        $this->connect(); // If not already connected.
        $inStr = mysqli_real_escape_string($this->dbc, $inStr);
        return $inStr;
    }

Now is a good time for a test of the application and a git commit. What's next?

Indirect Access Ambiguity

The final error says:

Indirect access to variables, properties and methods will be evaluated strictly in left-to-right order since PHP 7.0. Use curly braces to remove ambiguity.

This error is flagged in two spots. Let's look at the code:

$$_SERVER["SERVER_NAME"] = $oldservername;

Well, well how embarrassing. This is actually a typo and the two dollar signs shouldn't be there. Easy enough to fix. The other instance is the same.

Had this been a real use of variable variable names, or indirect access to variables... we can use curly braces to specify how to evaluate it. For example:

$$whichVar = "New value";

Becomes:

${$whichVar} = "New value";

With this issue fixed, let's commit to Git, then run the command one more time to verify that this PHP_CodeSniffer isn't finding anything else with this compatibility check. Great!

Migration Wrap Up

One of the most difficult parts of this migration process is that none of my older projects have automated tests of any kind. This exercise has really driven home why I need them and cemented my existing determination to create tests with all future projects. That said however, it has been surprisingly easy to migrate each individual project, with the help of these excellent tools. The main challenge is the sheer number of projects I have to migrate, and my limited time to allocate to this process.

In summary, here are the CLI tools I've been using:

Can be safely run automatically:

php-cs-fixer fix . --rules=full_opening_tag,no_closing_tag,@PSR1,@PSR2 --verbose --dry-run --using-cache=no

php-cs-fixer fix . --rules=@PHP70Migration,@PHP71Migration,@PHP73Migration --verbose --dry-run --using-cache=no

Should probably do manually:

php-cs-fixer fix . --rules=@PHP70Migration:risky,@PHP71Migration:risky,-declare_strict_types,-void_return --verbose --dry-run --using-cache=no --allow-risky=yes

phpcs --standard=PHPCompatibility --extensions=php,inc,lib --runtime-set testVersion 7.3- . 

Plus:

• Clean out commented out or dead code.
• Remove abandoned tiny projects and proof of concepts that are lying around inside other projects.
• Organize project structure.
• Get it running on my local dev environment.
• Fix obvious issues like the use of __autoload() and count().
• Migrate Mercurial to Git.
• Full manual test on PHP v 7.3.

It would be a great idea to integrate these types of checks into your Continuous Integration process, or at least in a pre-push or pre-commit Git hook to help keep these issues from sneaking back in. Especially style issues that aren't caught by tests. This is something I'm in the process of doing myself.

I hope this series was helpful, and has given you a starting point for migrating legacy PHP projects, using command line analysis tools, and trusting automated fixer to tidy and correct your code.

Thanks for reading!

--
Originally Published on Blog.SyntaxSeed.com