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

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: > > > 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