On Wednesday, 10 July 2024 at 01:52, Levi Morrison <levi.morri...@datadoghq.com> wrote:
> > Moreover, I know the traffic on the list has been pretty high, but I do > > intend to have this RFC up for voting for inclusion in PHP 8.4, and I'm not > > exactly sure how I am meant to interpret the lack of responses. > > > I am personally strongly in favor of the direction. As mentioned in > the PR, my main concern is honestly quite a small one: I think > `Appendable::append` ought to be renamed. Maybe `Appendable` and > `FetchAppendable` too. > > The reason is that `append` is a common operation on a container type, > which is likely to want to implement these interfaces. I easily > identified a few such things with a quick GitHub search: > 1. > https://github.com/pmjones/php-styler/blob/5c7603f420e3a75a5750b3e54cc95dfdbef7d6e2/src/Line.php#L166 > 2. > https://github.com/ParvulaCMS/parvula/blob/dcb1876bef70caa14d09e212838a35cb29e23411/core/Models/Config.php#L46 > > Given that I anticipate these methods to largely be called by > handlers, and not by names, I think an easy solution is to just name > this `offsetAppend` to match the other offset operations. For example, > I don't anticipate code doing: > > $container->append($item); > > > I expect largely they will do: > > $container[] = $item; > > So it doesn't really matter if the name is `append` or `offsetAppend` > for the main use-case, and thereby we avoid some road bumps on > adoption. Any SPL containers with `append`, such as ArrayObject, can > make it an alias of `offsetAppend`, I think? > > Anyway, this is a minor thing, and I will vote yes regardless of > whether it (and maybe the *Appendable interface names) are changed. > But I do think it would be prudent to change it. It would also match > the `offset*` convention of the other interfaces. Part of the RFC is to deprecate aliases to the dimension handlers, e.g. SplObjectStorage, because those classes are not final, and you can extend the aliased method and make it behave in whatever way you want. Which causes unintuitive bugs, because if you overwrite SplObjectStorage::contains(), which is currently considered the "canonical" method, you don't actually overwrite the behaviour of isset($obj[$index]) The documentation even indicates that offsetExists is an alias of contains() [1] Considering, one part of the RFC is to get rid of this aliasing, introducing it for append() doesn't make sense to me. It should be noted, we have other internal classes that have an append() method that is compatible with the current interface design. One such example is AppendIterator, currently it doesn't implement ArrayAccess because... that doesn't make sense, but it would be quite useful for it to implement the new Appendable interface (or whatever preferred name). Changing the name of the method on the interface would require introducing more aliases, something that I don't think is wise. (another class that could benefit from this is the Dom\Element class) Side note: we had issues in the past with SplFileObject having different methods names for the same thing being aliased, and just thinking about it again makes my brain hurt. Moreover, the name 'offsetAppend' is somewhat nonsensical, one appends to a *container*, the naming here implies to me that I am appending to an offset, which to me makes this a bad name. Finally, the whole point of ArrayAccess is to make container types feel more like a normal array, so it seems perfectly normal for me to have append be the name of it. Yes there will be some classes that are incompatible with this interface, but I would guess the majority of implementations of an append() method just take a single value, or a variadic number of values to append to the custom container. Both such custom container type implementations would be able to use the interface without any major changes. [2] >From a quick glance, I can see various classes of Symfony and Laravel that >could add an implementation of Appendable, and just widen the parameter type >(truly sad the lack of generics) to allow people to use the $container[] = >$value syntax with said class. Requiring a lot of projects to duplicate the implementations of their methods just to support this, and possibly hitting the problem we face with ArrayObject at a larger scale, seems ill-advised to me. For container types where appending means something more complex, then yes they cannot use the interface, but regardless of name they would not be able to. And I don't see how them continuing to use "append" as a method name causes problems. Thus, from my PoV, the only adoption issue is for classes that pass the value by reference to the append() method, which seems to indicate that they need to implement fetchAppend() to grab a reference and then assign the value. Everything considered, I still do not think changing the name of the append() method is a good idea, will it hurt adoption in the short term for some classes, yes, but I prefer this than having a large part of the ecosystem needing to create an alias just for some weird cases. Best regards, Gina P. Banyard [1] https://www.php.net/manual/en/splobjectstorage.offsetexists.php [2] https://3v4l.org/E8BcK
