Re: Merge PR 621
On Thu, Mar 20, 2014 at 9:16 AM, Stas Malyshev <[email protected]>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.
Here's my earlier work on this: https://github.com/php/php-src/pull/614
It currently "doesn't work" because isset()/empty() work like
array_key_exists() on objects that extend ArrayObject, which is in some
cases not the desired behaviour; see also:
http://lxr.php.net/xref/PHP_5_6/ext/spl/spl_array.c#604
If 'offsetExists()' is not overridden, the logic is done correctly; see
also: http://lxr.php.net/xref/PHP_5_6/ext/spl/spl_array.c#617
The behaviour is currently correct inside zend_std_has_dimension(), so
making the fix in SPL alone (#614) would make performance "suffer" less
unless offsetGet() is overridden.
Personally I'm fine either way; like I said, my first patch was only done
in SPL when Etienne suggested the approach in #621.
> 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.
>
> --
> Stanislav Malyshev, Software Architect
> SugarCRM: http://www.sugarcrm.com/
> (408)454-6900 ext. 227
>
--
--
Tjerk
Thread (23 messages)