Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:73345
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 74.125.82.49 as permitted sender)
MIME-Version: 1.0
Sender: ekneuss@gmail.com
In-Reply-To: <532B4A1B.5090503@sugarcrm.com>
References: <CAHMUw2EeYA3m4QnRtLYm1PRa3taGiS8Sx5GxdN==ZYTQoFV=vQ@mail.gmail.com>
 <532A3E88.20202@sugarcrm.com> <ECAD39ED-2A1D-4B54-80D5-4CB4FBA71364@gmail.com>
 <532A418A.8020607@sugarcrm.com> <CACQesk74e3=FNd4KgC4XDHXiq97yk8KXRXAvV3tqRWOzFwN_pw@mail.gmail.com>
 <532B4A1B.5090503@sugarcrm.com>
Date: Thu, 20 Mar 2014 22:21:41 +0100
Message-ID: <CACQesk5W8H+NuZVeUprDOMK26szTQYtHFeT5XoWwYXLCb-8=Vw@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=047d7ba98294c037f304f5105ca4
Subject: Re: [PHP-DEV] Merge PR 621
From: colder@php.net (Etienne Kneuss)

--047d7ba98294c037f304f5105ca4
Content-Type: text/plain; charset=UTF-8

On Thu, Mar 20, 2014 at 9:05 PM, Stas Malyshev <smalyshev@sugarcrm.com>wrote:

> Hi!
>
> > The current behavior is:
> >
> > isset()   ->   has_dimension(check_exmpty=false)   ->   offsetExists()
> > empty()   ->   !has_dimension(check_exmpty=true)   ->   !offsetExists()
> > || offsetGet()
>
> I think isset part is wrong - it should use offsetGet (probably via
> check_empty=true) and then verify that the result is not null.
>
> > 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()
>
> I'm not sure this is the right way to go. It changes the existing API
> without any need - while it can be handled within the existing API as well.
>

I agree that the API change is not in fact needed. We could limit ourselves
to changing the behavior of the default has_dimension handler. It is also
probably faster to do everything in has_dimension.

I still think the "new version" is clearer, but it might not be worth the
hassle of forcing this API change.

The important part is that isset() triggers read_dimension and eventually
offsetGet() for isset(), and we seem to agree on that.


> > 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.
>
> This is actually OK, isset should return for ArrayObject the same it
> would return for array with the same data.
>
> --
> Stanislav Malyshev, Software Architect
> SugarCRM: http://www.sugarcrm.com/
> (408)454-6900 ext. 227
>



-- 
Etienne Kneuss
http://www.colder.ch

--047d7ba98294c037f304f5105ca4--