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

Inconsistent Phalcon\Config::merge() #13201

Closed
pentagonal opened this issue Dec 8, 2017 · 1 comment

Comments

@pentagonal
Copy link

commented Dec 8, 2017

Inconsistent config merge

On some case of Phalcon\Config::merge that contain numeric (string/float/double) values possible lack of key name of config.

Because Phalcon\Config convert all keys into string, and when config doing merge process it will be validate the keys as numeric value not an integer (string -> integer).

Because on some case, config has been used to handle dynamic array values, eg: versioning.
Remove the is_numeric logic to is_int maybe better to make it more relevance.

CASE (Logic numeric)

if is_numeric(key) {

Code Execution

<?php
    use Phalcon\Config;
    $config        = new Config(['0.4' => 0.4]);
    $configToMerge = new Config(['1.1' => 1, '1.2' => 1.2, '2.8610229492188E-6' => 3]);
    $config->merge($configToMerge);

Result

Phalcon\Config Object
(
    [0.4] => 0.4
    [1] => 1
    [2] => 1.2
    [3] => 3
)

CASE (arrayaccess increments) -> offsetSet

And when config set as the increment also does not follow the array rules on method offsetSet

public function offsetSet(var index, var value)

Code


$config   = new Config();
$config[] =  'a';

Result

Phalcon\Config Object
(
    [] => a
)

The increment if produce array

Idea for increment key values (example code) -> like array reproduce of increment

/**
 * php code start for increment
 *
 * @var \Phalcon\Config $this
 */
$array = get_object_vars($this);
// manipulate array this will be reproduce increment values
$array[] = true;
// move pointer into last array
end($array);
// real last increment
$increment = key($array);

the offsetSet(index, value) the index will be contains key as null if the increment set as empty array increment

@stale stale bot added the stale label Apr 16, 2018

@sergeyklay sergeyklay closed this Apr 16, 2018

@phalcon phalcon deleted a comment from stale bot May 23, 2019

@niden niden reopened this May 23, 2019

@stale stale bot removed the stale label May 23, 2019

@niden niden added this to To do in 4.0 Release via automation May 23, 2019

@niden niden added the Bug - Low label May 23, 2019

@niden niden moved this from To do to In progress in 4.0 Release May 25, 2019

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Jun 18, 2019

Fixed phalcon#13201: Inconsistent config merging
Removed __set_state method
`Phalcon/Config` now extends `ArrayObject`
Merging is done properly
Assignment works as expected _all_ the time

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Jun 18, 2019

Fixed phalcon#13201: Inconsistent config merging
Removed __set_state method
`Phalcon/Config` now extends `ArrayObject`
Merging is done properly
Assignment works as expected _all_ the time

CameronHall added a commit to CameronHall/cphalcon that referenced this issue Jun 18, 2019

Fixed phalcon#13201: Inconsistent config merging
Removed __set_state method
`Phalcon/Config` now extends `ArrayObject`
Merging is done properly
Assignment works as expected _all_ the time

niden added a commit that referenced this issue Jun 20, 2019

Fixed #13201: Inconsistent config merging
Removed __set_state method
`Phalcon/Config` now extends `ArrayObject`
Merging is done properly
Assignment works as expected _all_ the time
@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
3 participants
You can’t perform that action at this time.