Method offsetUnset does not remove key

[prwater]

Method \Noodlehaus\AbstractConfig::offsetUnset (Deletes a key and its value) does not remove the key from the configuration but only set the value of the key to null.

The following code:

$config = new \Noodlehaus\Config('tests/mocks/pass/config.ini');
print_r($config->all());
$config->offsetUnset('application');
print_r($config->all());

yields the following output:

Array
(
    [host] => localhost
    [port] => 80
    [servers] => Array
        (
            [0] => host1
            [1] => host2
            [2] => host3
        )

    [application] => Array
        (
            [name] => configuration
            [secret] => s3cr3t
        )

)

Array
(
    [host] => localhost
    [port] => 80
    [servers] => Array
        (
            [0] => host1
            [1] => host2
            [2] => host3
        )

    [application] => 
)

The key application is still in the configuration (with value null).

Unit test \Noodlehaus\AbstractConfigTest\testRemove must use assertArrayNotHasKey instead of assertNull:

public function testRemove()
{
     $this->config->remove('application');
     $this->assertArrayNotHasKey('application', $this->config->all());
}

[filips123]

Ok, I will try to create a fix.

[filips123]

There are a few possible solutions how to fix this:

1. Create unset method in ConfigInterface and implement it in AbstractConfig that calls PHP's unset on correct keys in $this->data and $this->cache. Then use the new method in offsetUnset method. Adventage of this is that it is "most correct code" and also allows developers to remove keys directly with available methods. The first disadvantage is that some of the code may be duplicated with the set method. But probably not too much code because some things will also be different because of unsetting instead of setting keys. Another disadvantage may be that some developers consider changing the interface as a backward compatibility break. This may actually be true if someone would implement AbstractConfig themselves as old custom code will break (because of undefined unset method). But if developer use provided AbstractConfig class, there will be no break. This should we discuss.

2. Modify set method to unset key when given null value. An advantage is that it doesn't require interface change. A disadvantage is that it will be impossible to set a key to actual null value as the key will be deleted instead.

3. Modify set method to unset key when given special "undefined constant". This is similar to 2nd solution, but instead of unsetting key on null value, create a special constant with some pre-defined value, and delete key when providing this constant. Method offsetUnset should be modified to call set with that constant. Advantages are that it doesn't require interface change and it is still possible to set key to null value. Disadvantages are that it would be a bit hard to unset a key with provided methods (because it would require a call with pre-defined constant) and this is actually some kind of "hack" to make code work.

So, I would most like 1st or 3rd solution. @prwater @DavidePastore @hassankhan What do you think?

[DavidePastore]

@filips123 Sorry for the delay and thanks for your recap. I think that the best solution is the first one, even if it breaks the interface. We could add it as a new major release to be sure that developers have to upgrade their code to make it works.