ext/spl: Fix various ArrayObject bugs

[Girgias]

This fixes:

• Dynamic property deprecation warning/readonly error not emitted when modifying through new ArrayObject($targetObj); #9432
• new ArrayObject($targetObject) allows setting typed properties to invalid types #9434
• Throwing an error when appending a backing object via $ao[] = $v
• Writing inaccessible private/protected properties
• And other bugs...

ArrayObject is dealing with the property HashTable directly, this is not much of an issue for read, isset, and empty operations.

However, write operations are very different in that classes can impose restriction on those, ranging from visibility, types and dynamic properties.

Moreover, for some unknown reason, ArrayObject does not use the backing class overloaded property access magic methods and tests for this. Meaning, we cannot just use the standard write_property handler of the CE, thus we need to reimplement it ourself to dodge the __set() call.

Commit 1 is just code rearranging so that I could navigate this file more easily...

[arnaud-lb]

This should be done in the !offset branch as well: https://3v4l.org/AYJjF

[Girgias]

Not sure if I understand what you are asking me to do here?

[arnaud-lb]

In the if (!offset) { branch above, you need to call offsetSet() if there is such method on the object. https://3v4l.org/AYJjF shows that offsetSet() used to be called during $arrayObject[] = ...;

[arnaud-lb]

With the code at https://3v4l.org/lO4Tf, this is the output on master:

string(12) "A::offsetSet"
string(12) "B::offsetSet"

and this is the output on this branch:

string(12) "A::offsetSet"

Is this an expected behavior change? If not, I think that we should move the if (check_inherited && intern->fptr_offset_set) { block above if (!offset) {

[Girgias]

Huuuum I think it is slightly problematic that an object that implements ArrayAccess supports appending as it is indistinguishable from $o[null] but this is something I should add tests for in #12723

But I do agree that ArrayObject subclassing and ArrayAccess implementations should behave identically. Thanks for spotting this, I'm amending this and adding a test :)

[bukka]

I don't think this can go to 8.3 as it is quite late for such potentially breaking fixes considering that PR is also not ready at this stage (object leak and failed CI).

Especially the append bit is a BC break even though I agree that it should be fixed but I would prefer 8.4 instead as this should be for sure in UPGRADING. Note that at this stage we should no longer do any changes that require adding note to UPGRADING.

I would also prefer to add the Dynamic properties change to upgrading. You could argue that this should be done before and it is just an omission which is true but it wasn't done and users might be suprised if they see such change so I think each such changes should be explicitly mentioned in UPGRADING (deprecated functionality). You can see that this is already impacting one standard test so I guess some users will be impacted by this.

[arnaud-lb]

The code looks good to me except for the behavior change when offsetSet() is overridden (#12037 (comment))

[arnaud-lb]

I have mixed opinions about the change in 7e46992. On one hand it seems sensible and allows sub-classes to differentiate $o[null]= from $o[]=, but on the other hand it introduces an inconsistency in ArrayAccess handling and a breaking change.

I would like to see what others think.