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 · Jun 18, 2019 · b65e660 · b65e660
1 parent b69fed7
commit b65e660
Show file tree

Hide file tree

Showing 6 changed files with 347 additions and 185 deletions.
diff --git a/CHANGELOG-4.0.md b/CHANGELOG-4.0.md
@@ -40,6 +40,7 @@
 - Refactored `Phalcon\Registry` to align with the `Phalcon\Collection\Collection` interface. [#14154](https://github.com/phalcon/cphalcon/pull/14154)
 - `Phalcon\Mvc\Micro::setModelBinder()` now uses the Factory Default DI if none is set. [#14171](https://github.com/phalcon/cphalcon/pull/14171)
 - `Phalcon\Mvc\Model\ValidationFailed` now works with `ModelInterface`.
+- Refactored `Phalcon\Config` to extend `ArrayObject`
 
 ## Fixed
 - Fixed `Phalcon\Mvc\View::getRender()` to call `view->finish()` instead of `ob_end_clean()`. [#14095](https://github.com/phalcon/cphalcon/issues/14095)
@@ -58,12 +59,14 @@
 - `Phalcon\Di\FactoryDefault\Cli` can now use the new Filter system.
 - Fixed `Phalcon\Mvc\Router` now parses and uses path. [#14087](https://github.com/phalcon/cphalcon/issues/14087)
 - Fixed various areas in `Phalcon\Acl\Adapter` and `Phalcon\Acl\Adapter\Memory` including comments, logic, `denyComponentAccess` and `AdapterInterface`. Added tests. [#13870](https://github.com/phalcon/cphalcon/issues/13870)
+- Fixed `Phalcon\Config::merge()` not merging numeric values properly [#13201](https://github.com/phalcon/cphalcon/issues/13201)
 
 ## Removed
 - Removed `Phalcon\Session\Factory`. [#13672](https://github.com/phalcon/cphalcon/issues/13672)
 - Removed `Phalcon\Factory` and `Phalcon\FactoryInterface`. [#13672](https://github.com/phalcon/cphalcon/issues/13672)
 - Removed `Phalcon\Translate`. [#13672](https://github.com/phalcon/cphalcon/issues/13672)
 - Removed `Phalcon\Db\Column::getSchemaName()` as its not relevant or settable.
+- Removed `Phalcon\Config::__set_state()` as it does not serve any purpose and skipped the constructor.
 
 # [4.0.0-alpha.5](https://github.com/phalcon/cphalcon/releases/tag/v4.0.0-alpha.5) (2019-05-18)
 

diff --git a/phalcon/Config.zep b/phalcon/Config.zep
@@ -39,7 +39,7 @@ use Phalcon\Config\Exception;
  * );
  *</code>
  */
-class Config implements \ArrayAccess, \Countable
+class Config extends \ArrayObject
 {
     const DEFAULT_PATH_DELIMITER = ".";
 
@@ -50,39 +50,40 @@ class Config implements \ArrayAccess, \Countable
      */
     public function __construct(array! arrayConfig = null) -> void
     {
-        var key, value;
+        this->setFlags(\ArrayObject::ARRAY_AS_PROPS);
 
-        for key, value in arrayConfig {
-            this->offsetSet(key, value);
+        if typeof arrayConfig !== "null" {
+            this->fromArray(arrayConfig);
         }
     }
 
     /**
-     * Restores the state of a Phalcon\Config object
-     */
-    public static function __set_state(array! data) -> <Config>
-    {
-        return new self(data);
-    }
-
-    /**
-     * Returns the count of properties set in the config
-     *
-     *<code>
-     * print count($config);
-     *</code>
-     *
-     * or
+     * Sets an attribute using the array-syntax
      *
      *<code>
-     * print $config->count();
+     * $config["database"] = [
+     *     "type" => "Sqlite",
+     * ];
      *</code>
      */
-    public function count() -> int
+    public function offsetSet(var index, var value) -> void
     {
-        return count(
-            get_object_vars(this)
-        );
+        var data;
+
+        switch typeof value {
+            case "array":
+                let data = new self(value);
+                break;
+            case "object":
+                let data = value;
+                break;
+            default:
+                let data = (string) value;
+        }
+
+        let index = (string) index;
+
+        parent::offsetSet(index, data);
     }
 
     /**
@@ -96,13 +97,13 @@ class Config implements \ArrayAccess, \Countable
      */
     public function get(var index, var defaultValue = null) -> var
     {
-        let index = strval(index);
+        let index = (string) index;
 
-        if !isset this->{index} {
+        if !this->offsetExists(index) {
             return defaultValue;
         }
 
-        return this->{index};
+        return this->offsetGet(index);
     }
 
     /**
@@ -138,11 +139,11 @@ class Config implements \ArrayAccess, \Countable
      */
     public function merge(var configParam) -> <Config>
     {
-        var config;
+        var config, result, source, target;
 
         switch typeof configParam {
             case "array":
-                let config = new Config(configParam);
+                let config = new self(configParam);
                 break;
             case "object":
                 let config = configParam;
@@ -151,75 +152,15 @@ class Config implements \ArrayAccess, \Countable
                 throw new Exception("Invalid data type for merge.");
         }
 
-        return this->internalMerge(config);
-    }
-
-    /**
-     * Gets an attribute using the array-syntax
-     *
-     *<code>
-     * print_r(
-     *     $config["database"]
-     * );
-     *</code>
-     */
-    public function offsetGet(var index) -> var
-    {
-        let index = strval(index);
+        let source = this->toArray();
+        let target = config->toArray();
 
-        return this->{index};
-    }
+        let result = this->internalMerge(source, target);
 
+        this->reset();
+        this->fromArray(result);
 
-    /**
-     * Allows to check whether an attribute is defined using the array-syntax
-     *
-     *<code>
-     * var_dump(
-     *     isset($config["database"])
-     * );
-     *</code>
-     */
-    public function offsetExists(var index) -> bool
-    {
-        let index = strval(index);
-
-        return isset this->{index};
-    }
-
-    /**
-     * Sets an attribute using the array-syntax
-     *
-     *<code>
-     * $config["database"] = [
-     *     "type" => "Sqlite",
-     * ];
-     *</code>
-     */
-    public function offsetSet(var index, var value) -> void
-    {
-        let index = strval(index);
-
-        if typeof value === "array" {
-            let this->{index} = new self(value);
-        } else {
-            let this->{index} = value;
-        }
-    }
-
-    /**
-     * Unsets an attribute using the array-syntax
-     *
-     *<code>
-     * unset($config["database"]);
-     *</code>
-     */
-    public function offsetUnset(var index) -> void
-    {
-        let index = strval(index);
-
-        //unset(this->{index});
-        let this->{index} = null;
+        return this;
     }
 
     /**
@@ -285,10 +226,10 @@ class Config implements \ArrayAccess, \Countable
     public function toArray() -> array
     {
         var key, value, arrayConfig;
-
+      
         let arrayConfig = [];
 
-        for key, value in get_object_vars(this) {
+        for key, value in this->getArrayCopy() {
             if typeof value === "object" && method_exists(value, "toArray") {
                 let value = value->toArray();
             }
@@ -300,46 +241,40 @@ class Config implements \ArrayAccess, \Countable
     }
 
     /**
-     * Helper method for merge configs (forwarding nested config instance)
+     * Unsets all the values in object.
      */
-    final protected function internalMerge(<Config> config, <Config> instance = null) -> <Config>
+    public function reset() -> void
     {
-        var key, value, number, localObject, property;
+        var key;
 
-        if typeof instance !== "object" {
-            let instance = this;
+        for key in array_keys(this->getArrayCopy()) {
+            this->offsetUnset(key);
         }
+    }
 
-        let number = instance->count();
+    protected function fromArray(array arrayConfig) -> void
+    {
+        var key, value;
 
-        for key, value in get_object_vars(config) {
-            let property = strval(key);
+        for key, value in arrayConfig {
+            this->offsetSet(key, value);
+        }
+    }
 
-            if fetch localObject, instance->{property} {
-                if typeof localObject === "object" && typeof value === "object" {
-                    if localObject instanceof Config && value instanceof Config {
-                        this->internalMerge(value, localObject);
-                        continue;
-                    }
-                }
-            }
+    final protected function internalMerge(array source, array target) -> array
+    {
+        var key, value;
 
-            if is_numeric(key) {
-                let key = strval(key);
-
-                while instance->offsetExists(key) {
-                    /**
-                     * Increment the number afterwards, because "number" starts
-                     * at one not zero.
-                     */
-                    let key = strval(number);
-                    let number++;
-                }
+        for key, value in target {
+            if typeof value === "array" && isset source[key] && typeof source[key] === "array" {
+                let source[key] = this->internalMerge(source[key], value);
+            } elseif typeof key === "integer" {
+                let source[] = value;
+            } else {
+                let source[key] = value;
             }
-
-            let instance->{key} = value;
         }
 
-        return instance;
+        return source;
     }
 }
diff --git a/phalcon/Config/Adapter/Grouped.zep b/phalcon/Config/Adapter/Grouped.zep
@@ -76,9 +76,7 @@ class Grouped extends Config
     {
         var configName, configInstance, configArray;
 
-        parent::__construct(
-            []
-        );
+        parent::__construct([]);
 
         for configName in arrayConfig {
             let configInstance = configName;

diff --git a/tests/unit/Config/ConfigCest.php b/tests/unit/Config/ConfigCest.php
@@ -107,9 +107,7 @@ public function testStandardConfigSimpleArray(UnitTester $I)
                 ],
             ]
         );
-
-        $actual = new Config($settings);
-
+        $actual   = new Config($settings);
         $I->assertEquals($expected, $actual);
     }
 }
diff --git a/tests/unit/Config/GetCest.php b/tests/unit/Config/GetCest.php
@@ -12,6 +12,7 @@
 
 namespace Phalcon\Test\Unit\Config;
 
+use Phalcon\Config;
 use Phalcon\Test\Fixtures\Traits\ConfigTrait;
 use UnitTester;
 
@@ -20,14 +21,34 @@ class GetCest
     use ConfigTrait;
 
     /**
-     * Tests Phalcon\Config :: get()
+     * Tests Phalcon\Config :: __get()
      *
-     * @author Phalcon Team <team@phalconphp.com>
-     * @since  2018-11-13
+     * @author Cameron Hall <me@chall.id.au>
+     * @since  2019-06-17
+     */
+    public function configGetter(UnitTester $I)
+    {
+        $I->wantToTest('Config - get()');
+        $config = $this->getConfig();
+        $I->assertEquals(
+            $config->database->adapter,
+            $this->config['database']['adapter']
+        );
+    }
+
+    /**
+     * Tests Phalcon\Config :: __get()
+     *
+     * @author Cameron Hall <me@chall.id.au>
+     * @since  2019-06-17
      */
     public function configGet(UnitTester $I)
     {
         $I->wantToTest('Config - get()');
-        $this->checkGet($I);
+        $config = $this->getConfig();
+        $I->assertEquals(
+            $config->get('database')->get('adapter'),
+            $this->config['database']['adapter']
+        );
     }
 }