ext/spl: Deprecate SplObjectStorage::contains(), SplObjectStorage::attach(), and SplObjectStorage::detach()

[Girgias]

RFC: https://wiki.php.net/rfc/deprecations_php_8_5#deprecate_splobjectstoragecontains_splobjectstorageattach_and_splobjectstoragedetach

[dg]

Folks, I'm very glad about the effort to Improve language coherence for the behaviour of offsets and containers because there really are many inconsistencies here. However, I'm concerned that this change is a step in the wrong direction.

1. SplObjectStorage has a truly poor API. It contains methods for iteration that, in my opinion, must confuse programmers. It implements ArrayAccess in an inappropriate way. The strangest thing is:

$storage[$obj] = 'value';

foreach ($storage as $key => $value) // $key = 'value' and $value = $obj

This is very unfortunate.

The only thing about the SplObjectStorage API that is truly nice, elegant, and correct are the attach(), contains(), and detach() methods. These are genuinely clear and understandable. And this change kills them.

2. Methods of the ArrayAccess interface are fundamentally not expected to be called directly. They are like internal methods. I don't even want them offered by autocomplete in editors, because their sole purpose is to enable syntactic sugar in the form of array-like access. This change directly encourages users to call the offset*() method.

3. The method names like offsetSet() are completely meaningless and unintuitive from the perspective of the Object Storage class. What offset?

I see this as a major step backward. If the goal is to solve the problem with method overriding in child classes, a better solution can certainly be found.

Therefore, I ask you to please keep the attach(), contains(), and detach() methods as they are until a better solution is found. // cc @Girgias

[danog]

+1 on @dg's comment, this is really counterintuitive, one of the most actively harmful and counterproductive deprecations in 8.5 IMO

[Girgias]

It is not possible to guarantee that the implementation of attach/detach/contains matches the implementation of offsetSet/offsetUnset/offsetExists as the class is not final.

Moreover, this is not the place to discuss language changes, if you want to revert this decision please submit an RFC.

[danog]

It is not possible to guarantee that the implementation of attach/detach/contains matches the implementation of offsetSet/offsetUnset/offsetExists as the class is not final.

IMO, this is not a bad enough problem to warrant removal of a clearly superior set of methods in terms of DX (actually, it's not really a problem, it's absolutely logical that if you don't override a method, its behavior doesn't change).

I won't be submitting an RFC to revert this, but I do feel like the recent deprecations in 8.5 really went overboard a bit, and I'd kindly recommend being a bit more considerate in future deprecations.