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

On Sat, Mar 8, 2014 at 4:56 AM, Tjerk Meesters <tjerk.meesters@gmail.com>wrote: > On Sat, Mar 8, 2014 at 8:27 AM, Tjerk Meesters <tjerk.meesters@gmail.com > >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: > > > > ����LJ� Gw > > ��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? > > > > > > > > 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 > > > > > > -- > -- > Tjerk > Could you actually make that into a sane macro or would that just be more trouble than it's worth?