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

Phalcon\Config->merge() sometimes deletes the last key/value of first config #13351

Closed
VCJeroen opened this Issue Apr 19, 2018 · 2 comments

Comments

Projects
None yet
5 participants
@VCJeroen

VCJeroen commented Apr 19, 2018

Expected and Actual Behavior

I am trying to merge two configs. This works nice when all keys in the config are strings, but produces unexpected results when all keys are integers or numeric strings. Simplest example to reproduce I could come up with:

use Phalcon\Config;
$config = new Config([1 => 'Apple']);
$config2 = new Config([2 => 'Banana']);
$config->merge($config2);
print_r($config);

Expected behaviour:

Phalcon\Config Object
(
    [1] => Apple
    [2] => Banana
)

Actual behaviour:

Phalcon\Config Object
(
    [1] => Banana
)

Several examples:

// Same code as above but array starting at 0; this merges as expected
$config = new Config([0 => 'Apple']);
$config2 = new Config([1 => 'Banana']);

// Any *non-numeric* string as key in the arrays being merged; this merges as expected
$config = new Config([1 => 'Apple', 'p' => 'Pineapple']);
$config2 = new Config([2 => 'Banana']);

// To illustrate nested configs: "One" will merge as expected, "Two" will not
$config  = new Config([
    'One' => [1 => 'Apple', 'p' => 'Pineapple'],
    'Two' => [1 => 'Apple'],
]);
$config2 = new Config([
    'One' => [2 => 'Banana'],
    'Two' => [2 => 'Banana'],
]);

Details

I've reproduced this behaviour on 3 different servers:

PHP 7.1.12, Phalcon 3.2.0, Zephir 0.9.8-6335775f25
PHP 7.1.8, Phalcon 3.2.1, Zephir 0.9.9-868cb1f92b
PHP 7.2.3, Phalcon 3.3.2, Zephir 0.10.7-2917ebe8ae

  • Operating System: Ubuntu 7.2.0 and CentOS 7.4
  • Installation type: Compiling from source
  • Server: Apache
@kornerita

This comment has been minimized.

kornerita commented Apr 19, 2018

Hi,

I think this is related with this code:

cphalcon/phalcon/config.zep

Lines 325 to 329 in 4b37bdf

if is_numeric(key) {
let key = strval(number),
number++;
}
let instance->{key} = value;

That works fine with some numeric keys and does not with others.

Regards,
Federico.

@sergeyklay sergeyklay added this to the 3.4.x milestone Apr 26, 2018

@sergeyklay sergeyklay removed this from the 3.4.x milestone Jun 1, 2018

@sergeyklay sergeyklay added this to the unplanned milestone Aug 12, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Nov 21, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Nov 21, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Nov 21, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Nov 21, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Nov 21, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Nov 21, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Nov 21, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Nov 21, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Nov 21, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Nov 21, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Nov 21, 2018

@sergeyklay sergeyklay modified the milestones: unplanned, 3.4.2 Nov 22, 2018

@sergeyklay sergeyklay assigned sergeyklay and unassigned Jurigag Nov 22, 2018

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Nov 24, 2018

niden added a commit that referenced this issue Nov 24, 2018

sergeyklay added a commit that referenced this issue Nov 24, 2018

@niden

This comment has been minimized.

Member

niden commented Nov 24, 2018

This has been resolved

@niden niden closed this Nov 24, 2018

niden added a commit to niden/cphalcon that referenced this issue Nov 24, 2018

[phalcon#13578] - Merge 4.0 - Work on Config tests
* 4.0.x:
  Fixes phalcon#13351: Numeric config properties merge correctly now
  fix issue 13351 merge config on non-zero-based numeric index
  Added method to check is a relationship is loaded

niden added a commit to niden/cphalcon that referenced this issue Nov 29, 2018

[phalcon#13543] - Merge branch '4.0.x' into T13543-add-more-pdo-types-3
* 4.0.x: (21 commits)
  Fixed broken view test
  AppVeyor use latest Zephir
  Use latest Zephir from the 0.10.x branch [skip appveyor]
  Fixes phalcon#13351: Numeric config properties merge correctly now
  fix issue 13351 merge config on non-zero-based numeric index
  Added method to check is a relationship is loaded
  Updated Changelog [ci skip]
  Fixed phalcon#12725: Numeric INI config now builds properly
  Fixed default PHP config filename [skip appveyor]
  Add code coverage support [skip appveyor]
  Add `Phalcon\Http\Response\Cookies::getCookies`.
  Add `Phalcon\Http\Response\Cookies::getCookies`.
  Fixed AppVeyor build [skip travis]
  Update build/test deps for AppVeyor
  Update build/test deps for Travis CI
  Update CHANGELOG-4.0.md
  add offset to query builder
  Add changelog entry.
  Remove name property from Service.
  Remove name property from Service.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment