Anatomy of the Magento PHP 5.4 Patch

Last week eBay’s Magento division (team? section? corporate unit?) released a patch to bring PHP 5.4 compatibility to versions of Magento CE 1.6 and greater, and Magento EE 1.11 and greater. While seemingly out of nowhere, it’s a welcome sign that development and support on the 1.x branch of Magento hasn’t been abandoned. PHP 5.3 entered its end of life critical bug fix cycle last March, and those bug fixes are scheduled to stop in July of 2014, (or March of 2014, or already stopped on January 1st, 2014, depending on how you read the messaging).

Rather than release new versions of each Magento point release, Magento Inc. chose to distribute this upgrade as a unix patch file (available via the download page), with PHP 5.4 compatibility promised for future official releases.

What’s a Patch

Unix patch files are the original version control. The patch process allows a developer on one computer to create a diff set for a number of files, hand that set off to another developer, and that second developer can apply the same changes to their files with the patch program.

Since applying patch files can be tricky, and Magento has chosen to distribute their patch via a shell script wrapper. While this still requires a small degree of technical competence to apply, it does save junior sysadmins from needing to mess around with patch levels.

What’s in the Patch

Unlike the transition from PHP 5.2 to 5.3, which was painful, Magento CE was already mostly compatible with PHP 5.4. If we take a look at one of the patch files, we can see the embedded diff at the end

SUPEE-2631 | EE_1.11.0.0 | v1 | 209945a0d4ad2f72974f299b38ce439687bc7fdb | Thu Dec 12 17:31:24 2013 +0200 | v1.11.0.0..HEAD

__PATCHFILE_FOLLOWS__
[... diff here ...]

And that it only applies to four files

app/code/core/Mage/Catalog/Model/Product.php

app/code/core/Mage/Core/Controller/Varien/Router/Standard.php

app/code/core/Mage/Install/etc/config.xml

app/code/core/Zend/Pdf/FileParserDataSource.php 

Additionally, the changes to these files are small, and without the original developer’s context it’s hard to know why some were applied. Hopefully the rumors are true and we’ll soon have a community manager who can help with these sorts of questions.

In the meantime, here’s my take on the changes.

Array Diffs

First up, there’s this change

#File: app/code/core/Mage/Catalog/Model/Product.php
-            $options->setOptions(array_diff($buyRequest->getOptions(), array('')));
+            foreach ($customOptions as $key => $value) {
+                if ($value === '') {
+                    unset($customOptions[$key]);
+                }
+            }
+            $options->setOptions($customOptions);    

The removed line

$options->setOptions(array_diff($buyRequest->getOptions(), array('')));

does a little bit of trickery to remove blank options from the array returned by $buyRequest->getOptions(). Here’s the same code, broken into multiple lines

$customOptions = $buyRequest->getOptions();    
$customOptions = array_diff($buyRequest->getOptions(), array(''));
$options->setOptions($customOptions);

By diffing the original array against an array with a single blank string, this one liner effectively removes any “blank” item from the array. This has been replaced with a much more explicit/verbose bit of code.

foreach ($customOptions as $key => $value) {
    if ($value === '') {
        unset($customOptions[$key]);
    }
}
$options->setOptions($customOptions);

This code does almost the same thing — except it only removes an item from the array if it’s an empty string. The === ensures this extra type check is made. When I first saw this, I assumed there was some subtle change to PHP’s type coercion between PHP 5.3 and PHP 5.4. Consider this simple test case with the sorts of values that PHP’s type coercion handles in unexpected ways

$test = array(1,'',false,0,null,"0.0",0.0,"0",array());
$result = array_diff($test,array(''));
var_dump($result);

If you run this code in both PHP 5.3 and PHP 5.4, the diff produced array is the same. However, in PHP 5.4, several PHP Notices are fired off

