added test showing section override

[intco]

currently you can not overwrite a single key (e.g. 'secret' in the test method) without overwriting the whole section (e.g. 'application' in the test method).

[DavidePastore]

Hi @intco . Pay attention because $this->config is different from $config.

[intco]

I know :-) I created specifically a new instance to demonstrate that default options from getDefaults will be overridden entirely. The application.name is lost even if i set only the application.secret.

[DavidePastore]

Please, take a look to the source code of getDefaults to better understand it. It should be overridden to work as you want. Tell me in details if I'm missing something and if I'm missing your point.

[intco]

consider the SimpleConfig class used in the test

https://github.com/hassankhan/config/blob/develop/tests/Fixture/SimpleConfig.php

its getDefaults() is:

config/tests/Fixture/SimpleConfig.php

Lines 11 to 23 in a2b9931

	return [
	'host' => 'localhost',
	'port' => 80,
	'servers' => [
	'host1',
	'host2',
	'host3'
	],
	'application' => [
	'name' => 'configuration',
	'secret' => 's3cr3t'
	]
	];

If you instantiate the class providing an array like this:

      $config = new SimpleConfig(
            [
                'application' => ['secret' => 'verysecret']
            ]
        );

then this will fail

$this->assertEquals('configuration', $config->get('application.name')); 

because $this->data is now

     [
            'host' => 'localhost',
            'port'    => 80,
            'servers' => [
                'host1',
                'host2',
                'host3'
            ],
            'application' => [
                'secret' => 's3cr3t'
            ]
     ];

just add these lines to your local copy of AbstractConfigTest and try yourself

config/tests/AbstractConfigTest.php

Lines 506 to 515 in c72f0f0

	public function testDefaultOptionsSetOnInstantiationDoNotOverwriteSections()
	{
	$config = new SimpleConfig(
	[
	'application' => ['secret' => 'verysecret']
	]
	);
	$this->assertEquals('configuration', $config->get('application.name'));
	$this->assertEquals('verysecret', $config->get('application.secret'));
	}

[DavidePastore]

Thanks for the detailed explanation @intco , now it's clear. 😄 It's an issue and you are right about it.

A possible solution could be using array_merge_recursive instead of array_merge here:

config/src/AbstractConfig.php

Line 40 in a2b9931

$this->data = array_merge($this->getDefaults(), $data);

What do you think about it?

[intco]

I've tried with array_merge_recursive but a lot of tests failed, see below

> .\vendor\bin\phpunit
PHPUnit 7.5.1 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.1.17 with Xdebug 2.6.1
Configuration: (project-dir)phpunit.xml.dist

FF.F...F.........FFFF.........F..F............................... 65 / 72 ( 90%)
.......                                                           72 / 72 (100%)

Time: 2.35 seconds, Memory: 8.00MB

There were 10 failures:

1) Noodlehaus\AbstractConfigTest::testDefaultOptionsSetOnInstantiation
Array (...) does not match expected type "string".

(project-dir)tests\AbstractConfigTest.php:62

2) Noodlehaus\AbstractConfigTest::testGet
Array (...) does not match expected type "string".

(project-dir)tests\AbstractConfigTest.php:71

3) Noodlehaus\AbstractConfigTest::testGetNestedKey
Array (...) does not match expected type "string".

(project-dir)tests\AbstractConfigTest.php:87

4) Noodlehaus\AbstractConfigTest::testGetReturnsArray
Array (...) does not match expected type "string".

(project-dir)tests\AbstractConfigTest.php:120

5) Noodlehaus\AbstractConfigTest::testAll
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'host' => 'localhost'
-    'port' => 80
+    'host' => Array (...)
+    'port' => Array (...)
     'servers' => Array (
         0 => 'host1'
         1 => 'host2'
         2 => 'host3'
+        3 => 'host1'
+        4 => 'host2'
+        5 => 'host3'
     )
     'application' => Array (
-        'name' => 'configuration'
-        'secret' => 's3cr3t'
+        'name' => Array (...)
+        'secret' => Array (...)
         'runtime' => null
     )
     'user' => null
 )

(project-dir)tests\AbstractConfigTest.php:288

6) Noodlehaus\AbstractConfigTest::testMerge
Array (...) does not match expected type "string".

(project-dir)tests\AbstractConfigTest.php:304

7) Noodlehaus\AbstractConfigTest::testOffsetGet
Array (...) does not match expected type "string".

(project-dir)tests\AbstractConfigTest.php:312

8) Noodlehaus\AbstractConfigTest::testOffsetGetNestedKey
Array (...) does not match expected type "string".

(project-dir)tests\AbstractConfigTest.php:320

9) Noodlehaus\AbstractConfigTest::testIterator
Array (...) does not match expected type "string".

(project-dir)tests\AbstractConfigTest.php:485

10) Noodlehaus\AbstractConfigTest::testDefaultOptionsSetOnInstantiationDoNotOverwriteSections
Array (...) does not match expected type "string".

(project-dir)tests\AbstractConfigTest.php:514

FAILURES!
Tests: 72, Assertions: 142, Failures: 10.

[hassankhan]

Hi @intco, thanks for the detailed response and for posting your findings. I haven't had the chance to properly look, but I think those failing test cases are mainly due to test fixtures that weren't set up for recursive merging of properties.