Re: [PR 614] ArrayObject and isset() / empty() behaviour

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? On Sat, Mar 8, 2014 at 12:12 AM, Etienne Kneuss <colder@php.net> wrote: > > > > On Fri, Mar 7, 2014 at 4:58 PM, Etienne Kneuss <colder@php.net> wrote: > >> >> >> On Fri, Mar 7, 2014 at 3:15 PM, Julien Pauli <jpauli@php.net> wrote: >> >>> On Fri, Mar 7, 2014 at 11:14 AM, Tjerk Meesters >>> <tjerk.meesters@gmail.com> 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: >>> >> >> 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 > -- -- Tjerk