Re: Merge PR 621

On Thu, Mar 20, 2014 at 9:05 PM, Stas Malyshev <smalyshev@sugarcrm.com>wrote: > Hi! > > > The current behavior is: > > > > isset() -> has_dimension(check_exmpty=false) -> offsetExists() > > empty() -> !has_dimension(check_exmpty=true) -> !offsetExists() > > || offsetGet() > > I think isset part is wrong - it should use offsetGet (probably via > check_empty=true) and then verify that the result is not null. > > > 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() > > I'm not sure this is the right way to go. It changes the existing API > without any need - while it can be handled within the existing API as well. > I agree that the API change is not in fact needed. We could limit ourselves to changing the behavior of the default has_dimension handler. It is also probably faster to do everything in has_dimension. I still think the "new version" is clearer, but it might not be worth the hassle of forcing this API change. The important part is that isset() triggers read_dimension and eventually offsetGet() for isset(), and we seem to agree on that. > > 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. > > This is actually OK, isset should return for ArrayObject the same it > would return for array with the same data. > > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ > (408)454-6900 ext. 227 > -- Etienne Kneuss http://www.colder.ch