Skip to content

Fix confusing return statement #15

Merged
merged 1 commit into from Dec 30, 2011

2 participants

@avdg
avdg commented Dec 28, 2011

No description provided.

@avdg
avdg commented Dec 28, 2011

Notice: This patch should be fine, but I am not able to run phpunit for some reason.

@dragoonis
PPI Framework member

I think here we're assuming that $offset is also an array when you pass $value as null, but the null value might be a mistake, so probably best to check

if($value === null && is_array($offset))

avdg replied Dec 28, 2011

Yeah, the array notation was making me frustrating simply because I didn't investigate that use case enough atm.

Should be fixed by adding more tests

@dragoonis
PPI Framework member

I'm wondering why the return and $this->offsetUnset() aren't on the same line, this is forcing a void return value, will the function now always return void?

What's your stand on this.. PS, we (my company) are about to be using cookie class in production so it's getting thrown into the deep end :)

Thanks.

avdg replied Dec 28, 2011

Well, you was the one who complained about the return statement ;-)

ebd85c7#commitcomment-824650

But this version might be better as it could avoid confusions.

@dragoonis
PPI Framework member

Once we discuss the above 2 points, this can get merged and unit tests verified to be running.

Good work @avdg as usual :-)

@dragoonis
PPI Framework member

Have my comments/concerns been resolved? I couldn't see at a glance.

@dragoonis dragoonis merged commit 0707d3b into ppi:master Dec 30, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.