5dee3c11 break View as Markdown

If you think the current behavior is right, we need protection from
recursion (similar to __get()).

Thanks. Dmitry.

zend_std_has_dimension() doesn't know what (check_empty == 2) means.

check_empty == 0 - ISSET => we don't need to call offsetGet() and relay
on
offsetExists() return value.
check_empty == 1 - ISEMPTY => we should call offsetGet() after
offsetExists().

NULL values should be handled by offsetExists().

I am a bit curious, isset checks that the variable exists and is not
null; empty checks that the variable exists and is not empty. Why does
one call offsetGet and not the other? Both look at the value.

Sorry if I missed that bit of conversation.

yeah, no problem :)
I also, don't talk that my opinion is completely right.

I think that we make us and users more and more troubles with this messy
behavior.

Actually, you introduced new behavior (NULL check) and broke at least ZF2
and probably many based on it applications.
It leads to crash and it'll make bad php experience.
Then someone will report it as a security problem and we all will be
blamed :(
We shouldn't make existing applications crash!

Thanks. Dmitry.

zend_std_has_dimension() doesn't know what (check_empty == 2) means.

check_empty == 0 - ISSET => we don't need to call offsetGet() and relay
on
offsetExists() return value.
check_empty == 1 - ISEMPTY => we should call offsetGet() after
offsetExists().

NULL values should be handled by offsetExists().

I am a bit curious, isset checks that the variable exists and is not
null; empty checks that the variable exists and is not empty. Why does
one call offsetGet and not the other? Both look at the value.

Sorry if I missed that bit of conversation.

yeah, no problem :)
I also, don't talk that my opinion is completely right.

I think that we make us and users more and more troubles with this messy
behavior.

Actually, you introduced new behavior (NULL check) and broke at least ZF2
and probably many based on it applications.

That isn't any new behavior, at least it shouldn't be. isset does not
ONLY check existence, it checks that the value is not null; see
http://php.net/isset

It leads to crash and it'll make bad php experience.

Then someone will report it as a security problem and we all will be
blamed :(
We shouldn't make existing applications crash!

Agreed, but at least from what I understand the current behavior doesn't do
what it is supposed to anyway. Sometimes bugs become features but I really
don't see this as one of those. We should fix this. That's my $0.02.

On Tue, May 6, 2014 at 11:30 PM, Levi Morrison morrison.levi@gmail.comwrote:

I think perhaps I haven't been clear in the past posts; I apologize. The
basic premise is that isset and empty should work the same on arrays as
they do on any array object. The problem comes from the fact that
offsetExists does not say that you should return false for an existing
offset with a null value.

At the end of this email is is a gauntlet of tests that should work, the
only exception is ExtendedArrayAccess which returns true for an existing
key with a null value (uses array_key_exists). I'd argue that
ExtendedArrayAccess uses the correct semantics and that all others should
be changed to not do the null check (only does it exist) but that would be
a big BC break. Hopefully this all makes sense.

<?php

class ExtendedArrayObject extends ArrayObject {
function offsetExists($offset) {
//echo "\t", METHOD, PHP_EOL;
return parent::offsetExists($offset);
}
function offsetGet($offset) {
//echo "\t", METHOD, PHP_EOL;
return parent::offsetGet($offset);
}
}

class ExtendedArrayAccess implements ArrayAccess {
private $data = array();
function __construct(array $array = array()) {
$this->data = $array;
}
function offsetExists($offset) {
return array_key_exists($offset, $this->data);
}
function offsetGet($offset) {
return $this->data[$offset];
}
function offsetSet($offset, $value) {
$this->data[$offset] = $value;
}
function offsetUnset($offset) {
unset($this->data[$offset]);
}
}

function createInputs() {
return array(
array(),
new ArrayObject(),
new ExtendedArrayObject(),
new ExtendedArrayAccess(),
);
}

class Tests {
function isset_existing_key_with_not_empty_value($array) {
$array['foo'] = 1;
$this->expect(isset($array['foo']) === true, $array);
}

function empty_existing_key_with_not_empty_value($array) {
    $array['foo'] = 1;
    $this->expect(empty($array['foo']) === false, $array);
}

function isset_existing_key_with_empty_value($array) {
    $array['foo'] = 0;
    $this->expect(isset($array['foo']) === true, $array);
}

function empty_existing_key_with_empty_value($array) {
    $array['foo'] = 0;
    $this->expect(empty($array['foo']) === true, $array);
}

function isset_non_existent_key($array) {
    $this->expect(isset($array['foo']) === false, $array);
}

function empty_non_existent_key($array) {
    $this->expect(empty($array['foo']) === true, $array);
}

function isset_existing_key_with_null_value($array) {
    $array['foo'] = null;
    $this->expect(isset($array['foo']) === false, $array);
}

function empty_existing_key_with_null_value($array) {
    $array['foo'] = null;
    $this->expect(empty($array['foo']) === true, $array);
}

private function getType($v) {
   return is_array($v) ? 'array' : get_class($v);
}

private function expect($condition, $array) {
    if (!$condition) {
        echo "\t", $this->getType($array), ' failed ', PHP_EOL;
    }
}

}

$tests = new Tests;
foreach (get_class_methods($tests) as $test) {
echo "$test:", PHP_EOL;
foreach (createInputs() as $structure) {
$tests->$test($structure);
}
echo PHP_EOL;
}

Here it is on 3v4l: http://3v4l.org/SoFnK