Re: Merge PR 621

Sorry, I hit "Send" too soon. On Thu, Mar 20, 2014 at 10:36 AM, Tjerk Meesters <tjerk.meesters@gmail.com>wrote: > > > > 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 > I forgot to mention that the current behaviour is not necessarily correct if offsetGet() is overridden in a child class; the overridden method may decide to return null or something empty. > 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 > -- -- Tjerk