Invalid logic of Phalcon\Config::merge()

[zatrepalek]

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]

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]

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

Unfortunately, I not from the Phalcon team.
@CameronHall

[CameronHall]

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

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.

[CameronHall]

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 zephir-lang/zephir#1818 is fixed as Zephir thinks that objects that inherit ArrayAccess which are subsequently Traversable are not iterable :|

[CameronHall]

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

[niden]

Resolved in #14186