Re: Merge PR 621

On Tue, Mar 25, 2014 at 11:34 AM, Stas Malyshev <smalyshev@sugarcrm.com>wrote: > Hi! > > > Do you have any issues with the code as it stands now? > > > > https://github.com/php/php-src/pull/614 > > Something I don't understand in this patch: so if we don't have "has" > override, we'd get value == NULL and then we go and check internal > hashtable. Shouldn't we first check the get override and ask it with > BP_VAR_IS, and only if get override is not there use direct hash access? > It's my understanding that you should (have to) only call "get" if you know that the offset exists. > > E.g. imagine this: > > class SecretArrayObject extends ArrayObject > { > public function offsetGet($offset) { > var_dump('Called: '.__METHOD__); > return parent::offsetGet(str_rot13($offset)); > } > public function offsetSet($offset) { > var_dump('Called: '.__METHOD__); > return parent::offsetSet(str_rot13($offset)); > } > } > > I think on this example your code may fail to go to correct items when > doing isset/isempty, since if I ask for 'qux' it would actually check > the value I've put there for 'dhk'. > This particular class should override offsetExists($offset) as well, i.e.: public function offsetExists($offset) { return parent::offsetExists(str_rot13($offset)); } > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ > (408)454-6900 ext. 227 > -- -- Tjerk