New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
temuri416 opened this Issue Apr 8, 2017 · 44 comments

Comments

Projects
None yet
5 participants
@temuri416
Contributor

temuri416 commented Apr 8, 2017

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:

image

Everything is correct.

PHP v7.0.17

The same code results in the following $config value:

image

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

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Apr 8, 2017

Contributor

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

Contributor

temuri416 commented Apr 8, 2017

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

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Apr 20, 2017

Member

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

Member

sergeyklay commented Apr 20, 2017

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

@sergeyklay sergeyklay self-assigned this Apr 20, 2017

@piotrgolasz

This comment has been minimized.

Show comment
Hide comment
@piotrgolasz

piotrgolasz Apr 25, 2017

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;
    }
}

piotrgolasz commented Apr 25, 2017

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

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Apr 25, 2017

Contributor

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

Contributor

temuri416 commented Apr 25, 2017

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

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Apr 25, 2017

Contributor

@sergeyklay

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

Contributor

temuri416 commented Apr 25, 2017

@sergeyklay

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

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Apr 25, 2017

Member

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

Member

sergeyklay commented Apr 25, 2017

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

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Apr 27, 2017

Member

@temuri416 more likely in zephir.

Member

Jurigag commented Apr 27, 2017

@temuri416 more likely in zephir.

@piotrgolasz

This comment has been minimized.

Show comment
Hide comment
@piotrgolasz

piotrgolasz Jun 14, 2017

@sergeyklay any updates with this issue?

piotrgolasz commented Jun 14, 2017

@sergeyklay any updates with this issue?

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Jun 14, 2017

Member

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

Member

sergeyklay commented Jun 14, 2017

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

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Jun 14, 2017

Contributor

@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
Contributor

temuri416 commented Jun 14, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Jun 14, 2017

Member

Powered by Zephir => Version 0.9.7-1fae5e50ac

Could you please use latest Zephir ?

Member

sergeyklay commented Jun 14, 2017

Powered by Zephir => Version 0.9.7-1fae5e50ac

Could you please use latest Zephir ?

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Jun 14, 2017

Contributor

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

Contributor

temuri416 commented Jun 14, 2017

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

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jun 14, 2017

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

Jurigag commented Jun 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Jun 14, 2017

Contributor

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
Contributor

temuri416 commented Jun 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Jun 14, 2017

Member

Provide please ini config example

Member

sergeyklay commented Jun 14, 2017

Provide please ini config example

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Jun 14, 2017

Contributor

You mean php.ini config?

Contributor

temuri416 commented Jun 14, 2017

You mean php.ini config?

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jun 14, 2017

Member
$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?

Member

Jurigag commented Jun 14, 2017

$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?

@sergeyklay sergeyklay closed this Jun 14, 2017

@sergeyklay sergeyklay reopened this Jun 14, 2017

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Jun 14, 2017

Contributor

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

Contributor

temuri416 commented Jun 14, 2017

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

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jun 14, 2017

Member

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

Member

Jurigag commented Jun 14, 2017

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

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Jun 14, 2017

Contributor

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.

Contributor

temuri416 commented Jun 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jun 14, 2017

Member

You sure that you had the same combo?

Member

Jurigag commented Jun 14, 2017

You sure that you had the same combo?

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Jun 14, 2017

Member

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

Member

sergeyklay commented Jun 14, 2017

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

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Jun 14, 2017

Contributor

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,

Contributor

temuri416 commented Jun 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Jun 14, 2017

Contributor

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

Contributor

temuri416 commented Jun 14, 2017

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

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Jun 14, 2017

Contributor

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

Contributor

temuri416 commented Jun 14, 2017

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

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jun 14, 2017

Member

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.

Member

Jurigag commented Jun 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jun 14, 2017

Member

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

Member

Jurigag commented Jun 14, 2017

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

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jun 14, 2017

Member

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

Member

Jurigag commented Jun 14, 2017

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

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jun 14, 2017

Member

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.

Member

Jurigag commented Jun 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jun 14, 2017

Member

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} {
Member

Jurigag commented Jun 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jun 14, 2017

Member

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.

Member

Jurigag commented Jun 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
Member

Jurigag commented Jun 14, 2017

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Jun 14, 2017

Contributor

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

Contributor

temuri416 commented Jun 14, 2017

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

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jun 14, 2017

Member

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

Member

Jurigag commented Jun 14, 2017

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 comment has been minimized.

Show comment
Hide comment
Contributor

temuri416 commented Jun 14, 2017

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jun 14, 2017

Member

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

Member

Jurigag commented Jun 14, 2017

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

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Jun 14, 2017

Contributor

F**k.

Contributor

temuri416 commented Jun 14, 2017

F**k.

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jun 14, 2017

Member

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

Member

Jurigag commented Jun 14, 2017

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

@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jun 14, 2017

Member

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.

Member

Jurigag commented Jun 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jun 14, 2017

Member

What is more stupid:

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

Jurigag commented Jun 14, 2017

What is more stupid:

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

This comment has been minimized.

Show comment
Hide comment
@andrew-demb

andrew-demb Jun 15, 2017

@Jurigag, just PHP syntax

$object->{'1'}

andrew-demb commented Jun 15, 2017

@Jurigag, just PHP syntax

$object->{'1'}
@Jurigag

This comment has been minimized.

Show comment
Hide comment
@Jurigag

Jurigag Jun 15, 2017

Member

Actually $object->{1} works too

Member

Jurigag commented Jun 15, 2017

Actually $object->{1} works too

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

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

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

@Jurigag Jurigag referenced this issue Jun 19, 2017

Merged

Fixes #12779 #12910

3 of 3 tasks complete

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

@sergeyklay

This comment has been minimized.

Show comment
Hide comment
@sergeyklay

sergeyklay Jun 19, 2017

Member

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

Member

sergeyklay commented Jun 19, 2017

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

@sergeyklay sergeyklay closed this Jun 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment