PHP 7.2 Support

[ghost]

I'm using mage-lts on PHP 7.2 (brave, I know).

Anyway, perhaps we should merge these changes: https://github.com/mklooss/Loewenstark_Php72

It should not affect anything negatively, since it's just adding parameters to a few methods that aren't compatible with the parent class.

[LeeSaferite]

If you made a PR, I'm sure we could get it merged.

[Quazz]

Discussion about this (primarily about mcrypt being deprecated on PHP 7.1) on Inchoo_PHP7 module:

Inchoo/Inchoo_PHP7#98

Looks like they are working on a compatible OpenSSL adapter to replace mcrypt, which seems like a better solution, imo. They have an early version available on their development branch.

Since Inchoo's solution is already integrated in this project, updating it seems like the superior solution here.

Just my 2 cents.

[Flyingmana]

adding https://github.com/phpseclib/mcrypt_compat should be enough for now I think?

[ghost]

adding https://github.com/phpseclib/mcrypt_compat should be enough for now I think?

Can confirm. With the PR I made, and this mcrypt polyfill I'm running on 7.2 with no problems yet. Obviously replacing mcrypt as has been talked about would be a better solution, the polyfill works fine for now.

[colinmollenhour]

The polyfill referenced uses mcrypt and falls back to raw php so may be a small performance hit (depending on how much encryption is used, which is probably very little).

OpenSSL can also be used to replace mcrypt as a compatible drop-in.

<?php
/**
 * Magento
 *
 * NOTICE OF LICENSE
 *
 * This source file is subject to the Open Software License (OSL 3.0)
 * that is bundled with this package in the file LICENSE.txt.
 * It is also available through the world-wide-web at this URL:
 * http://opensource.org/licenses/osl-3.0.php
 * If you did not receive a copy of the license and are unable to
 * obtain it through the world-wide-web, please send an email
 * to license@magentocommerce.com so we can send you a copy immediately.
 *
 * DISCLAIMER
 *
 * Do not edit or add to this file if you wish to upgrade Magento to newer
 * versions in the future. If you wish to customize Magento for your
 * needs please refer to http://www.magentocommerce.com for more information.
 *
 * @category   Varien
 * @package    Varien_Crypt
 * @license    http://opensource.org/licenses/osl-3.0.php  Open Software License (OSL 3.0)
 */


/**
 * Openssl plugin
 *
 * @category   Varien
 * @package    Varien_Crypt
 */
class Varien_Crypt_Openssl extends Varien_Crypt_Abstract
{

    private $_key;

    /**
     * Initialize encryption module
     *
     * @param string $key cipher private key
     * @return $this
     */
    public function init($key)
    {
        $this->_key = $key;

        return $this;
    }

    /**
     * Encrypt data
     *
     * @param string $data source string
     * @return string
     */
    public function encrypt($data)
    {
        return openssl_encrypt($data, 'bf-ecb', $this->_key, OPENSSL_RAW_DATA);
    }

    /**
     * Decrypt data
     *
     * @param string $data encrypted string
     * @return string
     */
    public function decrypt($data)
    {
        return openssl_decrypt($data, 'bf-ecb', $this->_key, OPENSSL_RAW_DATA | OPENSSL_ZERO_PADDING);
    }

}

This was tested like so:

    $key = 'A1ZppiL7hNAGo4J4KD8CqduTo2nr9LVDqQwc9BpzayU7yBPQmfje';
    $mcrypt = new Varien_Crypt_Mcrypt();
    $openssl = new Varien_Crypt_Openssl();
    $mcrypt->init($key);
    $openssl->init($key);
    $data = 'foobar123foobar123foobar123foobar123foobar123';
    $mData = $mcrypt->encrypt($data);
    $oData = $openssl->encrypt($data);
    var_dump(base64_encode($mData));
    var_dump(base64_encode($oData));
    var_dump($mcrypt->decrypt($mData));
    var_dump($mcrypt->decrypt($oData));
    var_dump($openssl->decrypt($mData));
    var_dump($openssl->decrypt($oData));

With result:

string(64) "B3Uay314GboVPZddN4tqYGyS0OpcJeSBlKBPvLeqVn1dtsogXckM3wJiwhybAsIJ"
string(64) "B3Uay314GboVPZddN4tqYGyS0OpcJeSBlKBPvLeqVn1dtsogXckM33WaPl8rPgp5"
string(48) "foobar123foobar123foobar123foobar123foobar123\000\000\000"
string(48) "foobar123foobar123foobar123foobar123foobar123���"
string(48) "foobar123foobar123foobar123foobar123foobar123\000\000\000"
string(48) "foobar123foobar123foobar123foobar123foobar123���"

Note, the padding at the end is stripped by the Mage_Core_Model_Encryption helper so is not an issue.

I tested the above with 10,000 iterations and mcrypt was faster than Openssl by quite a bit but again, unless encryption is used unusually heavily for some reason it probably won't make a difference:

mcrypt: 0.06453 seconds
openssl: 0.34680 seconds

I haven't tested the performance of the polyfill yet.

However, ideally it would be possible to re-encrypt all data and start using sodium with modern ciphers.

[clotted]

Quick question. Just want to confirm that adding https://github.com/phpseclib/mcrypt_compat
via composer with zero code changes is sufficient for full PHP 7.2 support.

I found this article which details a change as per 'Step 2' : https://df.tips/t/topic/281
Is that required?

and is the api.php impact accounted for? (as per comment in linked article)

[Flyingmana]

@clotted from what I have seen the only PHP 7.2 problem left seems to be the mcrypt issue. At least the Inchoo module has no other fix added yet.

So adding the mcrypt_compat should be enough to be compatible.
You will still need to add the include by yourself and I know the problem, that the index.php is not enough. I worked on this problem because of the magento composer installer. The best place to put such an include is the Mage.php file (before the class starts), its the earliest point which is always executed, independent of the used entry point. (they for example also missed the cron.php in the linked article).

@colinmollenhour drop-in only when you dont have existing data which need to get decrypted (like encrypted config fields)
But in general using openssl as a replacement should be great.

[clotted]

Thanks @Flyingmana
I guess it would be possible to add the include for mcrypt_compat by modifying Cotya/Magento-composer-installer (in the /Patcher/Bootstrap.php file) which gracefully adds it's include to Mage.php
Eg. tack it on the end.

[colinmollenhour]

I just pushed a PR with the Varien_Crypt_Openssl replacement. Not heavily tested. (#506)

[googlygoo]

One issue that seems to still be around in 7.2 is Zend/Locale/Math/PhpMath is not 100% compatible with bcmath. For example for Sub is the second parameter php7.2 will throw a warning. This is only noticeable if bcmath is not installed on your server. If it is the class is not used.

On this error, and on the the discussion #416 and #105 of bring Magento 1 forward I spent some time updating the Zend library, validating it against php 7.2 and small fixes. You can view my work magento-lts/tree/issue-449-php7.2.

I have not included this in any pull request as I first want to know if this is something that this project is interested if so do you happy with a big commit like this or smaller chunks.

One thing I have noted is my implementation of fixing mcrypt is probably not going to be accepted as it diverges a lot from the above PR'ed solution.

[colinmollenhour]

I propose that bcmath simply be made a required extension.

[Flyingmana]

I would second that. bcmath is easy to install, reduces the need for maintenance a bit and has a much higher userbase

[tmotyl]

+1 make it required. It's required in M2 too AFAIK.

[Flyingmana]

solved with official PHP 7.2 patch from Magento