[BUG]: Phalcon\Config::merge change in 4.x

[dmarkic]

Phalcon\Config::merge() now works, in some parts, for integer keys as PHP's array_merge. Integer keys do not get overwritten, but integer keys also do not get reindexed/renumbered.
The behavior is even stranger when we already have some numeric key inside, before merge, as keys get appended/incremented. Some examples below:

Script to reproduce:

$cfg = new \Phalcon\Config(['test' => 123, 'empty' => [], 'nonEmpty' => [5 => 'test']]);

echo " - original -\n";
print_r($cfg->toArray());

$emptyMerge = new \Phalcon\Config([3 => 'toEmpty']);
$nonEmptyMerge = new \Phalcon\Config([3 => 'toNonEmpty']);

$cfg->empty->merge($emptyMerge);
$cfg->nonEmpty->merge($nonEmptyMerge);

echo " - after merge -\n";
print_r($cfg->toArray());

Output (as expected) on Phalcon 3.x:

 - original -
Array
(
    [test] => 123
    [empty] => Array
        (
        )

    [nonEmpty] => Array
        (
            [5] => test
        )

)
 - after merge -
Array
(
    [test] => 123
    [empty] => Array
        (
            [3] => toEmpty
        )

    [nonEmpty] => Array
        (
            [5] => test
            [3] => toNonEmpty
        )

)

Everything is OK. empty has received 3 => toEmpty and nonEmpty has received 3 => toNonEmpty.

Output on Phalcon 4.x is completely different:

 - original -
Array
(
    [test] => 123
    [empty] => Array
        (
        )

    [nonEmpty] => Array
        (
            [5] => test
        )

)
 - after merge -
Array
(
    [test] => 123
    [empty] => Array
        (
            [0] => toEmpty
        )

    [nonEmpty] => Array
        (
            [5] => test
            [6] => toNonEmpty
        )

)

Here we see that empty receives key [0]. And nonEmpty gets appended to next key.

Expected behavior
I think Phalcon 3.x is the expected behavior, since this is configuration and keys may be important. Maybe add argument to Config::merge to overwrite or reindex keys on merge?

I didn't tested it, but simply removing elseif in https://github.com/phalcon/cphalcon/blob/master/phalcon/Config.zep#L191 would probably return to 3.x behavior.

If you'd like to move back to 3.x behavior I can try and change Config.zep and make PR.

Details

• Phalcon version: 3.4.5 and 4.0.0
• PHP Version: PHP 7.3.11-1~deb10u1
• Operating System: Linux Debian
• Installation type: installing via package manager

[ruudboon]

I think this issue needs some discussion. Personally I would like to align with how PHP is handeling this. Values in the input arrays with numeric keys will be renumbered with incrementing keys starting from zero in the result array.

http://sandbox.onlinephpfunctions.com/code/e79296edb95931346f14fb2155ae70965d586516

Array
(
    [test] => 123
    [empty] => Array
        (
            [0] => toEmpty
        )

    [nonEmpty] => Array
        (
            [0] => test
            [1] => toNonEmpty
        )

)

https://www.php.net/manual/en/function.array-merge.php
@niden @Jeckerson who do you guys think?

[Jeckerson]

I think current condition need to be here, to make sure that key is not overridden:

cphalcon/phalcon/Config.zep

Line 191 in b28b0a2

} elseif typeof key === "int" {

If in 4.0 approach was shifted to make same approach as PHP function array_merge(), even not intentionally, I prefer to stick with PHP's approach.

But, we need to update the docs with such examples as above.

[niden]

I agree with @Jeckerson - the closer we are to PHP's native methods the better it is. It makes it easier for developers to use it. I would venture to say even that merge should be a clone of array_merge or array_replace

[gpkappos]

But this is about config so numeric keys might need to remain as they are.

For example in PDO, the constructor options are key value pairs like
[ PDO::ATTR_PERSISTENT => true ]
where PDO::ATTR_PERSISTENT is an integer.
Also PDO options is something that is expected to be kept in a config object.

[gpkappos]

Hello,
is there a decision on the behaviour of config merge?

[Jeckerson]

@gpkappos This behavior will be fixed in next release or some.

[niden]

Resolved in #15723

[lcdennison]

@niden I believe the numerical-index merging is still a problem. Consider this test case, running on 5.0.0-beta1:

$array1 = ['first'];
$array2 = ['second', 'third'];

print_r((new Phalcon\Config\Config())
	->merge($array1)
	->merge($array2)
	->toArray()
);

gives:

Array
(
    [0] => second
    [1] => third
)

Even when I nested these arrays within a string-indexed child array, I got the same result for the merged element. Elements are always overwritten by their numerical indexes.

Based on the comments above, I thought the intention was to make the result like array_merge(), which applied to the same $array1 and $array2 above gives:

Array
(
    [0] => first
    [1] => second
    [2] => third
)

Did I just misunderstand the intended result from this fix, or is there still something to be adjusted here?