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

temuri416 · 2017-04-08T14:02:44Z

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

The text was updated successfully, but these errors were encountered:

temuri416 · 2017-04-08T21:51:14Z

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

sergeyklay · 2017-04-20T21:34:10Z

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

piotrbaczek · 2017-04-25T07:08:22Z

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 · 2017-04-25T13:15:22Z

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

temuri416 · 2017-04-25T13:18:32Z

@sergeyklay

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

sergeyklay · 2017-04-25T22:17:52Z

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

Jurigag · 2017-04-27T15:02:07Z

@temuri416 more likely in zephir.

piotrbaczek · 2017-06-14T12:18:17Z

@sergeyklay any updates with this issue?

sergeyklay · 2017-06-14T14:08:32Z

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

temuri416 · 2017-06-14T18:44:06Z

@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 · 2017-06-14T21:14:29Z

Powered by Zephir => Version 0.9.7-1fae5e50ac

Could you please use latest Zephir ?

temuri416 · 2017-06-14T21:27:29Z

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

Jurigag · 2017-06-14T21:39:41Z

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

temuri416 · 2017-06-14T21:48:52Z

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 · 2017-06-14T22:01:31Z

Provide please ini config example

temuri416 · 2017-06-14T22:03:05Z

You mean php.ini config?

Jurigag · 2017-06-14T22:04:42Z

$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 · 2017-06-14T22:09:31Z

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

Jurigag · 2017-06-14T22:10:17Z

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

temuri416 · 2017-06-14T22:12:47Z

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 · 2017-06-14T22:13:50Z

You sure that you had the same combo?

sergeyklay · 2017-06-14T22:15:48Z

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

temuri416 · 2017-06-14T22:15:57Z

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 · 2017-06-14T22:17:00Z

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

sergeyklay · 2017-06-14T22:18:48Z

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

temuri416 · 2017-06-14T22:20:21Z

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

Jurigag · 2017-06-14T22:23:36Z

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 · 2017-06-14T22:32:59Z

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

Jurigag · 2017-06-14T22:51:39Z

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

Jurigag · 2017-06-14T22:53:01Z

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 · 2017-06-14T22:55:54Z

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 · 2017-06-14T23:06:34Z

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 · 2017-06-14T23:07:00Z

Check this out:

https://3v4l.org/TqueM

@sergeyklay @temuri416 @piotrgolasz

WTF PHP

temuri416 · 2017-06-14T23:11:23Z

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

Jurigag · 2017-06-14T23:12:14Z

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 · 2017-06-14T23:12:47Z

This?

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

Jurigag · 2017-06-14T23:13:48Z

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

temuri416 · 2017-06-14T23:14:04Z

F**k.

Jurigag · 2017-06-14T23:14:56Z

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

Jurigag · 2017-06-14T23:17:52Z

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 · 2017-06-14T23:47:03Z

What is more stupid:

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

andrew-demb · 2017-06-15T14:36:03Z

@Jurigag, just PHP syntax

$object->{'1'}

Jurigag · 2017-06-15T16:59:04Z

Actually $object->{1} works too

Fixes #12779

sergeyklay · 2017-06-19T10:38:55Z

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

sergeyklay added the Bug - Medium label Apr 20, 2017

sergeyklay self-assigned this Apr 20, 2017

sergeyklay closed this as completed Jun 14, 2017

sergeyklay reopened this Jun 14, 2017

Jurigag added a commit to Jurigag/cphalcon that referenced this issue Jun 19, 2017

Fixes phalcon#12779

ccab530

Jurigag added a commit to Jurigag/cphalcon that referenced this issue Jun 19, 2017

Fixes phalcon#12779

25697f3

Jurigag added a commit to Jurigag/cphalcon that referenced this issue Jun 19, 2017

Fixes phalcon#12779

26f4883

Jurigag mentioned this issue Jun 19, 2017

Fixes #12779 #12910

Merged

3 tasks

sergeyklay added a commit that referenced this issue Jun 19, 2017

Merge pull request #12910 from Jurigag/3.2.x-12779

26fa39e

Fixes #12779

sergeyklay closed this as completed Jun 19, 2017

niden added bug A bug report status: medium Medium and removed Bug - Medium labels Dec 23, 2019

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

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

Comments

temuri416 commented Apr 8, 2017

PHP v7.0.14

Expected and Actual Behavior

PHP v7.0.17

Details

temuri416 commented Apr 8, 2017

sergeyklay commented Apr 20, 2017

piotrbaczek commented Apr 25, 2017 • edited Loading

temuri416 commented Apr 25, 2017

temuri416 commented Apr 25, 2017

sergeyklay commented Apr 25, 2017 • edited Loading

Jurigag commented Apr 27, 2017

piotrbaczek commented Jun 14, 2017

sergeyklay commented Jun 14, 2017

temuri416 commented Jun 14, 2017

sergeyklay commented Jun 14, 2017

temuri416 commented Jun 14, 2017

Jurigag commented Jun 14, 2017

temuri416 commented Jun 14, 2017

sergeyklay commented Jun 14, 2017

temuri416 commented Jun 14, 2017

Jurigag commented Jun 14, 2017 • edited Loading

temuri416 commented Jun 14, 2017

Jurigag commented Jun 14, 2017

temuri416 commented Jun 14, 2017

Jurigag commented Jun 14, 2017

sergeyklay commented Jun 14, 2017

temuri416 commented Jun 14, 2017 • edited Loading

temuri416 commented Jun 14, 2017

sergeyklay commented Jun 14, 2017

temuri416 commented Jun 14, 2017

Jurigag commented Jun 14, 2017

Jurigag commented Jun 14, 2017

Jurigag commented Jun 14, 2017

Jurigag commented Jun 14, 2017

Jurigag commented Jun 14, 2017

Jurigag commented Jun 14, 2017

Jurigag commented Jun 14, 2017

temuri416 commented Jun 14, 2017

Jurigag commented Jun 14, 2017

temuri416 commented Jun 14, 2017

Jurigag commented Jun 14, 2017

temuri416 commented Jun 14, 2017

Jurigag commented Jun 14, 2017 • edited Loading

Jurigag commented Jun 14, 2017 • edited Loading

Jurigag commented Jun 14, 2017

andrew-demb commented Jun 15, 2017

Jurigag commented Jun 15, 2017

sergeyklay commented Jun 19, 2017

piotrbaczek commented Apr 25, 2017 •

edited

Loading

sergeyklay commented Apr 25, 2017 •

edited

Loading

Jurigag commented Jun 14, 2017 •

edited

Loading

temuri416 commented Jun 14, 2017 •

edited

Loading

Jurigag commented Jun 14, 2017 •

edited

Loading

Jurigag commented Jun 14, 2017 •

edited

Loading