# [PR 614] ArrayObject and isset() / empty() behaviour - Thread: https://externals.io/message/72989 - Original archive: https://news-web.php.net/php.internals/72989 - Started by Tjerk Meesters on 2014-03-07T10:14:18+00:00 - Messages: 13 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #72989 — Tjerk Meesters — 2014-03-07T10:14:18+00:00 URL: https://externals.io/message/72989#72989 Hi, I come across the following bug: https://bugs.php.net/bug.php?id=66834 With an extended ArrayObject instance, the behaviour of `isset()` and `empty()` is as follows: 1. Call `static::offsetExists()` 2. If return value is truthy, then a. isset() returns true b. empty() returns false c. spl_array_has_property() returns true However, if the ArrayObject instance is not overridden: 1. Fetch value (either via string or long key) 2. If key is found, then: a. isset() returns false if value is `null`, true otherwise. b. empty() returns true if value is truty, false otherwise. c. spl_array_has_property() returns true That's counterintuitive, so my patch addresses this by also calling `offsetGet()` if applicable. I wanted to bring this to the attention of the group first, because it may affect performance to achieve this goal: https://github.com/php/php-src/pull/614 I've tried to contact Marcus Boerger, but I was told that he's been less active lately. Let me know your thoughts. The patch targets 5.4 onwards. -- -- Tjerk --- ## #72991 — Julien Pauli — 2014-03-07T14:15:33+00:00 URL: https://externals.io/message/72989#72991 · In reply to: #72989 On Fri, Mar 7, 2014 at 11:14 AM, Tjerk Meesters wrote: > Hi, > > I come across the following bug: https://bugs.php.net/bug.php?id=66834 > > With an extended ArrayObject instance, the behaviour of `isset()` and > `empty()` is as follows: > > 1. Call `static::offsetExists()` > 2. If return value is truthy, then > a. isset() returns true > b. empty() returns false > c. spl_array_has_property() returns true > > However, if the ArrayObject instance is not overridden: > > 1. Fetch value (either via string or long key) > 2. If key is found, then: > a. isset() returns false if value is `null`, true otherwise. > b. empty() returns true if value is truty, false otherwise. > c. spl_array_has_property() returns true > > That's counterintuitive, so my patch addresses this by also calling > `offsetGet()` if applicable. I wanted to bring this to the attention of the > group first, because it may affect performance to achieve this goal: > > https://github.com/php/php-src/pull/614 > > I've tried to contact Marcus Boerger, but I was told that he's been less > active lately. Let me know your thoughts. The patch targets 5.4 onwards. You should try to contact Etienne Kneuss, he'll be able to help. He is often available on IRC. Julien --- ## #72994 — Etienne Kneuss — 2014-03-07T15:58:51+00:00 URL: https://externals.io/message/72989#72994 · In reply to: #72991 > On Fri, Mar 7, 2014 at 11:14 AM, Tjerk Meesters > wrote: > > […] > the > > […] > I have always been annoyed by this dichotomy between offsetExists() which is functionning like `array_key_exists()` and isset() which is returning false on defined, null values. Especially since isset() calls offsetExists() only. I would argue that the correct way to fix it is to have isset()/empty() call offsetGet() as well if it exists. So in that sense I feel like your patch is not addressing the right issue. In a sense ArrayObject does a hack to circumvent that in the first place, and you are trying to fix the hack to make it look more like what is supposed to happen. -- Etienne Kneuss --- ## #72995 — Etienne Kneuss — 2014-03-07T16:12:17+00:00 URL: https://externals.io/message/72989#72995 · In reply to: #72994 > > > > > > […] > > I have always been annoyed by this dichotomy between offsetExists() which > is functionning like `array_key_exists()` and isset() which is returning > false on defined, null values. Especially since isset() calls > offsetExists() only. > > I would argue that the correct way to fix it is to have isset()/empty() > call offsetGet() as well if it exists. So in that sense I feel like your > patch is not addressing the right issue. In a sense ArrayObject does a hack > to circumvent that in the first place, and you are trying to fix the hack > to make it look more like what is supposed to happen. > > Apart from this underlying "design" issue, it is true an overridden version of ArrayObject is not functionally equivalent to the original class. So it seems worth fixing independently from this hack. -- Etienne Kneuss --- ## #72998 — Tjerk Meesters — 2014-03-08T00:27:45+00:00 URL: https://externals.io/message/72989#72998 · In reply to: #72995 Hi Etienne, Thanks for your input, much appreciated. I have moved the decision up the chain as you suggested; the results can be found here: https://github.com/datibbaw/php-src/compare/php:PHP-5.4...bug66834a It's suffering from a few memory leaks which can be fixed, but would this be acceptable otherwise? > > > > > > > […] > Apart from this underlying "design" issue, it is true an overridden > version of ArrayObject is not functionally equivalent to the original > class. So it seems worth fixing independently from this hack. > -- > Etienne Kneuss > -- -- Tjerk --- ## #73004 — Tjerk Meesters — 2014-03-08T09:56:28+00:00 URL: https://externals.io/message/72989#73004 · In reply to: #72998 On Sat, Mar 8, 2014 at 8:27 AM, Tjerk Meesters wrote: > Hi Etienne, > > Thanks for your input, much appreciated. > > I have moved the decision up the chain as you suggested; the results can > be found here: > > https://github.com/datibbaw/php-src/compare/php:PHP-5.4...bug66834a > > > It's suffering from a few memory leaks which can be fixed, > They are fixed now and a new PR has been opened: https://github.com/php/php-src/pull/616 > but would this be acceptable otherwise? > > > > > > > […] > > > > -- > -- > Tjerk > -- -- Tjerk --- ## #73005 — Sherif Ramadan — 2014-03-08T16:50:06+00:00 URL: https://externals.io/message/72989#73005 · In reply to: #73004 On Sat, Mar 8, 2014 at 4:56 AM, Tjerk Meesters wrote: > On Sat, Mar 8, 2014 at 8:27 AM, Tjerk Meesters > […] > > > […] > > > […] > > They are fixed now and a new PR has been opened: > > https://github.com/php/php-src/pull/616 > > > > […] > and > > […] > your > > […] > hack > > […] > hack > > […] > > > > -- > -- > Tjerk > Could you actually make that into a sane macro or would that just be more trouble than it's worth? --- ## #73047 — Ferenc Kovacs — 2014-03-10T12:43:46+00:00 URL: https://externals.io/message/72989#73047 · In reply to: #73004 On Sat, Mar 8, 2014 at 10:56 AM, Tjerk Meesters wrote: > On Sat, Mar 8, 2014 at 8:27 AM, Tjerk Meesters > […] > > > […] > > > […] > > They are fixed now and a new PR has been opened: > > https://github.com/php/php-src/pull/616 > > Hi Etienne, Are you fine with the changes in the PR? -- Ferenc Kovács @Tyr43l - http://tyrael.hu --- ## #73050 — Etienne Kneuss — 2014-03-10T13:26:47+00:00 URL: https://externals.io/message/72989#73050 · In reply to: #73047 > > > > On Sat, Mar 8, 2014 at 10:56 AM, Tjerk Meesters wrote: > > > […] > Hi Etienne, > > Are you fine with the changes in the PR? > > Two things: 1) Where is this planned to be merged? Is it really 5.4 ? As is the PR is no longer a simple bug fix and while I agree it should be changed that way, it might introduce hard-to-track BC breaks. 2) Seems to me that with this PR, part of the hack present in ArrayObject in the has_dimension handler is no longer necessary, as it might be doing to value-check twice now. Best, -- > Ferenc Kovács > @Tyr43l - http://tyrael.hu > -- Etienne Kneuss http://www.colder.ch --- ## #73051 — Tjerk Meesters — 2014-03-10T13:58:31+00:00 URL: https://externals.io/message/72989#73051 · In reply to: #73050 Hi Etienne, > > > > > > > […] > Two things: > > 1) Where is this planned to be merged? Is it really 5.4 ? As is the PR is > no longer a simple bug fix and while I agree it should be changed that way, > it might introduce hard-to-track BC breaks. > After a short discussion on IRC I'm moving the branch to target 5.6 instead. > 2) Seems to me that with this PR, part of the hack present in ArrayObject > in the has_dimension handler is no longer necessary, as it might be doing > to value-check twice now. > I'll start looking into this now. Thanks for the heads up :) > > > Best, > > -- > > […] > > > > -- > Etienne Kneuss > http://www.colder.ch > -- -- Tjerk --- ## #73052 — Tjerk Meesters — 2014-03-10T16:20:59+00:00 URL: https://externals.io/message/72989#73052 · In reply to: #73051 On Mon, Mar 10, 2014 at 9:58 PM, Tjerk Meesters wrote: > Hi Etienne, > > > > > > […] > > After a short discussion on IRC I'm moving the branch to target 5.6 > instead. > > > > […] > > I'll start looking into this now. Thanks for the heads up :) > I've updated and cleaned up ext/spl; the most notable change is code removal according to a changed definition of zend_object_has_dimension_t: typedef int (*zend_object_has_dimension_t)(zval *object, zval *member TSRMLS_DC); PR: https://github.com/php/php-src/pull/621 Let me know if you see any other issues; I'm fine with reverting the typedef change if that could be an issue. > > > > […] > > > > -- > -- > Tjerk > -- -- Tjerk --- ## #73613 — Jakub Zelenka — 2014-04-06T12:55:20+00:00 URL: https://externals.io/message/72989#73613 · In reply to: #73052 Hi Tjerk, I was playing with ZF2 and getting segfaults for Mvc tests (for example phpunit ZendTest/Mvc/ApplicationTest.php). It's only on 5.6 and master branch. Think that it might be because of this patch: 0 zend_call_method zend_interfaces.c 35 0xae17fd 1 spl_array_read_dimension_ex spl_array.c 389 0x87179b 2 spl_array_has_dimension_ex spl_array.c 655 0x87273d 3 spl_array_has_dimension spl_array.c 675 0x8727d1 4 zend_isset_isempty_dim_prop_obj_handler_SPEC_CV_CV zend_vm_execute.h 40983 0xb817a8 5 ZEND_ISSET_ISEMPTY_DIM_OBJ_SPEC_CV_CV_HANDLER zend_vm_execute.h 41039 0xb819c1 6 execute_ex zend_vm_execute.h 363 0xb097cb 7 zend_execute zend_vm_execute.h 388 0xb098b1 8 zend_call_function zend_execute_API.c 952 0xaa6faa 9 zend_call_method zend_interfaces.c 97 0xae1bea 10 spl_array_read_dimension_ex spl_array.c 389 0x87179b 11 spl_array_has_dimension_ex spl_array.c 655 0x87273d 12 spl_array_has_dimension spl_array.c 675 0x8727d1 13 zend_isset_isempty_dim_prop_obj_handler_SPEC_CV_CV zend_vm_execute.h 40983 0xb817a8 14 ZEND_ISSET_ISEMPTY_DIM_OBJ_SPEC_CV_CV_HANDLER zend_vm_execute.h 41039 0xb819c1 15 execute_ex zend_vm_execute.h 363 0xb097cb 16 zend_execute zend_vm_execute.h 388 0xb098b1 17 zend_call_function zend_execute_API.c 952 0xaa6faa 18 zend_call_method zend_interfaces.c 97 0xae1bea 19 spl_array_read_dimension_ex spl_array.c 389 0x87179b 20 spl_array_has_dimension_ex spl_array.c 655 0x87273d ... Let me know if you can't recreate it Thanks Jakub On Mon, Mar 10, 2014 at 4:20 PM, Tjerk Meesters wrote: > On Mon, Mar 10, 2014 at 9:58 PM, Tjerk Meesters > […] > > > […] > wrote: > > […] > is > > […] > way, > > […] > ArrayObject > > […] > doing > > […] > > I've updated and cleaned up ext/spl; the most notable change is code > removal according to a changed definition of zend_object_has_dimension_t: > > typedef int (*zend_object_has_dimension_t)(zval *object, zval *member > TSRMLS_DC); > > PR: https://github.com/php/php-src/pull/621 > > Let me know if you see any other issues; I'm fine with reverting the > typedef change if that could be an issue. > > > > […] > > > > -- > -- > Tjerk --- ## #73628 — Tjerk Meesters — 2014-04-07T22:50:50+00:00 URL: https://externals.io/message/72989#73628 · In reply to: #73613 Hi, > Hi Tjerk, > > I was playing with ZF2 and getting segfaults for Mvc tests (for example > phpunit ZendTest/Mvc/ApplicationTest.php). It's only on 5.6 and master > branch. Think that it might be because of this patch: > > 0 zend_call_method zend_interfaces.c 35 0xae17fd > 1 spl_array_read_dimension_ex spl_array.c 389 0x87179b > 2 spl_array_has_dimension_ex spl_array.c 655 0x87273d > 3 spl_array_has_dimension spl_array.c 675 0x8727d1 > 4 zend_isset_isempty_dim_prop_obj_handler_SPEC_CV_CV > zend_vm_execute.h 40983 0xb817a8 > 5 ZEND_ISSET_ISEMPTY_DIM_OBJ_SPEC_CV_CV_HANDLER zend_vm_execute.h > 41039 0xb819c1 > 6 execute_ex zend_vm_execute.h 363 0xb097cb > 7 zend_execute zend_vm_execute.h 388 0xb098b1 > 8 zend_call_function zend_execute_API.c 952 0xaa6faa > 9 zend_call_method zend_interfaces.c 97 0xae1bea > 10 spl_array_read_dimension_ex spl_array.c 389 0x87179b > 11 spl_array_has_dimension_ex spl_array.c 655 0x87273d > 12 spl_array_has_dimension spl_array.c 675 0x8727d1 > 13 zend_isset_isempty_dim_prop_obj_handler_SPEC_CV_CV > zend_vm_execute.h 40983 0xb817a8 > 14 ZEND_ISSET_ISEMPTY_DIM_OBJ_SPEC_CV_CV_HANDLER > zend_vm_execute.h 41039 0xb819c1 > 15 execute_ex zend_vm_execute.h 363 0xb097cb > 16 zend_execute zend_vm_execute.h 388 0xb098b1 > 17 zend_call_function zend_execute_API.c 952 0xaa6faa > 18 zend_call_method zend_interfaces.c 97 0xae1bea > 19 spl_array_read_dimension_ex spl_array.c 389 0x87179b > 20 spl_array_has_dimension_ex spl_array.c 655 0x87273d > ... > Yeah, it causes infinite recursion because of this: public function offsetGet($name) { if (isset($this[$name])) { return parent::offsetGet($name); } return null; } Doing this "detaches" the call chain and enters it again later without any context to track a potential recursive call. I'm not sure whether it can be fixed other than not doing this. I've authored a (bc compatible) ZF2 fix here: https://github.com/zendframework/zf2/pull/6096 > Let me know if you can't recreate it > > Thanks > > Jakub > > > On Mon, Mar 10, 2014 at 4:20 PM, Tjerk Meesters wrote: > > > […] > > -- -- Tjerk