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