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

Config::has should check for key presence #66

Closed
55 changes: 29 additions & 26 deletions src/AbstractConfig.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ abstract class AbstractConfig implements ArrayAccess, ConfigInterface, Iterator
*
* @param array $data
*/
public function __construct(Array $data)
public function __construct(array $data)
{
$this->data = array_merge($this->getDefaults(), $data);
}
Expand All @@ -62,27 +62,11 @@ protected function getDefaults()
*/
public function get($key, $default = null)
{
// Check if already cached
if (isset($this->cache[$key])) {
if ($this->has($key)) {
return $this->cache[$key];
}

$segs = explode('.', $key);
$root = $this->data;

// nested case
foreach ($segs as $part) {
if (isset($root[$part])) {
$root = $root[$part];
continue;
} else {
$root = $default;
break;
}
}

// whatever we have is what we needed
return ($this->cache[$key] = $root);
return $default;
}

/**
Expand All @@ -96,7 +80,7 @@ public function set($key, $value)

// Look for the key, creating nested keys if needed
while ($part = array_shift($segs)) {
if($cacheKey != ''){
if ($cacheKey != '') {
$cacheKey .= '.';
}
$cacheKey .= $part;
Expand All @@ -106,14 +90,14 @@ public function set($key, $value)
$root = &$root[$part];

//Unset all old nested cache
if(isset($this->cache[$cacheKey])){
if (isset($this->cache[$cacheKey])) {
unset($this->cache[$cacheKey]);
}

//Unset all old nested cache in case of array
if(count($segs) == 0){
if (count($segs) == 0) {
foreach ($this->cache as $cacheLocalKey => $cacheValue) {
if(substr($cacheLocalKey, 0, strlen($cacheKey)) === $cacheKey){
if (substr($cacheLocalKey, 0, strlen($cacheKey)) === $cacheKey) {
unset($this->cache[$cacheLocalKey]);
}
}
Expand All @@ -129,7 +113,28 @@ public function set($key, $value)
*/
public function has($key)
{
return !is_null($this->get($key));
// Check if already cached
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is effectively copying the current body of get(), I'd suggest letting has() prime the cache when the key is found. You can then remove all this similar logic from get(), and just have get() call out to has() to see if it exists. If has() says false, then get() returns $default... if it says TRUE, then get() can return it right out of the cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I've also changed a little "side effect" the get method had, namely that it would set in cache the default value if the key didn't exist, which I think should not happen.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good catch... I agree that would be unexpected behavior to me.

if (isset($this->cache[$key])) {
return true;
}

$segments = explode('.', $key);
$root = $this->data;

// nested case
foreach ($segments as $segment) {
if (array_key_exists($segment, $root)) {
$root = $root[$segment];
continue;
} else {
return false;
}
}

// Set cache for the given key
$this->cache[$key] = $root;

return true;
}

/**
Expand All @@ -140,8 +145,6 @@ public function all()
return $this->data;
}



/**
* ArrayAccess Methods
*/
Expand Down
6 changes: 5 additions & 1 deletion src/Config.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,11 @@ public function __construct($path)

// Get file information
$info = pathinfo($path);
$extension = isset($info['extension']) ? $info['extension'] : '';
$parts = explode('.', $info['basename']);
$extension = array_pop($parts);
if ($extension === 'dist') {
$extension = array_pop($parts);
}
$parser = $this->getParser($extension);

// Try and load file
Expand Down
1 change: 0 additions & 1 deletion src/ConfigInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -52,5 +52,4 @@ public function has($key);
* @return array
*/
public function all();

}
7 changes: 7 additions & 0 deletions src/ErrorException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

namespace Noodlehaus;

class ErrorException extends \ErrorException
{
}
7 changes: 7 additions & 0 deletions src/Exception.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

namespace Noodlehaus;

class Exception extends \Exception
{
}
2 changes: 1 addition & 1 deletion src/Exception/EmptyDirectoryException.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

namespace Noodlehaus\Exception;

use Exception;
use Noodlehaus\Exception;

