Re: Merge PR 621
On Thu, Mar 20, 2014 at 9:05 PM, Stas Malyshev <[email protected]>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
Thread (23 messages)