Skip to content

Conversation

@Girgias
Copy link
Member

@Girgias Girgias commented Aug 8, 2025

@Girgias Girgias requested a review from kocsismate as a code owner August 8, 2025 21:04
@Girgias Girgias merged commit 1687231 into php:master Aug 9, 2025
9 checks passed
@Girgias Girgias deleted the 8.5-dep-spl-object-storage branch August 9, 2025 10:36
sorinsarca pushed a commit to opis/closure that referenced this pull request Oct 14, 2025
* Prefer `offsetSet` over deprecated `attach`

`SplObjectStorage::attach()` is deprecated since
PHP v8.5; `SplObjectStorage::offsetSet()` should
be used instead [1].

[1]: php/php-src#19424
@dg
Copy link

dg commented Nov 3, 2025

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.

  1. 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.

  2. 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants