ObjectAccess behaviour broken on ArrayObject

[kdambekalns]

Description

When using getGettablePropertyNames() on an ArrayObject, the returned result includes not only the "real" properties of the object, but also internal ArrayObject properties. This affects getGettableProperties(), too.

Steps to Reproduce

$object = new \ArrayObject([
    'foo' => 'bar'
]);
var_dump(ObjectAccess::getGettableProperties($object));

Expected behavior

array(1) {
  ["foo"]=>
  string(3) "bar"
}

Actual behavior

array(4) {
  ["arrayCopy"]=>
  array(1) {
    ["foo"]=>
    string(3) "bar"
  }
  ["flags"]=>
  int(0)
  ["iterator"]=>
  object(ArrayIterator)#10459 (1) {
    ["storage":"ArrayIterator":private]=>
    object(ArrayObject)#10460 (1) {
      ["storage":"ArrayObject":private]=>
      array(1) {
        ["foo"]=>
        string(3) "bar"
      }
    }
  }
  ["iteratorClass"]=>
  string(13) "ArrayIterator"
}

Affected Versions

Flow: since ages ago…

[kitsunet]

I somewhat disagree with this being a bug, because we make no promises as of the quality of gettableProperties. Those are clearly the gettableProperties of this object.

[kdambekalns]

Well… I think it is really unexpected behaviour.

[kitsunet]

It does what it says on the "tin":

- which can be get through a public getter method.	     
- public properties which can be directly get.

It's a rather low level utility function that is intentionally simple in what it does, I think adding exceptions on this level is rather not a good idea, because that could lead to unintentional behaviour (as well). I would rather want to think about how we use it and if that's maybe wrong, if we come to the conclusion that this is the right place, sure, but at the moment I am not really convinced it is.

Apart from the change being breaking if your code relies on the existing behaviour.

[bwaidelich]

I would also expect ArrayObject (i.e. ArrayAccess) to behave like an array in this case..

But changing this core behavior would probably break quite a lot of code in a very unpleasant way

[albe]

I'm a bit undecided tbh. I would wish ArrayObject to behave like array for all means and purposes, but then again the API of that class is designed in a way that it has getters for getArrayCopy, getFlags, getIterator and getIteratorClass (only has setters for flag and iteratorClass though). So if we work around this, we would effectively "fix" (or rather "work around") a (bad) class design in the PHP core.

Iff we fully agree on that, let's fix it in a way that will not blow up too many things - possibly means target master only, which in turn means different behaviour between supported versions that needs to be documented.

[kdambekalns]

I came across this in the contect of mapping CR nodes, and the ArrayPropertyCollection that is used to hold properties now triggered this (used to be an array). So… maybe we can fix it elsewhere.

For my particular code I did add a $propertyMappingConfiguration->forProperty('*')->skipUnknownProperties(), but… 🙈

[bwaidelich]

We could just add a dedicated PropertyCollectionInterface to array converter

[albe]

That sounds plausible, but PropertyCollectionInterface is part of Neos, while ArrayConverter is Flow. So needs a new Converter, like the ArrayConverter in Neos.Media I guess.

[kdambekalns]

Ping… bringing this up again. If we decide to fix it in some breaking way, it could just make it for 8.0.

If not, should I add a new converter instead? Would be 8.0, too, but non-breaking.

[albe]

I'm somehow still in favor of a new converter.