Re: Merge PR 621

On Thu, Mar 20, 2014 at 9: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. 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