PHP Notice:  Array to string conversion in /path/to/magento/test.php on line 3
PHP Stack trace:
PHP   1. {main}() /path/to/magento/test.php:0
PHP   2. array_diff() /path/to/magento/test.php:3

So, that’s one change explained, with a caveat. The new code that replaces this one liner doesn’t exactly replicate the old behavior. Because it uses $value === '', this means values of false and null will remain in the array. While it’s doubtful this will cause problems in the real world, it’s something to stay aware of if you’re migrating to PHP 5.4 and seeing weird behavior around product options.

SimpleXML and Booleans

The second file patched is

#File: app/code/core/Mage/Core/Controller/Varien/Router/Standard.php
                     foreach ($routerConfig->args->modules->children() as $customModule) {
-                        if ($customModule) {
+                        if ((string)$customModule) {

Here we have another change that seems related to the handling of booleans. The $customModule variable contains an XML node. The old code uses this variable raw in a conditional

if ($customModule) {

The new code performs an explicit conversion of the node to a string

if ((string)$customModule) {

The specific reason for this change is a little cloudy. The new code is certainly more “correct” as a node like

<Packagename_Modulename after="foo"></Packagename_Modulename>

will evaluate true, but without an actual text value a blank module will be slotted into the routers to test for.

array_splice($modules, $position+1, 0, (string)$customModule);

At best the old behavior is confusing, and at worst it introduces a fatal configuration error.

However, this is as true in PHP 5.3 as it is in PHP 5.4. If anyone has a configuration setup which triggers a PHP 5.3/PHP 5.4 difference here let me know. Regardless of that, this is a welcome fix to Magento 1.

Update: Andrey Tserkus informed me this change was made to address this bug in PHP related to casting SimpleXML nodes as booleans. It’s an odd bug, depending on the way you navigate/traverse to a particular node, and my initial testing missed it. However, this bug does impact <modules/> nodes without a before or after attribute, regardless of their contents.

<modules>
    <Mage_Paygate>Mage_Paygate_Adminhtml</Mage_Paygate>
</modules>

So it’s extra welcome now.

SimpleXML and Booleans: Part 2

Next up is another change that seems related to boolean values and SimpleXML

-                    <pdo_mysql/>
+                    <pdo_mysql>1</pdo_mysql>

Here, the empty <pdo_mysql/> node is replaced by a node with the value 1. Again though, it’s not clear why this change has been made as (to my knowledge) PHP 5.3 and PHP 5.4 treat these nodes the same. Additionally, the only place this particular node seems to be used is when gathering a list of supported database extensions

#File app/code/core/Mage/Install/Model/Installer/Db/Abstract.php
public function getRequiredExtensions()
{
    $extensions = array();
    $configExt = (array)Mage::getConfig()->getNode(sprintf('install/databases/%s/extensions', $this->getModel()));
    foreach ($configExt as $name=>$value) {
        $extensions[] = $name;
    }
    return $extensions;
}    

This code only uses the XML node name, and not the value. It also behaves identically in current versions of PHP 5.3 and PHP 5.4, so it’s unclear why this change has been made. Further confusing things, the install.xml file uses a similar pattern, but 1‘s have not been added.

<!-- File: app/code/core/Mage/Install/etc/install.xml -->
<extensions>
    <spl/>
    <dom/>
    <simplexml/>
    <mcrypt/>
    <hash/>
    <curl/>
    <iconv/>
    <ctype/>
    <gd/>
</extensions>    

Again, if anyone has the context for this change’s inclusion in the PHP 5.4 patch, I’d love to hear it.

Long time Magento community member Ashley Schroder pointed out this hackathon bug and this Stack Overflow question that point to this change being needed for PHP 5.4 compatibility. The behavior described in those bugs didn’t show up in my testing, so I suspect this was a problem in early versions of PHP 5.4 where XML nodes were incorrectly serialized. If you’ve got more information I’d love to hear it.

Patching Zend Files

The final change in the patch is a little different from the others. It’s the file

app/code/core/Zend/Pdf/FileParserDataSource.php

However — this file doesn’t exist in either a community edition or enterprise edition of Magento. It appears the patch is adding a file that’s never used.

However, there is a FileParserDataSource.php file in

lib/Zend/Pdf/FileParserDataSource.php

Here the patch file is taking advantage of Magento’s code pool feature. Rather than modify the lib/Zend/Pdf/FileParserDataSource.php file in place, the patch drops a new file in app/code/core/Zend/Pdf. Since the core code pool has more priority than the files in lib, Magento will load this new file. This means the system is patched without editing any core or lib files.

If we diff these files, we’ll see the patched file

- /**
-  * Object constructor. Opens the data source for parsing.
-  *
-  * Must set $this->_size to the total size in bytes of the data source.
-  *
-  * Upon return the data source can be interrogated using the primitive
-  * methods described here.
-  *
-  * If the data source cannot be opened for any reason (such as insufficient
-  * permissions, missing file, etc.), will throw an appropriate exception.
-  *
-  * @throws Zend_Pdf_Exception
-  */
- abstract public function __construct();

removes the abstract public function __construct prototype from the Zend class. This appears to be a “fix” for PHP 5.4’s stricter handling of abstract method prototypes. Consider the following PHP code.

abstract class A
{
    abstract public function __construct();
}

class B extends A
{
    public function __construct($foo)
    {

    }
}

This code runs fine in PHP 5.3, but PHP 5.4 has stricter compatibility requirements for abstract function inheritance. The code above would result in an error something like

PHP Fatal error:  Declaration of B::__construct() must be compatible with A::__construct() 

That’s because Class B attempts to add a parameter to the constructor, and PHP 5.4 insists it match the function prototype in he abstract class exactly.

Jumping back to Magento, the following two Zend classes extend the base abstract class defined in FileParserDataSource.php, and both add a parameter to the constructor.

#File: lib/Zend/Pdf/FileParserDataSource/File.php
public function __construct($filePath)
{
}

#File: lib/Zend/Pdf/FileParserDataSource/String.php
public function __construct($string)
{
}

So, the problem here isn’t strictly Magento’s fault — it’s Zend Framework’s. While this problem has been fixed in more recent versions of Zend Framework, upgrading Magento’s version of the framework is far from a drop in task, and it’s easy to see why they chose this simple patch instead. Also, if you think Magento’s solution (dropping the abstract constructor) is a little janky, you may be right, but it’s the same approach Zend Framework took.

Depending on Magento’s approach for the promised CE 1.8.2, if you apply this patch you’ll want to remember a file’s been placed in app/code/core/Zend/Pdf/FileParserDataSource.php. If Magento updates the underlying lib file without removing this patch file, it may create future incompatibles.

Wrap Up

While the release of this patch is an encouraging sign Magento hasn’t abandoned the 1.x branch, it’s clear they’re treating it as a maintenance project. I have to imagine there’s other subtle PHP 5.4 bugs that were never reported to Magento, or that Magento couldn’t reproduce.

Even if these are the only 5.4 bugs — upgrading isn’t for the feint of heart. Almost all Magento production systems rely on some amount of custom code and/or extensions, and vetting these for PHP 5.4 support will be mandatory for anyone upgrading.

Additionally, most hosting companies are on PHP 5.3 and it’s been a historically long slog to get any hosting company to update. Folks less reliant on the specific implementation at a hosting company will be in a better position to make the upgrade themselves, but my instincts say their infrastructure will have MORE dependencies on PHP 5.3.

It’s a classic chicken/egg situation — and in the ROI obsessed world of online retail engineering wait and see is the usual approach. If I was running a Magento consultancy right now, I’d opt for keeping my clients on PHP 5.3, and picking a new project as a guinea pig for a PHP 5.4 rollout. Then, depending on what that project uncovered, I’d start assessing the pros/cons of moving other clients over to PHP 5.4.