Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:73004
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.128.177 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CAHMUw2HAOpM8E93rEyo6YVQab36sYbi4ssLoQPX+Kk8MMEJ-nA@mail.gmail.com>
References: <CAHMUw2GjeDRveLK99Q8U08y-3zqftAN8mLKPt=yikjT2BrNdBQ@mail.gmail.com>
	<CAMUwpuTxoiR-QhniYsBAebec0rusiu+FHoXx2hY=VY=z9um_mQ@mail.gmail.com>
	<CACQesk5+Wvy_kY9qjR6Ma9ChFjyvRjJrXa9DQYjOh43_uNu3hA@mail.gmail.com>
	<CACQesk6jiYdzzibjzr2ba5XC3U1hSRCnj1gyaC1KNDJMaP1+sQ@mail.gmail.com>
	<CAHMUw2HAOpM8E93rEyo6YVQab36sYbi4ssLoQPX+Kk8MMEJ-nA@mail.gmail.com>
Date: Sat, 8 Mar 2014 17:56:28 +0800
Message-ID: <CAHMUw2HPbFKX5pds-BY0NLjyfX_LGONzYYL68t_nj7fDgObzBQ@mail.gmail.com>
To: Etienne Kneuss <colder@php.net>
Cc: Julien Pauli <jpauli@php.net>, PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=047d7b342d305b122d04f4156260
Subject: Re: [PHP-DEV] [PR 614] ArrayObject and isset() / empty() behaviour
From: tjerk.meesters@gmail.com (Tjerk Meesters)

--047d7b342d305b122d04f4156260
Content-Type: text/plain; charset=ISO-8859-1

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

--047d7b342d305b122d04f4156260--