FAQ
Hi Tjerk,

your commit broke the code that worked fine before (still works in 5.5 but
broken in 5.6 and above).
It leads into infinity recursion until stack overflow.

It must be fixed or reverted.

Thanks. Dmitry.

<?php
class Parameters extends ArrayObject {
     public function __construct(array $values = null) {
         if (null === $values) {
             $values = array();
         }
         parent::__construct($values, ArrayObject::ARRAY_AS_PROPS);
     }
     public function offsetGet($name) {
         if (isset($this[$name])) {
             return parent::offsetGet($name);
         }
         return null;
     }
}
$x = new Parameters();
var_dump($x['foo']);
$x['foo'] = 'bar';
var_dump($x['foo']);
?>

Search Discussions

  • Tjerk Anne Meesters at May 6, 2014 at 10:31 am
    Hi Dmitry,
    On Tue, May 6, 2014 at 4:11 PM, Dmitry Stogov wrote:

    Hi Tjerk,

    your commit broke the code that worked fine before (still works in 5.5 but
    broken in 5.6 and above).
    It leads into infinity recursion until stack overflow.

    It must be fixed or reverted.
    This has been mentioned by Jakub before and a fix to ZF2 has already been
    merged:

    https://github.com/zendframework/zf2/pull/6096

    The previous code and my patch basically cannot coexist; it used to work in
    5.6 before, but only by the "virtue" of an unfortunate implementation.

    I believe this is not the only 5.6 issue that ZF2 is dealing with, but if
    you feel that this breaks too many things for a 5.x release I suppose we
    can revert it in PHP-5.6 and keep it for PHP-6?

    Let me know.

    Thanks. Dmitry.

    <?php
    class Parameters extends ArrayObject {
    public function __construct(array $values = null) {
    if (null === $values) {
    $values = array();
    }
    parent::__construct($values, ArrayObject::ARRAY_AS_PROPS);
    }
    public function offsetGet($name) {
    if (isset($this[$name])) {
    return parent::offsetGet($name);
    }
    return null;
    }
    }
    $x = new Parameters();
    var_dump($x['foo']);
    $x['foo'] = 'bar';
    var_dump($x['foo']);
    ?>


    --
    --
    Tjerk
  • Dmitry Stogov at May 6, 2014 at 11:25 am
    I didn't review you patch careful, but why do you need to call offsetGet()
    when offsetExists() must be enough?
    is it for empty()?
    Then you probably should implement it a way similar to
    zend_std_has_dimension() in Zend/zend_object_handlers.c.
    call offsetExists() and then offsetGet() if necessary.

    Thanks. Dmitry.

    On Tue, May 6, 2014 at 2:31 PM, Tjerk Anne Meesters wrote:

    Hi Dmitry,
    On Tue, May 6, 2014 at 4:11 PM, Dmitry Stogov wrote:

    Hi Tjerk,

    your commit broke the code that worked fine before (still works in 5.5
    but broken in 5.6 and above).
    It leads into infinity recursion until stack overflow.

    It must be fixed or reverted.
    This has been mentioned by Jakub before and a fix to ZF2 has already been
    merged:

    https://github.com/zendframework/zf2/pull/6096

    The previous code and my patch basically cannot coexist; it used to work
    in 5.6 before, but only by the "virtue" of an unfortunate implementation.

    I believe this is not the only 5.6 issue that ZF2 is dealing with, but if
    you feel that this breaks too many things for a 5.x release I suppose we
    can revert it in PHP-5.6 and keep it for PHP-6?

    Let me know.

    Thanks. Dmitry.

    <?php
    class Parameters extends ArrayObject {
    public function __construct(array $values = null) {
    if (null === $values) {
    $values = array();
    }
    parent::__construct($values, ArrayObject::ARRAY_AS_PROPS);
    }
    public function offsetGet($name) {
    if (isset($this[$name])) {
    return parent::offsetGet($name);
    }
    return null;
    }
    }
    $x = new Parameters();
    var_dump($x['foo']);
    $x['foo'] = 'bar';
    var_dump($x['foo']);
    ?>


    --
    --
    Tjerk
  • Tjerk Anne Meesters at May 6, 2014 at 11:30 am
    Hi,

    On Tue, May 6, 2014 at 7:25 PM, Dmitry Stogov wrote:

    I didn't review you patch careful, but why do you need to call offsetGet()
    when offsetExists() must be enough?
    is it for empty()?
    Mostly for empty(), yes. It fixes the expected empty() behaviour for
    `$a['foo'] == 0` when using ArrayObject. This is further explained in this
    bug report: https://bugs.php.net/bug.php?id=66834

    Then you probably should implement it a way similar to
    zend_std_has_dimension() in Zend/zend_object_handlers.c.
    call offsetExists() and then offsetGet() if necessary.
    That's already the case :)

    http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_object_handlers.c#719

    Thanks. Dmitry.

    On Tue, May 6, 2014 at 2:31 PM, Tjerk Anne Meesters wrote:

    Hi Dmitry,
    On Tue, May 6, 2014 at 4:11 PM, Dmitry Stogov wrote:

    Hi Tjerk,

    your commit broke the code that worked fine before (still works in 5.5
    but broken in 5.6 and above).
    It leads into infinity recursion until stack overflow.

    It must be fixed or reverted.
    This has been mentioned by Jakub before and a fix to ZF2 has already been
    merged:

    https://github.com/zendframework/zf2/pull/6096

    The previous code and my patch basically cannot coexist; it used to work
    in 5.6 before, but only by the "virtue" of an unfortunate implementation.

    I believe this is not the only 5.6 issue that ZF2 is dealing with, but if
    you feel that this breaks too many things for a 5.x release I suppose we
    can revert it in PHP-5.6 and keep it for PHP-6?

    Let me know.

    Thanks. Dmitry.

    <?php
    class Parameters extends ArrayObject {
    public function __construct(array $values = null) {
    if (null === $values) {
    $values = array();
    }
    parent::__construct($values, ArrayObject::ARRAY_AS_PROPS);
    }
    public function offsetGet($name) {
    if (isset($this[$name])) {
    return parent::offsetGet($name);
    }
    return null;
    }
    }
    $x = new Parameters();
    var_dump($x['foo']);
    $x['foo'] = 'bar';
    var_dump($x['foo']);
    ?>


    --
    --
    Tjerk

    --
    --
    Tjerk
  • Dmitry Stogov at May 6, 2014 at 11:50 am
    Ah, I didn't get what did you mean by (check_empty == 2), but they probably
    should be changed into (check_empty != 1)

    It fixes the problem.
    Could you please verify and commit into all relevant branches.

    Thanks. Dmitry.

    --- a/ext/spl/spl_array.c
    +++ b/ext/spl/spl_array.c
    @@ -603,7 +603,7 @@ static int spl_array_has_dimension_ex(int
    check_inherited, zval *object, zval *o

                     if (rv && zend_is_true(rv TSRMLS_CC)) {
                             zval_ptr_dtor(&rv);
    - if (check_empty == 2) {
    + if (check_empty != 1) {
                                     return 1;
                             } else if (intern->fptr_offset_get) {
                                     value = spl_array_read_dimension_ex(1,
    object, offset, BP_VAR_R TSRMLS_CC);
    @@ -622,7 +622,7 @@ static int spl_array_has_dimension_ex(int
    check_inherited, zval *object, zval *o
                     switch(Z_TYPE_P(offset)) {
                             case IS_STRING:
                                     if (zend_symtable_find(ht,
    Z_STRVAL_P(offset), Z_STRLEN_P(offset)+1, (void **) &tmp) != FAILURE) {
    - if (check_empty == 2) {
    + if (check_empty != 1) {
                                                     return 1;
                                             }
                                     } else {
    @@ -639,7 +639,7 @@ static int spl_array_has_dimension_ex(int
    check_inherited, zval *object, zval *o
                                             index = Z_LVAL_P(offset);
                                     }
                                     if (zend_hash_index_find(ht, index, (void
    **)&tmp) != FAILURE) {
    - if (check_empty == 2) {
    + if (check_empty != 1) {
                                                     return 1;
                                             }
                                     } else {


    On Tue, May 6, 2014 at 3:29 PM, Tjerk Anne Meesters wrote:

    Hi,

    On Tue, May 6, 2014 at 7:25 PM, Dmitry Stogov wrote:

    I didn't review you patch careful, but why do you need to call
    offsetGet() when offsetExists() must be enough?
    is it for empty()?
    Mostly for empty(), yes. It fixes the expected empty() behaviour for
    `$a['foo'] == 0` when using ArrayObject. This is further explained in this
    bug report: https://bugs.php.net/bug.php?id=66834

    Then you probably should implement it a way similar to
    zend_std_has_dimension() in Zend/zend_object_handlers.c.
    call offsetExists() and then offsetGet() if necessary.
    That's already the case :)

    http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_object_handlers.c#719

    Thanks. Dmitry.

    On Tue, May 6, 2014 at 2:31 PM, Tjerk Anne Meesters wrote:

    Hi Dmitry,
    On Tue, May 6, 2014 at 4:11 PM, Dmitry Stogov wrote:

    Hi Tjerk,

    your commit broke the code that worked fine before (still works in 5.5
    but broken in 5.6 and above).
    It leads into infinity recursion until stack overflow.

    It must be fixed or reverted.
    This has been mentioned by Jakub before and a fix to ZF2 has already
    been merged:

    https://github.com/zendframework/zf2/pull/6096

    The previous code and my patch basically cannot coexist; it used to work
    in 5.6 before, but only by the "virtue" of an unfortunate implementation.

    I believe this is not the only 5.6 issue that ZF2 is dealing with, but
    if you feel that this breaks too many things for a 5.x release I suppose we
    can revert it in PHP-5.6 and keep it for PHP-6?

    Let me know.

    Thanks. Dmitry.

    <?php
    class Parameters extends ArrayObject {
    public function __construct(array $values = null) {
    if (null === $values) {
    $values = array();
    }
    parent::__construct($values, ArrayObject::ARRAY_AS_PROPS);
    }
    public function offsetGet($name) {
    if (isset($this[$name])) {
    return parent::offsetGet($name);
    }
    return null;
    }
    }
    $x = new Parameters();
    var_dump($x['foo']);
    $x['foo'] = 'bar';
    var_dump($x['foo']);
    ?>


    --
    --
    Tjerk

    --
    --
    Tjerk
  • Tjerk Anne Meesters at May 6, 2014 at 12:36 pm

    On Tue, May 6, 2014 at 7:50 PM, Dmitry Stogov wrote:

    Ah, I didn't get what did you mean by (check_empty == 2), but they
    probably should be changed into (check_empty != 1)
    When I said mostly, perhaps I should have mentioned that the isset()
    behaviour was deliberately updated as well; in other words, the following
    works as expected:

    $x['foo'] = null;
    isset($x['foo']); // false

    After taking a closer look at zend_std_has_dimension() it seems that both
    isset() and empty() behave as if empty() was always used; of course, if
    this behaviour was translated into spl_array.c, the infinite recursion
    would still be an issue. So, at this point I'm unsure what the proper
    action point should be.


    It fixes the problem.
    Could you please verify and commit into all relevant branches.

    Thanks. Dmitry.

    --- a/ext/spl/spl_array.c
    +++ b/ext/spl/spl_array.c
    @@ -603,7 +603,7 @@ static int spl_array_has_dimension_ex(int
    check_inherited, zval *object, zval *o

    if (rv && zend_is_true(rv TSRMLS_CC)) {
    zval_ptr_dtor(&rv);
    - if (check_empty == 2) {
    + if (check_empty != 1) {
    return 1;
    } else if (intern->fptr_offset_get) {
    value = spl_array_read_dimension_ex(1,
    object, offset, BP_VAR_R TSRMLS_CC);
    @@ -622,7 +622,7 @@ static int spl_array_has_dimension_ex(int
    check_inherited, zval *object, zval *o
    switch(Z_TYPE_P(offset)) {
    case IS_STRING:
    if (zend_symtable_find(ht,
    Z_STRVAL_P(offset), Z_STRLEN_P(offset)+1, (void **) &tmp) != FAILURE) {
    - if (check_empty == 2) {
    + if (check_empty != 1) {
    return 1;
    }
    } else {
    @@ -639,7 +639,7 @@ static int spl_array_has_dimension_ex(int
    check_inherited, zval *object, zval *o
    index = Z_LVAL_P(offset);
    }
    if (zend_hash_index_find(ht, index, (void
    **)&tmp) != FAILURE) {
    - if (check_empty == 2) {
    + if (check_empty != 1) {
    return 1;
    }
    } else {


    On Tue, May 6, 2014 at 3:29 PM, Tjerk Anne Meesters wrote:

    Hi,

    On Tue, May 6, 2014 at 7:25 PM, Dmitry Stogov wrote:

    I didn't review you patch careful, but why do you need to call
    offsetGet() when offsetExists() must be enough?
    is it for empty()?
    Mostly for empty(), yes. It fixes the expected empty() behaviour for
    `$a['foo'] == 0` when using ArrayObject. This is further explained in this
    bug report: https://bugs.php.net/bug.php?id=66834

    Then you probably should implement it a way similar to
    zend_std_has_dimension() in Zend/zend_object_handlers.c.
    call offsetExists() and then offsetGet() if necessary.
    That's already the case :)

    http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_object_handlers.c#719

    Thanks. Dmitry.

    On Tue, May 6, 2014 at 2:31 PM, Tjerk Anne Meesters wrote:

    Hi Dmitry,
    On Tue, May 6, 2014 at 4:11 PM, Dmitry Stogov wrote:

    Hi Tjerk,

    your commit broke the code that worked fine before (still works in 5.5
    but broken in 5.6 and above).
    It leads into infinity recursion until stack overflow.

    It must be fixed or reverted.
    This has been mentioned by Jakub before and a fix to ZF2 has already
    been merged:

    https://github.com/zendframework/zf2/pull/6096

    The previous code and my patch basically cannot coexist; it used to
    work in 5.6 before, but only by the "virtue" of an unfortunate
    implementation.

    I believe this is not the only 5.6 issue that ZF2 is dealing with, but
    if you feel that this breaks too many things for a 5.x release I suppose we
    can revert it in PHP-5.6 and keep it for PHP-6?

    Let me know.

    Thanks. Dmitry.

    <?php
    class Parameters extends ArrayObject {
    public function __construct(array $values = null) {
    if (null === $values) {
    $values = array();
    }
    parent::__construct($values, ArrayObject::ARRAY_AS_PROPS);
    }
    public function offsetGet($name) {
    if (isset($this[$name])) {
    return parent::offsetGet($name);
    }
    return null;
    }
    }
    $x = new Parameters();
    var_dump($x['foo']);
    $x['foo'] = 'bar';
    var_dump($x['foo']);
    ?>


    --
    --
    Tjerk

    --
    --
    Tjerk

    --
    --
    Tjerk
  • Dmitry Stogov at May 6, 2014 at 12:43 pm
    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().

    Thanks. Dmitry.

    On Tue, May 6, 2014 at 4:36 PM, Tjerk Anne Meesters wrote:


    On Tue, May 6, 2014 at 7:50 PM, Dmitry Stogov wrote:

    Ah, I didn't get what did you mean by (check_empty == 2), but they
    probably should be changed into (check_empty != 1)
    When I said mostly, perhaps I should have mentioned that the isset()
    behaviour was deliberately updated as well; in other words, the following
    works as expected:

    $x['foo'] = null;
    isset($x['foo']); // false

    After taking a closer look at zend_std_has_dimension() it seems that both
    isset() and empty() behave as if empty() was always used; of course, if
    this behaviour was translated into spl_array.c, the infinite recursion
    would still be an issue. So, at this point I'm unsure what the proper
    action point should be.


    It fixes the problem.
    Could you please verify and commit into all relevant branches.

    Thanks. Dmitry.

    --- a/ext/spl/spl_array.c
    +++ b/ext/spl/spl_array.c
    @@ -603,7 +603,7 @@ static int spl_array_has_dimension_ex(int
    check_inherited, zval *object, zval *o

    if (rv && zend_is_true(rv TSRMLS_CC)) {
    zval_ptr_dtor(&rv);
    - if (check_empty == 2) {
    + if (check_empty != 1) {
    return 1;
    } else if (intern->fptr_offset_get) {
    value = spl_array_read_dimension_ex(1,
    object, offset, BP_VAR_R TSRMLS_CC);
    @@ -622,7 +622,7 @@ static int spl_array_has_dimension_ex(int
    check_inherited, zval *object, zval *o
    switch(Z_TYPE_P(offset)) {
    case IS_STRING:
    if (zend_symtable_find(ht,
    Z_STRVAL_P(offset), Z_STRLEN_P(offset)+1, (void **) &tmp) != FAILURE) {
    - if (check_empty == 2) {
    + if (check_empty != 1) {
    return 1;
    }
    } else {
    @@ -639,7 +639,7 @@ static int spl_array_has_dimension_ex(int
    check_inherited, zval *object, zval *o
    index = Z_LVAL_P(offset);
    }
    if (zend_hash_index_find(ht, index, (void
    **)&tmp) != FAILURE) {
    - if (check_empty == 2) {
    + if (check_empty != 1) {
    return 1;
    }
    } else {


    On Tue, May 6, 2014 at 3:29 PM, Tjerk Anne Meesters wrote:

    Hi,

    On Tue, May 6, 2014 at 7:25 PM, Dmitry Stogov wrote:

    I didn't review you patch careful, but why do you need to call
    offsetGet() when offsetExists() must be enough?
    is it for empty()?
    Mostly for empty(), yes. It fixes the expected empty() behaviour for
    `$a['foo'] == 0` when using ArrayObject. This is further explained in this
    bug report: https://bugs.php.net/bug.php?id=66834

    Then you probably should implement it a way similar to
    zend_std_has_dimension() in Zend/zend_object_handlers.c.
    call offsetExists() and then offsetGet() if necessary.
    That's already the case :)

    http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_object_handlers.c#719

    Thanks. Dmitry.

    On Tue, May 6, 2014 at 2:31 PM, Tjerk Anne Meesters wrote:

    Hi Dmitry,
    On Tue, May 6, 2014 at 4:11 PM, Dmitry Stogov wrote:

    Hi Tjerk,

    your commit broke the code that worked fine before (still works in
    5.5 but broken in 5.6 and above).
    It leads into infinity recursion until stack overflow.

    It must be fixed or reverted.
    This has been mentioned by Jakub before and a fix to ZF2 has already
    been merged:

    https://github.com/zendframework/zf2/pull/6096

    The previous code and my patch basically cannot coexist; it used to
    work in 5.6 before, but only by the "virtue" of an unfortunate
    implementation.

    I believe this is not the only 5.6 issue that ZF2 is dealing with, but
    if you feel that this breaks too many things for a 5.x release I suppose we
    can revert it in PHP-5.6 and keep it for PHP-6?

    Let me know.

    Thanks. Dmitry.

    <?php
    class Parameters extends ArrayObject {
    public function __construct(array $values = null) {
    if (null === $values) {
    $values = array();
    }
    parent::__construct($values, ArrayObject::ARRAY_AS_PROPS);
    }
    public function offsetGet($name) {
    if (isset($this[$name])) {
    return parent::offsetGet($name);
    }
    return null;
    }
    }
    $x = new Parameters();
    var_dump($x['foo']);
    $x['foo'] = 'bar';
    var_dump($x['foo']);
    ?>


    --
    --
    Tjerk

    --
    --
    Tjerk

    --
    --
    Tjerk
  • Levi Morrison at May 6, 2014 at 1:11 pm

    On Tue, May 6, 2014 at 6:43 AM, Dmitry Stogov wrote:

    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.
  • Dmitry Stogov at May 6, 2014 at 2:46 pm

    On Tue, May 6, 2014 at 5:11 PM, Levi Morrison wrote:
    On Tue, May 6, 2014 at 6:43 AM, Dmitry Stogov wrote:

    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.
  • Dmitry Stogov at May 6, 2014 at 2:48 pm
    If you think the current behavior is right, we need protection from
    recursion (similar to __get()).

    Thanks. Dmitry.

    On Tue, May 6, 2014 at 6:46 PM, Dmitry Stogov wrote:



    On Tue, May 6, 2014 at 5:11 PM, Levi Morrison wrote:
    On Tue, May 6, 2014 at 6:43 AM, Dmitry Stogov wrote:

    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.
  • Levi Morrison at May 6, 2014 at 2:50 pm

    On Tue, May 6, 2014 at 8:46 AM, Dmitry Stogov wrote:
    On Tue, May 6, 2014 at 5:11 PM, Levi Morrison wrote:
    On Tue, May 6, 2014 at 6:43 AM, Dmitry Stogov wrote:

    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.
  • Dmitry Stogov at May 6, 2014 at 2:57 pm
    On Tue, May 6, 2014 at 6:50 PM, Levi Morrison wrote:
    On Tue, May 6, 2014 at 8:46 AM, Dmitry Stogov wrote:
    On Tue, May 6, 2014 at 5:11 PM, Levi Morrison wrote:
    On Tue, May 6, 2014 at 6:43 AM, Dmitry Stogov wrote:

    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
    I know :), but we never did it for magic handlers. In case offsetExists()
    ot __isset() returned "flase" we didn't try to check the value.

    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.
    I would be very glad to have it fixed.

    Thanks. Dmitry.
  • Tjerk Anne Meesters at May 6, 2014 at 11:33 pm
    Hi,

    On Tue, May 6, 2014 at 8:43 PM, Dmitry Stogov wrote:

    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().
    Okay, I suppose it's fair to make its behaviour the same as for zend_std :)
    I'll make the necessary changes by tonight for 5.6 and master.

    Thanks. Dmitry.

    On Tue, May 6, 2014 at 4:36 PM, Tjerk Anne Meesters wrote:


    On Tue, May 6, 2014 at 7:50 PM, Dmitry Stogov wrote:

    Ah, I didn't get what did you mean by (check_empty == 2), but they
    probably should be changed into (check_empty != 1)
    When I said mostly, perhaps I should have mentioned that the isset()
    behaviour was deliberately updated as well; in other words, the following
    works as expected:

    $x['foo'] = null;
    isset($x['foo']); // false

    After taking a closer look at zend_std_has_dimension() it seems that both
    isset() and empty() behave as if empty() was always used; of course, if
    this behaviour was translated into spl_array.c, the infinite recursion
    would still be an issue. So, at this point I'm unsure what the proper
    action point should be.


    It fixes the problem.
    Could you please verify and commit into all relevant branches.

    Thanks. Dmitry.

    --- a/ext/spl/spl_array.c
    +++ b/ext/spl/spl_array.c
    @@ -603,7 +603,7 @@ static int spl_array_has_dimension_ex(int
    check_inherited, zval *object, zval *o

    if (rv && zend_is_true(rv TSRMLS_CC)) {
    zval_ptr_dtor(&rv);
    - if (check_empty == 2) {
    + if (check_empty != 1) {
    return 1;
    } else if (intern->fptr_offset_get) {
    value = spl_array_read_dimension_ex(1,
    object, offset, BP_VAR_R TSRMLS_CC);
    @@ -622,7 +622,7 @@ static int spl_array_has_dimension_ex(int
    check_inherited, zval *object, zval *o
    switch(Z_TYPE_P(offset)) {
    case IS_STRING:
    if (zend_symtable_find(ht,
    Z_STRVAL_P(offset), Z_STRLEN_P(offset)+1, (void **) &tmp) != FAILURE) {
    - if (check_empty == 2) {
    + if (check_empty != 1) {
    return 1;
    }
    } else {
    @@ -639,7 +639,7 @@ static int spl_array_has_dimension_ex(int
    check_inherited, zval *object, zval *o
    index = Z_LVAL_P(offset);
    }
    if (zend_hash_index_find(ht, index,
    (void **)&tmp) != FAILURE) {
    - if (check_empty == 2) {
    + if (check_empty != 1) {
    return 1;
    }
    } else {


    On Tue, May 6, 2014 at 3:29 PM, Tjerk Anne Meesters wrote:

    Hi,

    On Tue, May 6, 2014 at 7:25 PM, Dmitry Stogov wrote:

    I didn't review you patch careful, but why do you need to call
    offsetGet() when offsetExists() must be enough?
    is it for empty()?
    Mostly for empty(), yes. It fixes the expected empty() behaviour for
    `$a['foo'] == 0` when using ArrayObject. This is further explained in this
    bug report: https://bugs.php.net/bug.php?id=66834

    Then you probably should implement it a way similar to
    zend_std_has_dimension() in Zend/zend_object_handlers.c.
    call offsetExists() and then offsetGet() if necessary.
    That's already the case :)

    http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_object_handlers.c#719

    Thanks. Dmitry.

    On Tue, May 6, 2014 at 2:31 PM, Tjerk Anne Meesters wrote:

    Hi Dmitry,
    On Tue, May 6, 2014 at 4:11 PM, Dmitry Stogov wrote:

    Hi Tjerk,

    your commit broke the code that worked fine before (still works in
    5.5 but broken in 5.6 and above).
    It leads into infinity recursion until stack overflow.

    It must be fixed or reverted.
    This has been mentioned by Jakub before and a fix to ZF2 has already
    been merged:

    https://github.com/zendframework/zf2/pull/6096

    The previous code and my patch basically cannot coexist; it used to
    work in 5.6 before, but only by the "virtue" of an unfortunate
    implementation.

    I believe this is not the only 5.6 issue that ZF2 is dealing with,
    but if you feel that this breaks too many things for a 5.x release I
    suppose we can revert it in PHP-5.6 and keep it for PHP-6?

    Let me know.

    Thanks. Dmitry.

    <?php
    class Parameters extends ArrayObject {
    public function __construct(array $values = null) {
    if (null === $values) {
    $values = array();
    }
    parent::__construct($values, ArrayObject::ARRAY_AS_PROPS);
    }
    public function offsetGet($name) {
    if (isset($this[$name])) {
    return parent::offsetGet($name);
    }
    return null;
    }
    }
    $x = new Parameters();
    var_dump($x['foo']);
    $x['foo'] = 'bar';
    var_dump($x['foo']);
    ?>


    --
    --
    Tjerk

    --
    --
    Tjerk

    --
    --
    Tjerk

    --
    --
    Tjerk
  • Levi Morrison at May 7, 2014 at 5:30 am
    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;
    }
  • Levi Morrison at May 7, 2014 at 5:31 am

    On Tue, May 6, 2014 at 11:30 PM, Levi Morrison wrote:

    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
  • Tjerk Anne Meesters at May 7, 2014 at 4:31 am
    Hi,

    On Tue, May 6, 2014 at 7:50 PM, Dmitry Stogov wrote:

    Ah, I didn't get what did you mean by (check_empty == 2), but they
    probably should be changed into (check_empty != 1)

    It fixes the problem.
    Could you please verify and commit into all relevant branches.

    Thanks. Dmitry.

    --- a/ext/spl/spl_array.c
    +++ b/ext/spl/spl_array.c
    @@ -603,7 +603,7 @@ static int spl_array_has_dimension_ex(int
    check_inherited, zval *object, zval *o

    if (rv && zend_is_true(rv TSRMLS_CC)) {
    zval_ptr_dtor(&rv);
    - if (check_empty == 2) {
    + if (check_empty != 1) {
    return 1;
    } else if (intern->fptr_offset_get) {
    value = spl_array_read_dimension_ex(1,
    object, offset, BP_VAR_R TSRMLS_CC);
    @@ -622,7 +622,7 @@ static int spl_array_has_dimension_ex(int
    check_inherited, zval *object, zval *o
    switch(Z_TYPE_P(offset)) {
    case IS_STRING:
    if (zend_symtable_find(ht,
    Z_STRVAL_P(offset), Z_STRLEN_P(offset)+1, (void **) &tmp) != FAILURE) {
    - if (check_empty == 2) {
    + if (check_empty != 1) {
    return 1;
    }
    } else {
    @@ -639,7 +639,7 @@ static int spl_array_has_dimension_ex(int
    check_inherited, zval *object, zval *o
    index = Z_LVAL_P(offset);
    }
    if (zend_hash_index_find(ht, index, (void
    **)&tmp) != FAILURE) {
    - if (check_empty == 2) {
    + if (check_empty != 1) {
    return 1;
    }
    } else {
    This patch broke an earlier test case, so I've refactored it into the
    following compromise (until PHP.next):

    https://gist.github.com/datibbaw/55f0dc8e95f45b91e514

    Does that look okay to you?


    On Tue, May 6, 2014 at 3:29 PM, Tjerk Anne Meesters wrote:

    Hi,

    On Tue, May 6, 2014 at 7:25 PM, Dmitry Stogov wrote:

    I didn't review you patch careful, but why do you need to call
    offsetGet() when offsetExists() must be enough?
    is it for empty()?
    Mostly for empty(), yes. It fixes the expected empty() behaviour for
    `$a['foo'] == 0` when using ArrayObject. This is further explained in this
    bug report: https://bugs.php.net/bug.php?id=66834

    Then you probably should implement it a way similar to
    zend_std_has_dimension() in Zend/zend_object_handlers.c.
    call offsetExists() and then offsetGet() if necessary.
    That's already the case :)

    http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_object_handlers.c#719

    Thanks. Dmitry.

    On Tue, May 6, 2014 at 2:31 PM, Tjerk Anne Meesters wrote:

    Hi Dmitry,
    On Tue, May 6, 2014 at 4:11 PM, Dmitry Stogov wrote:

    Hi Tjerk,

    your commit broke the code that worked fine before (still works in 5.5
    but broken in 5.6 and above).
    It leads into infinity recursion until stack overflow.

    It must be fixed or reverted.
    This has been mentioned by Jakub before and a fix to ZF2 has already
    been merged:

    https://github.com/zendframework/zf2/pull/6096

    The previous code and my patch basically cannot coexist; it used to
    work in 5.6 before, but only by the "virtue" of an unfortunate
    implementation.

    I believe this is not the only 5.6 issue that ZF2 is dealing with, but
    if you feel that this breaks too many things for a 5.x release I suppose we
    can revert it in PHP-5.6 and keep it for PHP-6?

    Let me know.

    Thanks. Dmitry.

    <?php
    class Parameters extends ArrayObject {
    public function __construct(array $values = null) {
    if (null === $values) {
    $values = array();
    }
    parent::__construct($values, ArrayObject::ARRAY_AS_PROPS);
    }
    public function offsetGet($name) {
    if (isset($this[$name])) {
    return parent::offsetGet($name);
    }
    return null;
    }
    }
    $x = new Parameters();
    var_dump($x['foo']);
    $x['foo'] = 'bar';
    var_dump($x['foo']);
    ?>


    --
    --
    Tjerk

    --
    --
    Tjerk

    --
    --
    Tjerk
  • Dmitry Stogov at May 16, 2014 at 4:32 pm
    hi Tjerk,

    sorry, for long delay.
    The patch looks fine to me.
    I think you may commit it.
    Thanks for fixing this.

    Dmitry.


    On Wed, May 7, 2014 at 8:31 AM, Tjerk Anne Meesters wrote:

    Hi,

    On Tue, May 6, 2014 at 7:50 PM, Dmitry Stogov wrote:

    Ah, I didn't get what did you mean by (check_empty == 2), but they
    probably should be changed into (check_empty != 1)

    It fixes the problem.
    Could you please verify and commit into all relevant branches.

    Thanks. Dmitry.

    --- a/ext/spl/spl_array.c
    +++ b/ext/spl/spl_array.c
    @@ -603,7 +603,7 @@ static int spl_array_has_dimension_ex(int
    check_inherited, zval *object, zval *o

    if (rv && zend_is_true(rv TSRMLS_CC)) {
    zval_ptr_dtor(&rv);
    - if (check_empty == 2) {
    + if (check_empty != 1) {
    return 1;
    } else if (intern->fptr_offset_get) {
    value = spl_array_read_dimension_ex(1,
    object, offset, BP_VAR_R TSRMLS_CC);
    @@ -622,7 +622,7 @@ static int spl_array_has_dimension_ex(int
    check_inherited, zval *object, zval *o
    switch(Z_TYPE_P(offset)) {
    case IS_STRING:
    if (zend_symtable_find(ht,
    Z_STRVAL_P(offset), Z_STRLEN_P(offset)+1, (void **) &tmp) != FAILURE) {
    - if (check_empty == 2) {
    + if (check_empty != 1) {
    return 1;
    }
    } else {
    @@ -639,7 +639,7 @@ static int spl_array_has_dimension_ex(int
    check_inherited, zval *object, zval *o
    index = Z_LVAL_P(offset);
    }
    if (zend_hash_index_find(ht, index, (void
    **)&tmp) != FAILURE) {
    - if (check_empty == 2) {
    + if (check_empty != 1) {
    return 1;
    }
    } else {
    This patch broke an earlier test case, so I've refactored it into the
    following compromise (until PHP.next):

    https://gist.github.com/datibbaw/55f0dc8e95f45b91e514

    Does that look okay to you?


    On Tue, May 6, 2014 at 3:29 PM, Tjerk Anne Meesters wrote:

    Hi,

    On Tue, May 6, 2014 at 7:25 PM, Dmitry Stogov wrote:

    I didn't review you patch careful, but why do you need to call
    offsetGet() when offsetExists() must be enough?
    is it for empty()?
    Mostly for empty(), yes. It fixes the expected empty() behaviour for
    `$a['foo'] == 0` when using ArrayObject. This is further explained in this
    bug report: https://bugs.php.net/bug.php?id=66834

    Then you probably should implement it a way similar to
    zend_std_has_dimension() in Zend/zend_object_handlers.c.
    call offsetExists() and then offsetGet() if necessary.
    That's already the case :)

    http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_object_handlers.c#719

    Thanks. Dmitry.

    On Tue, May 6, 2014 at 2:31 PM, Tjerk Anne Meesters wrote:

    Hi Dmitry,
    On Tue, May 6, 2014 at 4:11 PM, Dmitry Stogov wrote:

    Hi Tjerk,

    your commit broke the code that worked fine before (still works in
    5.5 but broken in 5.6 and above).
    It leads into infinity recursion until stack overflow.

    It must be fixed or reverted.
    This has been mentioned by Jakub before and a fix to ZF2 has already
    been merged:

    https://github.com/zendframework/zf2/pull/6096

    The previous code and my patch basically cannot coexist; it used to
    work in 5.6 before, but only by the "virtue" of an unfortunate
    implementation.

    I believe this is not the only 5.6 issue that ZF2 is dealing with, but
    if you feel that this breaks too many things for a 5.x release I suppose we
    can revert it in PHP-5.6 and keep it for PHP-6?

    Let me know.

    Thanks. Dmitry.

    <?php
    class Parameters extends ArrayObject {
    public function __construct(array $values = null) {
    if (null === $values) {
    $values = array();
    }
    parent::__construct($values, ArrayObject::ARRAY_AS_PROPS);
    }
    public function offsetGet($name) {
    if (isset($this[$name])) {
    return parent::offsetGet($name);
    }
    return null;
    }
    }
    $x = new Parameters();
    var_dump($x['foo']);
    $x['foo'] = 'bar';
    var_dump($x['foo']);
    ?>


    --
    --
    Tjerk

    --
    --
    Tjerk

    --
    --
    Tjerk

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupphp-internals @
categoriesphp
postedMay 6, '14 at 8:11a
activeMay 16, '14 at 4:32p
posts17
users4
websitephp.net

People

Translate

site design / logo © 2022 Grokbase