class EmptyDirectoryException extends Exception
{
Expand Down
2 changes: 1 addition & 1 deletion src/Exception/FileNotFoundException.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

namespace Noodlehaus\Exception;

use Exception;
use Noodlehaus\Exception;

class FileNotFoundException extends Exception
{
Expand Down
2 changes: 1 addition & 1 deletion src/Exception/ParseException.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

namespace Noodlehaus\Exception;

use ErrorException;
use Noodlehaus\ErrorException;

class ParseException extends ErrorException
{
Expand Down
2 changes: 1 addition & 1 deletion src/Exception/UnsupportedFormatException.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

namespace Noodlehaus\Exception;

use Exception;
use Noodlehaus\Exception;

class UnsupportedFormatException extends Exception
{
Expand Down
9 changes: 4 additions & 5 deletions src/FileParser/Json.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,12 @@ public function parse($path)
{
$data = json_decode(file_get_contents($path), true);

if (function_exists('json_last_error_msg')) {
$error_message = json_last_error_msg();
} else {
if (json_last_error() !== JSON_ERROR_NONE) {
$error_message = 'Syntax error';
}
if (function_exists('json_last_error_msg')) {
$error_message = json_last_error_msg();
}

if (json_last_error() !== JSON_ERROR_NONE) {
$error = array(
'message' => $error_message,
'type' => json_last_error(),
Expand Down
42 changes: 32 additions & 10 deletions tests/AbstractConfigTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ protected function setUp()
),
'application' => array(
'name' => 'configuration',
'secret' => 's3cr3t'
)
'secret' => 's3cr3t',
'runtime' => null,
),
'user' => null,
)
);
}
Expand Down Expand Up @@ -115,7 +117,7 @@ public function testGetReturnsArray()
{
$this->assertArrayHasKey('name', $this->config->get('application'));
$this->assertEquals('configuration', $this->config->get('application.name'));
$this->assertCount(2, $this->config->get('application'));
$this->assertCount(3, $this->config->get('application'));
}

/**
Expand Down Expand Up @@ -238,6 +240,7 @@ public function testSetAndUnsetArray()
public function testHas()
{
$this->assertTrue($this->config->has('application'));
$this->assertTrue($this->config->has('user'));
$this->assertFalse($this->config->has('not_exist'));
}

Expand All @@ -247,6 +250,7 @@ public function testHas()
public function testHasNestedKey()
{
$this->assertTrue($this->config->has('application.name'));
$this->assertTrue($this->config->has('application.runtime'));
$this->assertFalse($this->config->has('application.not_exist'));
$this->assertFalse($this->config->has('not_exist.name'));
}
Expand All @@ -266,8 +270,10 @@ public function testAll()
),
'application' => array(
'name' => 'configuration',
'secret' => 's3cr3t'
)
'secret' => 's3cr3t',
'runtime' => null,
),
'user' => null,
);
$this->assertEquals($all, $this->config->all());
}
Expand Down Expand Up @@ -338,6 +344,8 @@ public function testCurrent()
$this->assertEquals($this->config['servers'], $this->config->current());
$this->config->next();
$this->assertEquals($this->config['application'], $this->config->current());
$this->config->next();
$this->assertEquals($this->config['user'], $this->config->current());

/* Step beyond the end and confirm the result */
$this->config->next();
Expand All @@ -360,6 +368,8 @@ public function testKey()
$this->assertEquals('servers', $this->config->key());
$this->config->next();
$this->assertEquals('application', $this->config->key());
$this->config->next();
$this->assertEquals('user', $this->config->key());

/* Step beyond the end and confirm the result */
$this->config->next();
Expand All @@ -378,6 +388,7 @@ public function testNext()
$this->assertEquals($this->config['port'], $this->config->next());
$this->assertEquals($this->config['servers'], $this->config->next());
$this->assertEquals($this->config['application'], $this->config->next());
$this->assertEquals($this->config['user'], $this->config->next());

/* Step beyond the end and confirm the result */
$this->assertFalse($this->config->next());
Expand Down Expand Up @@ -413,6 +424,8 @@ public function testValid()
$this->assertTrue($this->config->valid());
$this->config->next();
$this->assertTrue($this->config->valid());
$this->config->next();
$this->assertTrue($this->config->valid());

/* Step beyond the end and confirm the result */
$this->config->next();
Expand All @@ -426,23 +439,32 @@ public function testValid()
public function testIterator()
{
/* Create numerically indexed copies of the test config */
$expectedKeys = array('host', 'port', 'servers', 'application');
$expectedKeys = array('host', 'port', 'servers', 'application', 'user');
$expectedValues = array(
'localhost',
80,
array('host1', 'host2', 'host3'),
array(
'name' => 'configuration',
'secret' => 's3cr3t'
)
'secret' => 's3cr3t',
'runtime' => null,
),
null
);

$idxConfig = 0;

foreach ($this->config as $configKey => $configValue) {
$this->assertEquals($expectedKeys [$idxConfig], $configKey);
$this->assertEquals($expectedValues [$idxConfig], $configValue);
$this->assertEquals($expectedKeys[$idxConfig], $configKey);
$this->assertEquals($expectedValues[$idxConfig], $configValue);
$idxConfig++;
}
}

public function testGetShouldNotSet()
{
$this->config->get('invalid', 'default');
$actual = $this->config->get('invalid', 'expected');
$this->assertSame('expected', $actual);
}
}
18 changes: 17 additions & 1 deletion tests/ConfigTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,23 @@ public function testConstructWithYml()

$this->assertEquals($expected, $actual);
}


/**
* @covers Noodlehaus\Config::__construct()
* @covers Noodlehaus\Config::getParser()
* @covers Noodlehaus\Config::getPathFromArray()
* @covers Noodlehaus\Config::getValidPath()
*/
public function testConstructWithYmlDist()
{
$config = new Config(__DIR__ . '/mocks/pass/config.yml.dist');

$expected = 'localhost';
$actual = $config->get('host');

$this->assertEquals($expected, $actual);
}

/**
* @covers Noodlehaus\Config::__construct()
* @covers Noodlehaus\Config::getParser()
Expand Down
9 changes: 9 additions & 0 deletions tests/mocks/pass/config.yml.dist
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
application:
name: configuration
secret: s3cr3t
host: localhost
port: 80
servers:
- host1
- host2
- host3