array_key_exists BC break View as Markdown

I applied this to HEAD

Oops. I mean to PHP_5_3 (keep forgetting that it's not HEAD).

S

Em Qui, 2008-11-06 às 10:10 -0800, Stanislav Malyshev escreveu:

Hi!

So as suggested and wished, here is a patch that add a modifier '%' to
'a' in parameter parsing API, where it allows object that implements
ArrayAccess to be accept. Although it doesn't invoke any their methods,
i.e. just how it works nowdays.

I think if you use HASH_OF then any object having get_properties should
be fine... Is there any reason to restrict it to ArrayAccess?

Well, there is no reason, I was only following what was said.
It would make more sense in case of real BC break avoiding.

A list of functions that allow object on current 5.2:
. uksort
. uasort
. usort
. natcasesort
. natsort

Interestingly, this list includes no sort(). Is it intentional? Also, we
have a bunch of array functions - wouldn't it make sense that all of
them behaved the same way towards objects?

Yep, this was really interesting, see:

$x = new stdclass;
$x->foo = 1;
sort($x);

Warning: sort() expects parameter 1 to be array, object given ...

As other functions that I guess you must be remembered. :)

And yes, I have checked each array function.

--
Regards,
Felipe Pena

Hi!

So as suggested and wished, here is a patch that add a modifier '%' to
'a' in parameter parsing API, where it allows object that implements
ArrayAccess to be accept. Although it doesn't invoke any their methods,
i.e. just how it works nowdays.

I think if you use HASH_OF then any object having get_properties should
be fine... Is there any reason to restrict it to ArrayAccess?

This can be wrong/strange with classes implementing ArrayAccess (not
ArrayObject) or Iterator.

An example where one might expect an Iterator, reset() and next() use
HASH_OF in 5.2:

<?php
class I implements Iterator {
public $foo = "some real property";

public function `rewind()` {
    echo __METHOD__, "\n";
}
public function `current()` {
    echo __METHOD__, "\n";
    return "from iterator";
}
public function valid() {
    static $continue = true;

    echo __METHOD__, "\n";
    if ($continue) {
        $continue = false;
        return true;
    } else {
        return false;
    }
}
public function `next()` {} public function `key()` {}

}

$i = new I;

reset($i);
echo current($i);
?>
some real property

And another example using ArrayAccess: array_key_exists uses HASH_OF in
5.2:

<?php
class A implements ArrayAccess {
public $foo = 42;
function offsetExists($k) {
echo METHOD; return false;
}
function offsetGet($k) {}
function offsetSet($k, $v) {}
function offsetUnset($k) {}
}
var_dump(array_key_exists("foo", new A()));
?>
bool(true)

So allowing passing objects to array stuff using HASH_OF can lead to
unexpected behavior. Dropping the old behavior makes this clear ... but
is bad for stuff like ArrayObject.

I'm not sure whether we should add special rules for a single class
though ..

johannes

Hi!

Can anyone write up a summary of the situation and the options we have?

Sure. A number of array functions in 5.2 accept arrays, but use HASH_OF
to extract array value, which means they automatically accept objects if
the object has working get_properties handler. In 5.3, such function use
new API with 'a' parameter, which accepts only arrays proper.
The proposal is to have some new parameter - say 'a%' - which would call
HASH_OF on objects where it makes sense. The definition of "makes sense"
is not clear yet, since, for example, sort() on some objects may make
sense - if the object's get_properties returns real storage - or may
not, if that handler returns the mere copy of the real data. We might
use some interface as a marker - like ArrayAccess - though I'm not sure
if it will work in all circumstances.

Also cant we some how automate the BC break testing for this (aka scan
all functions that accept an array with the old API in 5.2, pass it an
ArrayObject instance and see what happens and compare that to 5.3)?

Yes we can ;) That should be not hard to do by looking at ones having
'a' in parameters in 5.3 and comparing how they react to arrays in 5.2
and in 5.3. Some of them, btw, might accept objects even though it
doesn't make much sense - as above.

Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com

2008/11/17 Andrei Zmievski andrei@gravitonic.com:

I meant '&' actually.

-Andrei

Ok, patch updated. http://felipe.ath.cx/diff/array_bc.diff

Go go go?

The rest is to fix 25 (at least ext/standard/array/) tests failing like:
343+ Warning: current() expects parameter 1 to be array or object,
null given in %s on line %d
343- Warning: current() expects parameter 1 to be array, null given in
%s on line %d

--
Regards,
Felipe Pena

Hi!

. end
. prev
. next
. reset
. current
. key

I think those should be more friendly with Iterators, but for other
objects probably not much sense to allow it. In 5.2 they modify or use
current hash pointer, but for objects that doesn't make too much sense.

Using iterators there might make sense, but two things to consider:

• prev() doesn't map to Iterator, so we might leave it out
• Doing that is a (silent) change in behavior, not going to the
  property's hashtable but the iterator interface, not sure if it
  hurts anybody, though .. language-wise I guess it makes more sense to
  allow iterators

. array_key_exists

Here it may make some sense, though I guess it should maybe go through
more specific handlers (i.e. has_dimension or has_property?). Also,
feels weird that key_exists works but in_array doesn't.

. array_unique
. array_flip
. array_walk_recursive
. array_walk

Those are ok with any object, in theory, though it's not clear why you
could do array_flip on object but not array_reverse or
array_keys/values. Maybe we should not have object support from those,
except array_walk_* for Iterators? If you want something else, get
properties manually...

I don't think we should pick a single function to do iterator stuff,
especially as there's iterator_apply() doing the same thing for some
time.

. uksort
. uasort
. usort
. natcasesort
. natsort

These frankly make little sense with objects and also other sorts do not
support object, so I'd say this is just a bug that they allow objects at
all.

I agree.

johannes

Attached is the patch that should restore BC for all those functions
except sorts and array_unique/array_flip (these should be trivial too
but I still thing their support for objects is just a bug). Please tell
if you are using those (ones not BCed) in any meaningful scenario on
objects.

This is done by having 'a' and 'h' specifiers have sister ones 'A' and
'H' that would accept objects if those have working HASH_OF. I have
opted out for new letters and not sign since signs currently used for
generic modifiers (like "separate it") and object-array relation is not
generic.
Any comments?

Only one comment: Could you please update README.PARAMETER_PARSING, too?
Thanks!

johannes