Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Adding test coverage for raw #18

Merged
merged 1 commit into from over 2 years ago

3 participants

Michael Dowling Igor Wiedler Fabien Potencier
Michael Dowling

No description provided.

Igor Wiedler

Thanks, one minor thing. The naming is inconsistent with testOffsetGetValidatesKeyIsPresent. Key vs Identifier.

Michael Dowling

I corrected the naming inconsistency and modified the getRaw() method to use array_key_exists instead of isset.

Igor Wiedler

Please, testRawWithNullValue() instead of testing two things in one test.

Michael Dowling

Done. Used the same naming convention as testOffsetGetHonorsNullValues().

Igor Wiedler

Looks good to me.

Fabien Potencier
Owner

Can you squash your 3 commits? Thanks.

On a side note, what about using array_key_exists() in offsetExists() too?

Michael Dowling

Sure thing. I'll try to figure it out and update the pull request.

I'm not sure about changing offsetExists()... Calling isset() on a null value returns false. Because offsetExists() is triggered by an isset() call, I think it should return false for keys that are set to null values so that it is consistent with standard array behavior.

<?php
$a = array("a" => null);
var_export(isset($a["a"]));

outputs false

Igor Wiedler

I would keep the isset() in offsetExists. If you need help with the squashing, send me a private message here on github.

Michael Dowling

Sorry that took so long. I squashed it into one commit. Let me know if that doesn't look right.

Fabien Potencier fabpot referenced this pull request from a commit
Fabien Potencier merged branch mtdowling/ee0cdaa4ad6e3cbab25227785f49a75b9fd828f4 (PR #18
)

Commits
-------

a78e1f7 Using array_key_exists for raw instead of isset. Adding coverage.

Discussion
----------

Adding test coverage for raw

---------------------------------------------------------------------------

by igorw at 2011/08/16 07:25:29 -0700

Thanks, one minor thing. The naming is inconsistent with `testOffsetGetValidatesKeyIsPresent`. Key vs Identifier.

---------------------------------------------------------------------------

by mtdowling at 2011/08/17 12:16:16 -0700

I corrected the naming inconsistency and modified the getRaw() method to use array_key_exists instead of isset.

---------------------------------------------------------------------------

by igorw at 2011/08/17 13:56:53 -0700

Please, `testRawWithNullValue()` instead of testing two things in one test.

---------------------------------------------------------------------------

by mtdowling at 2011/08/17 14:08:25 -0700

Done.  Used the same naming convention as testOffsetGetHonorsNullValues().

---------------------------------------------------------------------------

by igorw at 2011/08/17 14:56:57 -0700

Looks good to me.

---------------------------------------------------------------------------

by fabpot at 2011/08/18 10:15:48 -0700

Can you squash your 3 commits? Thanks.

On a side note, what about using `array_key_exists()` in `offsetExists()` too?

---------------------------------------------------------------------------

by mtdowling at 2011/08/18 11:05:54 -0700

Sure thing. I'll try to figure it out and update the pull request.

I'm not sure about changing `offsetExists()`... Calling `isset()` on a null value returns false.  Because `offsetExists()` is triggered by an `isset()` call, I think it should return false for keys that are set to null values so that it is consistent with standard array behavior.

```php
<?php
$a = array("a" => null);
var_export(isset($a["a"]));
```
outputs `false`

---------------------------------------------------------------------------

by igorw at 2011/11/08 04:55:26 -0800

I would keep the isset() in offsetExists. If you need help with the squashing, send me a private message here on github.

---------------------------------------------------------------------------

by mtdowling at 2011/11/08 19:23:52 -0800

Sorry that took so long.  I squashed it into one commit.  Let me know if that doesn't look right.
8c77a39
Fabien Potencier fabpot merged commit a78e1f7 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Nov 09, 2011
Michael Dowling mtdowling Using array_key_exists for raw instead of isset. Adding coverage. a78e1f7
This page is out of date. Refresh to see the latest.

Showing 2 changed files with 18 additions and 1 deletion. Show diff stats Hide diff stats

  1. +1 1  lib/Pimple.php
  2. +17 0 tests/Pimple/Tests/PimpleTest.php
2  lib/Pimple.php
@@ -139,7 +139,7 @@ function protect(Closure $callable)
139 139 */
140 140 function raw($id)
141 141 {
142   - if (!isset($this->values[$id])) {
  142 + if (!array_key_exists($id, $this->values)) {
143 143 throw new InvalidArgumentException(sprintf('Identifier "%s" is not defined.', $id));
144 144 }
145 145
17 tests/Pimple/Tests/PimpleTest.php
@@ -165,4 +165,21 @@ public function testRaw()
165 165 $pimple['service'] = $definition = function () { return 'foo'; };
166 166 $this->assertSame($definition, $pimple->raw('service'));
167 167 }
  168 +
  169 + public function testRawHonorsNullValues()
  170 + {
  171 + $pimple = new Pimple();
  172 + $pimple['foo'] = null;
  173 + $this->assertNull($pimple->raw('foo'));
  174 + }
  175 +
  176 + /**
  177 + * @expectedException InvalidArgumentException
  178 + * @expectedExceptionMessage Identifier "foo" is not defined.
  179 + */
  180 + public function testRawValidatesKeyIsPresent()
  181 + {
  182 + $pimple = new Pimple();
  183 + $pimple->raw('foo');
  184 + }
168 185 }

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.