Cphalcon: [BUG, v.3.1.2] Inconsistent behaviour of Phalcon\Config::merge() across minor versions of PHP7

[temuri416]

Phalcon\Config::merge() of the same version of Phalcon (v3.1.2) behaves differently on PHP versions 7.0.14 and 7.0.17.

PHP v7.0.14

$config = new Phalcon\Config([
    'a' => [
        [
            1
        ]
    ],
]);

$config->merge(new Phalcon\Config([
    'a' => [
        [
            2
        ]
    ],
]));

Expected and Actual Behavior

$config is:

Everything is correct.

PHP v7.0.17

The same code results in the following $config value:

I expect $config to have the same value that's produced under v7.0.14.

Details

• Phalcon version:

Version => 3.1.2
Build Date => Apr  6 2017 21:22:10
Powered by Zephir => Version 0.9.7-1fae5e50ac

• PHP Version: 7.0.14 vs 7.0.17
• Operating System: Ubuntu 16.04 64bit
• Installation type: Compiling from source
• Zephir version (if any): 0.9.7
• Server: Nginx

[temuri416]

Phalcon 3.2.x with PHP7.1.3 behaves exactly as Phalcon 3.1.2 & PHP7.0.17 (i.e. is broken).

[sergeyklay]

I'll try to sort out. Thank you for pointing out

[piotrbaczek]

This "feature" broke our dev environment. We also found that the function works incorrectly when config keys are numerical, adding a letter to a key makes it work as intended.
The temporary solution is to overwrite the merge method in PHP:

class MyConfig Extends Phalcon\Config
{
    function __merge($config, $instance = null)
    {
        if (gettype($instance) !== 'object') {
            $instance = $this;
        }
        $number = $this->count($instance);

        foreach (get_object_vars($config) as $key => $value) {
            if ($localObject = $instance->{$key}) {
                if (gettype($localObject) === 'object' && gettype($value) === 'object') {
                    if ($localObject instanceof Phalcon\Config && $value instanceof Phalcon\Config) {
                        $this->__merge($value, $localObject);
                        continue;
                    }
                }
            }
            if (is_numeric($key)) {
                $key = strval($number);
                $number++;
            }
            $instance->{$key} = $value;
        }
        return $instance;
    }
}

[temuri416]

@piotrgolasz what makes you think it's a "feature"? hope you're being sarcastic :)

[temuri416]

@sergeyklay

Do you know why this is happening? How could change in PHP minor version affect this, or is it something in Phalcon?

[sergeyklay]

@temuri416, @piotrgolasz Could you please try to compile Phalcon from master (v3.1.2) by using ff769131f3751b590da702e3c7595411c07b0919 Zephir's commit?

[Jurigag]

@temuri416 more likely in zephir.

[piotrbaczek]

@sergeyklay any updates with this issue?

[sergeyklay]

@piotrgolasz @temuri416 Could you please test by using latest Zephir from master branch and Phalcon 3.2.x ?

[temuri416]

@sergeyklay

Looks like it's broken with PHP 7.1.5, with latest Phalcon 3.2.x:

Running the snipped above I am getting this:

Phalcon\Config Object
(
    [a] => Phalcon\Config Object
        (
            [0] => Phalcon\Config Object
                (
                    [0] => 1
                )

            [1] => Phalcon\Config Object
                (
                    [0] => 2
                )

        )

)

Phalcon info:

Web framework delivered as a C-extension for PHP
phalcon => enabled
Author => Phalcon Team and contributors
Version => 3.2.0a
Build Date => Jun 14 2017 14:38:02
Powered by Zephir => Version 0.9.7-1fae5e50ac

Directive => Local Value => Master Value
phalcon.db.escape_identifiers => On => On
phalcon.db.force_casting => Off => Off
phalcon.orm.events => On => On
phalcon.orm.virtual_foreign_keys => On => On
phalcon.orm.column_renaming => On => On
phalcon.orm.not_null_validations => On => On
phalcon.orm.exception_on_failed_save => Off => Off
phalcon.orm.enable_literals => On => On
phalcon.orm.late_state_binding => Off => Off
phalcon.orm.enable_implicit_joins => On => On
phalcon.orm.cast_on_hydrate => Off => Off
phalcon.orm.ignore_unknown_columns => Off => Off

[sergeyklay]

Powered by Zephir => Version 0.9.7-1fae5e50ac

Could you please use latest Zephir ?

[temuri416]

Sure... where can I see the instructions on how to compile Phalcon with externally compiled Zephir?

[Jurigag]

1. git clone https://github.com/phalcon/zephir
2. cd zephir && ./install -c
3. go to phalcon repo
4. zephir build

[temuri416]

Same result:

Phalcon\Config Object
(
    [a] => Phalcon\Config Object
        (
            [0] => Phalcon\Config Object
                (
                    [0] => 1
                )

            [1] => Phalcon\Config Object
                (
                    [0] => 2
                )

        )

)

Web framework delivered as a C-extension for PHP
phalcon => enabled
Author => Phalcon Team and contributors
Version => 3.2.0a
Build Date => Jun 14 2017 17:43:44
Powered by Zephir => Version 0.9.8-096365f70b

Directive => Local Value => Master Value
phalcon.db.escape_identifiers => On => On
phalcon.db.force_casting => Off => Off
phalcon.orm.events => On => On
phalcon.orm.virtual_foreign_keys => On => On
phalcon.orm.column_renaming => On => On
phalcon.orm.not_null_validations => On => On
phalcon.orm.exception_on_failed_save => Off => Off
phalcon.orm.enable_literals => On => On
phalcon.orm.late_state_binding => Off => Off
phalcon.orm.enable_implicit_joins => On => On
phalcon.orm.cast_on_hydrate => Off => Off
phalcon.orm.ignore_unknown_columns => Off => Off
phalcon.orm.update_snapshot_on_save => On => On
phalcon.orm.disable_assign_setters => Off => Off

[sergeyklay]

Provide please ini config example

[temuri416]

You mean php.ini config?

[Jurigag]

$config = new Phalcon\Config([
    'a' => [
        [
            1
        ]
    ],
]);

$config->merge(new Phalcon\Config([
    'a' => [
        [
            2
        ]
    ],
]));

Your result is correct. You have TWO nested arrays. If you want to have returned a key with 1 and 2 value then you need to change your code to:

$config = new Phalcon\Config([
    'a' => [
            1
    ]
]);

$config->merge(new Phalcon\Config([
    'a' => [
            2
    ]
]));

Not sure why you have on php 7.0.14 diffrent result, maybe you were using outdated phalcon?

[temuri416]

Are you saying that it was working incorrectly before and now it is correct?

[Jurigag]

Well actually im not sure, well i need to test it.

[temuri416]

I don't care anymore which way is right - what was before or what's now. I don't need that type of merging anymore in my config (implemented it differently).

However, what's bugging me is that it's THE SAME combo of Phalcon & Zephir that behaves differently across minor PHP versions.

[Jurigag]

You sure that you had the same combo?

[sergeyklay]

@temuri416 Could you please provide a small unit test to test it on Travis with PHP 5.6 - 7.1 ?

[temuri416]

100%. Checked that several times, could not believe my eyes.

You can also ask @piotrgolasz.

The fact is, behaviour of Phalcon\Config::merge() has changed. And no one seem to understand why,

[temuri416]

ok, but I never worked with that. Link for more info?

[sergeyklay]

Just add new test here https://github.com/phalcon/cphalcon/blob/3.2.x/tests/unit/Config/Adapter/IniTest.php

[temuri416]

ok, I'll try to do it this weekend.

[Jurigag]

I just think that result on php 7.0.17 is correct, not sure why you had returned 7.0.14, from logical point of view what i wrote above should be true.

[Jurigag]

Well i will need to test it, though from conditions i see there it should merge, for some reason it doesn't.

[Jurigag]

Well it looks like that we need to cast key to string most likely.

[Jurigag]

Okay so on first _merge run it properly go into "a" key of both configs. Then we have 0 key. This if fetch localObject, instance->{key} { just doesn't return anything, that's why it breaks. I guess we need to cast key.

[Jurigag]

Just code must be updated to something like:

        for key, value in get_object_vars(config) {
            let property = (string)key;
            if fetch localObject, instance->{property} {

[Jurigag]

Yea i can confirm this is an fix, already tested @sergeyklay @temuri416 @piotrgolasz

Actually it's like this even in plain php, so im not sure how your code fixed it.

[Jurigag]

Check this out:

https://3v4l.org/TqueM

@sergeyklay @temuri416 @piotrgolasz

WTF PHP

[temuri416]

So it seems before v7.0.17 numeric keys were cast as strings

[Jurigag]

Yea and in 7.1.0-7.1.2 they went back to castings as ints and then go back to casting as string xD wtf

[temuri416]

This?

https://bugs.php.net/bug.php?id=73998

[Jurigag]

Well i guess just 7.1.3 was released same time as 7.0.17, right?

[temuri416]

F**k.

[Jurigag]

Yea exactly, it's about release date, though it's pretty funny.

[Jurigag]

Still - there is just a bug in phalcon, and it was working because php was returning wrong value :D :D

So bug in php was fixing bug in phalcon, funny.

[Jurigag]

What is more stupid:

$key = 1;
$object->$key; // works
$object->1; // syntax error

[andrew-demb]

@Jurigag, just PHP syntax

$object->{'1'}

[Jurigag]

Actually $object->{1} works too

[sergeyklay]

Fixed in the 3.2.x branch. Feel free to open new issue if the problem appears again. Thank you for contributing.