Re: Merge PR 621
| From: | Etienne Kneuss | Date: | Thu, 20 Mar 2014 16:26:45 +0000 |
| Subject: | Re: Merge PR 621 | ||
| References: | 1 2 3 4 | Groups: | php.internals |
| Request: | Send a blank email to internals+get-73339@lists.php.net to get a copy of this message | ||
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