Skip to content
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

Invalid logic of Phalcon\Config::merge() #13768

Closed
zatrepalek opened this issue Jan 17, 2019 · 8 comments

Comments

@zatrepalek
Copy link

commented Jan 17, 2019

Hi, I found this bug. Probably related to #13201 (stale - closed).

Expected and Actual Behavior

If an array is passed to merge() method string (number-like) keys are not merged correctly

Correct behaviour (for string-like keys):

$params1 = ['a' => 'aaa', 'b' => ['bar' => 'rrr', 'baz' => 'zzz']];
$config = new \Phalcon\Config($params1);
$config->toArray()
// output
array (
  'a' => 'aaa',
  'b' => 
  array (
    'bar' => 'rrr',
    'baz' => 'zzz',
  ),
)

$params2 = ['c' => 'ccc', 'b' => ['baz' => 'xxx']];
$config->merge(new \Phalcon\Config($params2));
$config->toArray()
// correct merge
array (
  'a' => 'aaa',
  'b' => 
  array (
    'bar' => 'rrr',
    'baz' => 'xxx',
  ),
  'c' => 'ccc',
)

Wrong behaviour (for integer-like keys):

$params1 = ['1' => 'aaa', '2' => ['11' => 'rrr', '12' => 'zzz']];
$config = new \Phalcon\Config($params1);
$config->toArray()
// output (string keys casted to integers)
array (
  1 => 'aaa',
  2 => 
  array (
    11 => 'rrr',
    12 => 'zzz',
  ),
)

$params2 = ['3' => 'ccc', '2' => ['12' => 'xxx']];
$config->merge(new \Phalcon\Config($params2));
$config->toArray()
// wrong merge (casted to integers and invalid merge logic for sub array)
array (
  1 => 'aaa',
  2 => 
  array (
    11 => 'rrr',
    12 => 'zzz',
    2 => 'xxx',
  ),
  3 => 'ccc',
)

Details

  • Phalcon version: 3.4.2
  • PHP Version: 7.1.25
  • Operating System: macOS 10.13.6 (host), Debian 9 (docker)
  • Installation type: Compiling from source (build/php7/64bits)
  • Zephir version: (Version 0.10.14-975ad02db4)
  • Server: Nginx
@TimurFlush

This comment has been minimized.

Copy link

commented Jan 18, 2019

It seems Phalcon Team wanted the behavior of merging mechanism as in the function array_merge().

You are must see http://php.net/manual/en/function.array-merge.php:

If the input arrays have the same string keys, then the later value for that key will overwrite the previous one. If, however, the arrays contain numeric keys, the later value will not overwrite the original value, but will be appended.

But, it all my opinion.

@CameronHall

This comment has been minimized.

Copy link
Member

commented Jan 20, 2019

@TimurFlush that is correct. It looks like the problem lies here with the is_numeric check. https://github.com/phalcon/cphalcon/blob/4.0.x/phalcon/config.zep#L311

The check is too loose. Should we say, is_numeric(key) && typeof key !== string so we get floats as well? Or, do we only allow integers?

@TimurFlush

This comment has been minimized.

Copy link

commented Jan 20, 2019

Unfortunately, I not from the Phalcon team.
@CameronHall

@CameronHall

This comment has been minimized.

Copy link
Member

commented Jan 20, 2019

@niden can you please answer this?

is_numeric(key) && typeof key !== string so we get floats as well? Or, do we only allow integers?

@niden

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

typeof key !== "string" will give you everything that is not string. So that will not work; it will bring side effects. As per discord discussion is_int and is_float should be ok.

@niden niden added this to To do in 4.0 Release via automation Feb 1, 2019

@niden niden added the Bug - Low label Feb 1, 2019

@niden niden moved this from To do to In progress in 4.0 Release Feb 2, 2019

@CameronHall

This comment was marked as outdated.

Copy link
Member

commented Feb 21, 2019

I swapped from get_object_vars when iterating over the object, to just the iterating over the object it self. It appears that ArrayAccess casts numeric keys to integers when retrieving the data via get_object_vars.

I got a pure PHP implementation working. But it seems, l've hit a road block with this one when converting it to Zephir. We will have to wait until phalcon/zephir#1818 is fixed as Zephir thinks that objects that inherit ArrayAccess which are subsequently Traversable are not iterable :|

@CameronHall

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

This should be fixed now. I'll see how I go and add some tests for this.

@niden

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Resolved in #14186

@niden niden closed this Jun 20, 2019

4.0 Release automation moved this from In progress to Done Jun 20, 2019

@niden niden added the 4.0 label Jun 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.