Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:73339
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 74.125.82.174 as permitted sender)
MIME-Version: 1.0
Sender: ekneuss@gmail.com
In-Reply-To: <532A418A.8020607@sugarcrm.com>
References: <CAHMUw2EeYA3m4QnRtLYm1PRa3taGiS8Sx5GxdN==ZYTQoFV=vQ@mail.gmail.com>
 <532A3E88.20202@sugarcrm.com> <ECAD39ED-2A1D-4B54-80D5-4CB4FBA71364@gmail.com>
 <532A418A.8020607@sugarcrm.com>
Date: Thu, 20 Mar 2014 17:26:45 +0100
Message-ID: <CACQesk74e3=FNd4KgC4XDHXiq97yk8KXRXAvV3tqRWOzFwN_pw@mail.gmail.com>
To: Stas Malyshev <smalyshev@sugarcrm.com>
Cc: Tjerk Meesters <tjerk.meesters@gmail.com>, PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=f46d0442807005f07e04f50c3e34
Subject: Re: [PHP-DEV] Merge PR 621
From: colder@php.net (Etienne Kneuss)

--f46d0442807005f07e04f50c3e34
Content-Type: text/plain; charset=UTF-8

On Thu, Mar 20, 2014 at 2:16 AM, Stas Malyshev <smalyshev@sugarcrm.com>wrote:

> 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.
>

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

--f46d0442807005f07e04f50c3e34--