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

Conversation

tomzx
Copy link
Contributor

@tomzx tomzx commented Feb 1, 2016

The current code in AbstractConfig::has is not going to return an expected result if the value in the configuration is null.

    public function has($key)
    {
        return !is_null($this->get($key));
    }
somekey: null

In this particular case, AbstractConfig::has will return false even if it has the key somekey.

* release/0.9.1:
  Updated Travis CI configuration
tomzx added a commit to TomzxForks/config that referenced this pull request Jan 27, 2016
…ming that a null value equals the configuration doesn't have the requested key.
@ashnazg
Copy link
Contributor

ashnazg commented Feb 10, 2016

+1 -- I'm inclined to agree with the rationale here, that the expectation of the contract from has() is to see if the specified key exists, without any particular regard to what its value is set to.

I concede that the docblock currently states "checking if configuration values exist", but I'd argue that Rule of Least Surprise backs up that the most common expectation from reading has($myKey) would be checking for the key's existence, rather than the non-nullness of the key's value.

@@ -129,7 +129,25 @@ 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.

@tomzx tomzx force-pushed the fixes/66-config-with-null-values branch from 8850422 to 236d0a2 Compare February 10, 2016 17:34
tomzx added a commit to TomzxForks/config that referenced this pull request Feb 10, 2016
…ming that a null value equals the configuration doesn't have the requested key.
tomzx and others added 3 commits February 10, 2016 18:07
…t-extension

Add support for .dist extensions files
…ming that a null value equals the configuration doesn't have the requested key.
@tomzx tomzx force-pushed the fixes/66-config-with-null-values branch from 236d0a2 to 2b9967c Compare February 11, 2016 04:08
@tomzx
Copy link
Contributor Author

tomzx commented Feb 11, 2016

Replaced by PR #72 targeting develop.

@tomzx tomzx closed this Feb 11, 2016
hassankhan added a commit that referenced this pull request Feb 11, 2016
* release/0.10.0:
  Updated `CHANGELOG.md`
  [#66] Check if the configuration key exists instead of assuming that a null value equals the configuration doesn't have the requested key.
  Add support for .dist extensions files
  rearrange error handling to reach 100% test coverage;
  PSR2 corrections as generated by PHPCBF;
  provide package-level exceptions, so that callers can catch at the package level;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants