# Merge PR 621 - Thread: https://externals.io/message/73309 - Original archive: https://news-web.php.net/php.internals/73309 - Started by Tjerk Meesters on 2014-03-20T00:03:20+00:00 - Messages: 23 - 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 --- ## #73309 — Tjerk Meesters — 2014-03-20T00:03:20+00:00 URL: https://externals.io/message/73309#73309 Hi internals, Would someone with Zend karma be so kind to merge the following into 5.6: https://github.com/php/php-src/pull/621 It makes isset() and empty() call offsetGet() on objects that extend ArrayObject if offsetExists() returns TRUE. This work has been checked off with Etienne; if there are any issues with it, please let me know. -- -- Tjerk --- ## #73311 — Stas Malyshev — 2014-03-20T01:04:08+00:00 URL: https://externals.io/message/73309#73311 · In reply to: #73309 Hi! > https://github.com/php/php-src/pull/621 > > It makes isset() and empty() call offsetGet() on objects that extend > ArrayObject if offsetExists() returns TRUE. Looks at this patch, isn't that what check_empty parameter was supposed to be doing? Why doesn't it work and it needs to be moved to opcodes? -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #73312 — Tjerk Meesters — 2014-03-20T01:13:24+00:00 URL: https://externals.io/message/73309#73312 · In reply to: #73311 Hi Stas, > > > Hi! > > > […] > > Looks at this patch, isn't that what check_empty parameter was supposed > to be doing? Why doesn't it work and it needs to be moved to opcodes? Yes, that's probably what it was supposed to do, but as you can see from bug 66834 that's clearly not the case. I had earlier approached the problem purely from SPL standpoint (see linked PR) but Etienne said that it would be a better idea to tackle this issue further up the chain so to speak :) > > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ > (408)454-6900 ext. 227 --- ## #73313 — Stas Malyshev — 2014-03-20T01:16:58+00:00 URL: https://externals.io/message/73309#73313 · In reply to: #73312 Hi! > Yes, that's probably what it was supposed to do, but as you can see > from bug 66834 that's clearly not the case. I had earlier approached > the problem purely from SPL standpoint (see linked PR) but Etienne > said that it would be a better idea to tackle this issue further up > the chain so to speak :) I think we need to first find out why that code did not work, instead of ripping out the code that already was supposed to do exactly what this bug is saying and replacing it with other code. At least we need to know the reason why that did not work. So please do not merge this patch until we know what's going on there. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #73317 — Tjerk Meesters — 2014-03-20T02:36:48+00:00 URL: https://externals.io/message/73309#73317 · In reply to: #73313 On Thu, Mar 20, 2014 at 9:16 AM, Stas Malyshev wrote: > Hi! > > > […] > > I think we need to first find out why that code did not work, instead of > ripping out the code that already was supposed to do exactly what this > bug is saying and replacing it with other code. Here's my earlier work on this: https://github.com/php/php-src/pull/614 It currently "doesn't work" because isset()/empty() work like `array_key_exists()` on objects that extend ArrayObject, which is in some cases not the desired behaviour; see also: http://lxr.php.net/xref/PHP_5_6/ext/spl/spl_array.c#604 If 'offsetExists()' is not overridden, the logic is done correctly; see also: http://lxr.php.net/xref/PHP_5_6/ext/spl/spl_array.c#617 The behaviour is currently correct inside zend_std_has_dimension(), so making the fix in SPL alone (#614) would make performance "suffer" less unless offsetGet() is overridden. Personally I'm fine either way; like I said, my first patch was only done in SPL when Etienne suggested the approach in #621. > At least we need to know > the reason why that did not work. So please do not merge this patch > until we know what's going on there. > > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ > (408)454-6900 ext. 227 > -- -- Tjerk --- ## #73318 — Tjerk Meesters — 2014-03-20T02:41:29+00:00 URL: https://externals.io/message/73309#73318 · In reply to: #73317 Sorry, I hit "Send" too soon. On Thu, Mar 20, 2014 at 10:36 AM, Tjerk Meesters wrote: > > > > On Thu, Mar 20, 2014 at 9:16 AM, Stas Malyshev wrote: > > > […] > > > Here's my earlier work on this: https://github.com/php/php-src/pull/614 > > It currently "doesn't work" because isset()/empty() work like > `array_key_exists()` on objects that extend ArrayObject, which is in some > cases not the desired behaviour; see also: > http://lxr.php.net/xref/PHP_5_6/ext/spl/spl_array.c#604 > > If 'offsetExists()' is not overridden, the logic is done correctly; see > also: http://lxr.php.net/xref/PHP_5_6/ext/spl/spl_array.c#617 > I forgot to mention that the current behaviour is not necessarily correct if `offsetGet()` is overridden in a child class; the overridden method may decide to return `null` or something empty. > The behaviour is currently correct inside zend_std_has_dimension(), so > making the fix in SPL alone (#614) would make performance "suffer" less > unless offsetGet() is overridden. > > Personally I'm fine either way; like I said, my first patch was only done > in SPL when Etienne suggested the approach in #621. > > > > […] > > > > -- > -- > Tjerk > -- -- Tjerk --- ## #73339 — Etienne Kneuss — 2014-03-20T16:26:45+00:00 URL: https://externals.io/message/73309#73339 · In reply to: #73313 On Thu, Mar 20, 2014 at 2:16 AM, Stas Malyshev wrote: > Hi! > > > […] > > I think we need to first find out why that code did not work, instead of > ripping out the code that already was supposed to do exactly what this > bug is saying and replacing it with other code. At least we need to know > the reason why that did not work. So please do not merge this patch > until we know what's going on there. > The main question here is the following: what is the supposed semantics of has_dimension: is it the semantics of isset(), or `array_key_exists()`. The current behavior is: isset() -> has_dimension(check_exmpty=false) -> offsetExists() empty() -> !has_dimension(check_exmpty=true) -> !offsetExists() || offsetGet() which one might argue is inconsistent, at least in names. offsetExists relates to array_key_exists, and as such should IMHO return true for entries that are defined to NULL. ArrayObject follows that by (badly) hacking within has_dimension: $o = new ArrayObject(array('a' => NULL)); var_dump($o->offsetExists('a')); // true var_dump(isset($o['a'])); // false as long as ArrayObject::offset* are not overriden This patch is about decoupling has_dimension from isset()/empty() and doing, instead: isset() -> has_dimension() && get_dimension() !== `NULL` -> offsetExists() && offsetGet() !== `NULL` empty() -> !has_dimension() || !get_dimension() -> !offsetExists() && !offsetGet() which is in my opinion a sensible thing to do. Now it is clear it is a BC breaking change for people that defined offsetExists with array_key_exists: isset() which returned true for `NULL` will now invoke offsetGet and thus return false, but it is a change towards more consistency. If not everybody is confident about this, we will need a RFC. -- Etienne Kneuss --- ## #73340 — Etienne Kneuss — 2014-03-20T16:30:44+00:00 URL: https://externals.io/message/73309#73340 · In reply to: #73339 oops, few typos. > > > > On Thu, Mar 20, 2014 at 2:16 AM, Stas Malyshev wrote: > > > […] > > The main question here is the following: > > what is the supposed semantics of has_dimension: is it the semantics of > isset(), or `array_key_exists()`. > > The current behavior is: > > isset() -> has_dimension(check_exmpty=false) -> offsetExists() > empty() -> !has_dimension(check_exmpty=true) -> !offsetExists() || > offsetGet() > empty() -> !has_dimension(check_exmpty=true) -> !offsetExists() || !offsetGet() > > > which one might argue is inconsistent, at least in names. offsetExists > relates to array_key_exists, and as such should IMHO return true for > entries that are defined to NULL. > > ArrayObject follows that by (badly) hacking within has_dimension: > > $o = new ArrayObject(array('a' => NULL)); > var_dump($o->offsetExists('a')); // true > var_dump(isset($o['a'])); // false as long as ArrayObject::offset* are not > overriden > > This patch is about decoupling has_dimension from isset()/empty() and > doing, instead: > > isset() -> has_dimension() && get_dimension() !== `NULL` -> > offsetExists() && offsetGet() !== `NULL` > empty() -> !has_dimension() || !get_dimension() -> !offsetExists() > && !offsetGet() > isset() -> has_dimension() && read_dimension() !== `NULL` -> offsetExists() && offsetGet() !== `NULL` empty() -> !has_dimension() || !read_dimension() -> !offsetExists() || !offsetGet() > > which is in my opinion a sensible thing to do. > > Now it is clear it is a BC breaking change for people that defined > offsetExists with array_key_exists: isset() which returned true for `NULL` > will now invoke offsetGet and thus return false, but it is a change towards > more consistency. > > If not everybody is confident about this, we will need a RFC. > > -- > Etienne Kneuss > -- Etienne Kneuss http://www.colder.ch --- ## #73344 — Stas Malyshev — 2014-03-20T20:05:47+00:00 URL: https://externals.io/message/73309#73344 · In reply to: #73339 Hi! > The current behavior is: > > isset() -> has_dimension(check_exmpty=false) -> offsetExists() > empty() -> !has_dimension(check_exmpty=true) -> !offsetExists() > || offsetGet() I think isset part is wrong - it should use offsetGet (probably via check_empty=true) and then verify that the result is not null. > This patch is about decoupling has_dimension from isset()/empty() and > doing, instead: > > isset() -> has_dimension() && get_dimension() !== `NULL` -> > offsetExists() && offsetGet() !== `NULL` > empty() -> !has_dimension() || !get_dimension() -> > !offsetExists() && !offsetGet() I'm not sure this is the right way to go. It changes the existing API without any need - while it can be handled within the existing API as well. > Now it is clear it is a BC breaking change for people that defined > offsetExists with array_key_exists: isset() which returned true for `NULL` > will now invoke offsetGet and thus return false, but it is a change > towards more consistency. This is actually OK, isset should return for ArrayObject the same it would return for array with the same data. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #73345 — Etienne Kneuss — 2014-03-20T21:21:41+00:00 URL: https://externals.io/message/73309#73345 · In reply to: #73344 On Thu, Mar 20, 2014 at 9:05 PM, Stas Malyshev wrote: > Hi! > > > […] > > I think isset part is wrong - it should use offsetGet (probably via > check_empty=true) and then verify that the result is not null. > > > […] > > I'm not sure this is the right way to go. It changes the existing API > without any need - while it can be handled within the existing API as well. > I agree that the API change is not in fact needed. We could limit ourselves to changing the behavior of the default has_dimension handler. It is also probably faster to do everything in has_dimension. I still think the "new version" is clearer, but it might not be worth the hassle of forcing this API change. The important part is that isset() triggers read_dimension and eventually offsetGet() for isset(), and we seem to agree on that. > > […] > > This is actually OK, isset should return for ArrayObject the same it > would return for array with the same data. > > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ > (408)454-6900 ext. 227 > -- Etienne Kneuss http://www.colder.ch --- ## #73346 — Stas Malyshev — 2014-03-20T22:02:41+00:00 URL: https://externals.io/message/73309#73346 · In reply to: #73345 Hi! > The important part is that isset() triggers read_dimension and > eventually offsetGet() for isset(), and we seem to agree on that. Yes, agreed. Unfortunately, I'll be busy till the next week so couldn't propose a fix, but if nobody beats me to it I'll look at it next week. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #73349 — Tjerk Meesters — 2014-03-20T23:23:11+00:00 URL: https://externals.io/message/73309#73349 · In reply to: #73346 Hi Stas, On Fri, Mar 21, 2014 at 6:02 AM, Stas Malyshev wrote: > Hi! > > > […] > > Yes, agreed. Unfortunately, I'll be busy till the next week so couldn't > propose a fix, but if nobody beats me to it I'll look at it next week. > It seems that PR 614 covers exactly that; it uses the check_empty flag within spl_array_has_dimension_ex() to make sure offsetGet() is called when needed. https://github.com/php/php-src/pull/614 Was there something I missed there? I'm happy to work on this :) > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ > (408)454-6900 ext. 227 > -- -- Tjerk --- ## #73351 — Stas Malyshev — 2014-03-21T01:11:11+00:00 URL: https://externals.io/message/73309#73351 · In reply to: #73349 Hi! > It seems that PR 614 covers exactly that; it uses the check_empty flag > within spl_array_has_dimension_ex() to make sure offsetGet() is called > when needed. I understand that the problem may be that check_empty is not sent on isset? Didn't verify that though. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #73352 — Tjerk Meesters — 2014-03-21T02:48:48+00:00 URL: https://externals.io/message/73309#73352 · In reply to: #73351 Hi Stas, On Fri, Mar 21, 2014 at 9:11 AM, Stas Malyshev wrote: > Hi! > > > […] > > I understand that the problem may be that check_empty is not sent on > isset? Didn't verify that though. > It does, as can be seen here: http://lxr.php.net/xref/PHP_5_6/Zend/zend_vm_def.h#4737 The check_empty argument value is ternary, meaning it can be 0 (isset), 1 (empty) or 2 (key exists). Having looked it over I realized that my older patch had some issues; I've addressed those and reopened #614. I've closed #621. > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ > (408)454-6900 ext. 227 > -- -- Tjerk --- ## #73397 — Tjerk Meesters — 2014-03-25T00:24:40+00:00 URL: https://externals.io/message/73309#73397 · In reply to: #73352 Hi Stas, On Fri, Mar 21, 2014 at 10:48 AM, Tjerk Meesters wrote: > Hi Stas, > > On Fri, Mar 21, 2014 at 9:11 AM, Stas Malyshev wrote: > > > […] > > It does, as can be seen here: > > http://lxr.php.net/xref/PHP_5_6/Zend/zend_vm_def.h#4737 > > The check_empty argument value is ternary, meaning it can be 0 (isset), 1 > (empty) or 2 (key exists). > > Having looked it over I realized that my older patch had some issues; I've > addressed those and reopened #614. > Do you have any issues with the code as it stands now? https://github.com/php/php-src/pull/614 It's currently targeted at 5.4; if there are any objections to that I'll move the changes to 5.6 instead. > > I've closed #621. > > > > […] > > > > -- > -- > Tjerk > -- -- Tjerk --- ## #73398 — Levi Morrison — 2014-03-25T01:48:17+00:00 URL: https://externals.io/message/73309#73398 · In reply to: #73397 > > It's currently targeted at 5.4; if there are any objections to that I'll > move the changes to 5.6 instead. I object. I really like the changes but don't think they should go into those releases. While this is a bug fix I imagine there really are people out there using this behavior; better to put it into PHP 5.6. I only did a really quick glance, though; maybe someone else can comment on it and we can reach an agreement. --- ## #73408 — Stas Malyshev — 2014-03-25T03:34:01+00:00 URL: https://externals.io/message/73309#73408 · In reply to: #73397 Hi! > Do you have any issues with the code as it stands now? > > https://github.com/php/php-src/pull/614 Something I don't understand in this patch: so if we don't have "has" override, we'd get value == `NULL` and then we go and check internal hashtable. Shouldn't we first check the get override and ask it with BP_VAR_IS, and only if get override is not there use direct hash access? E.g. imagine this: class SecretArrayObject extends ArrayObject { public function offsetGet($offset) { var_dump('Called: '.__METHOD__); return parent::offsetGet(str_rot13($offset)); } public function offsetSet($offset) { var_dump('Called: '.__METHOD__); return parent::offsetSet(str_rot13($offset)); } } I think on this example your code may fail to go to correct items when doing isset/isempty, since if I ask for 'qux' it would actually check the value I've put there for 'dhk'. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #73409 — Tjerk Meesters — 2014-03-25T03:45:23+00:00 URL: https://externals.io/message/73309#73409 · In reply to: #73408 On Tue, Mar 25, 2014 at 11:34 AM, Stas Malyshev wrote: > Hi! > > > […] > > Something I don't understand in this patch: so if we don't have "has" > override, we'd get value == `NULL` and then we go and check internal > hashtable. Shouldn't we first check the get override and ask it with > BP_VAR_IS, and only if get override is not there use direct hash access? > It's my understanding that you should (have to) only call "get" if you know that the offset exists. > > E.g. imagine this: > > class SecretArrayObject extends ArrayObject > { > public function offsetGet($offset) { > var_dump('Called: '.__METHOD__); > return parent::offsetGet(str_rot13($offset)); > } > public function offsetSet($offset) { > var_dump('Called: '.__METHOD__); > return parent::offsetSet(str_rot13($offset)); > } > } > > I think on this example your code may fail to go to correct items when > doing isset/isempty, since if I ask for 'qux' it would actually check > the value I've put there for 'dhk'. > This particular class should override `offsetExists($offset)` as well, i.e.: public function offsetExists($offset) { return parent::offsetExists(str_rot13($offset)); } > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ > (408)454-6900 ext. 227 > -- -- Tjerk --- ## #73410 — Stas Malyshev — 2014-03-25T03:54:31+00:00 URL: https://externals.io/message/73309#73410 · In reply to: #73409 Hi! > It's my understanding that you should (have to) only call "get" if you > know that the offset exists. Yes, you are right. I'd still add some example that changes the keys to the tests to ensure all cases are covered properly. But looks like it works OK in all cases I could think of. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #73421 — Tjerk Meesters — 2014-03-25T10:38:45+00:00 URL: https://externals.io/message/73309#73421 · In reply to: #73410 Hi, On Tue, Mar 25, 2014 at 11:54 AM, Stas Malyshev wrote: > Hi! > > > […] > > Yes, you are right. I'd still add some example that changes the keys to > the tests to ensure all cases are covered properly. But looks like it > works OK in all cases I could think of. > I've updated the test case with such an example. PHP-5.6 and master have been updated. > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ > (408)454-6900 ext. 227 > -- -- Tjerk --- ## #73426 — Levi Morrison — 2014-03-25T13:51:45+00:00 URL: https://externals.io/message/73309#73426 · In reply to: #73421 In a similar vein have we checked `offsetSet`? It seems `offsetSet` is not called with `+=`; I am not sure if this is intended; see https://bugs.php.net/bug.php?id=66641 On Tue, Mar 25, 2014 at 4:38 AM, Tjerk Meesters wrote: > Hi, > > > On Tue, Mar 25, 2014 at 11:54 AM, Stas Malyshev > […] > > > […] > > I've updated the test case with such an example. > > PHP-5.6 and master have been updated. > > > > > […] > > > > -- > -- > Tjerk --- ## #73440 — Tjerk Meesters — 2014-03-26T04:14:55+00:00 URL: https://externals.io/message/73309#73440 · In reply to: #73426 Hi Levi, On Tue, Mar 25, 2014 at 9:51 PM, Levi Morrison wrote: > In a similar vein have we checked `offsetSet`? It seems `offsetSet` is not > called with `+=`; I am not sure if this is intended; see > https://bugs.php.net/bug.php?id=66641 > This is due to how binary assignment is applied to properties vs arrays: http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_vm_execute.h#31979 The workaround here is to disable the `get_property_ptr_ptr` inside `spl_array.c`, like this: https://gist.github.com/datibbaw/703597322ad411287461 Not sure if everyone will agree to that change, but it's one way to fix the bug other than changing the API itself :) > > > On Tue, Mar 25, 2014 at 4:38 AM, Tjerk Meesters wrote: > > > […] > > -- -- Tjerk --- ## #73441 — Tjerk Meesters — 2014-03-26T04:56:06+00:00 URL: https://externals.io/message/73309#73441 · In reply to: #73440 On Wed, Mar 26, 2014 at 12:14 PM, Tjerk Meesters wrote: > Hi Levi, > > On Tue, Mar 25, 2014 at 9:51 PM, Levi Morrison wrote: > > > […] > > This is due to how binary assignment is applied to properties vs arrays: > > http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_vm_execute.h#31979 > > The workaround here is to disable the `get_property_ptr_ptr` inside > `spl_array.c`, like this: > > https://gist.github.com/datibbaw/703597322ad411287461 > The test suite proved me wrong about this, as bug65328.phpt fails and causes some memory leaks when applied; some tweaks may be required or an altogether different approach :) > > > Not sure if everyone will agree to that change, but it's one way to fix > the bug other than changing the API itself :) > > > > […] > > > -- > -- > Tjerk > -- -- Tjerk