Re: Merge PR 621
oops, few typos.
On Thu, Mar 20, 2014 at 5:26 PM, Etienne Kneuss <colder@php.net> wrote:
>
>
>
> 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()
>
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()
>
isset() -> has_dimension() && read_dimension() !== NULL ->
offsetExists() && offsetGet() !== NULL
empty() -> !has_dimension() || !read_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
>
--
Etienne Kneuss
http://www.colder.ch
Thread (23 messages)