Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:124349
Date: Wed, 10 Jul 2024 15:04:58 +0000
To: Levi Morrison <levi.morrison@datadoghq.com>
Cc: PHP internals <internals@lists.php.net>
Subject: Re: [PHP-DEV] [RFC] Improve language coherence for the behaviour of offsets and containers
Message-ID: <jBKyhPrjSX1cYulRZtMOtcc96dae07Vpn3gg56sEX3V8nB5PNiI8IYE0hkbQZDOlH3gCLm93ofbh3Y4y9CCZ6-pD6ql0zUS6JDA6S2EJqGU=@gpb.moe>
In-Reply-To: <CAAKU0umROg0TLOnA0dLpXVOLfV7rOjUhKaYXouTx-dV=CDtR3Q@mail.gmail.com>
References: <mhbeurk_htxbQxzIEs98wQoR8Xczf-L7L8aCAIVhATD7wQiEUJwpAYxYS-llyac0pN761mnQJfpVQcDUoXAgasaWpWylpt6LE-I2oM4zrbk=@gpb.moe> <EE-aMte3i-_DTmSrqDgoaRPATmjLUcLt-k31baT8lqqCsrzIy0UsOyKo9rlBs51gWKUK7e7X3VYDuJmxhd6bxOGe52k8ITQgoN0_AE0RnoY=@gpb.moe> <CAAKU0umROg0TLOnA0dLpXVOLfV7rOjUhKaYXouTx-dV=CDtR3Q@mail.gmail.com>
Feedback-ID: 96993444:user:proton
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From: internals@gpb.moe ("Gina P. Banyard")

On Wednesday, 10 July 2024 at 01:52, Levi Morrison <levi.morrison@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 no=
t exactly sure how I am meant to interpret the lack of responses.
>=20
>=20
> 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.
>=20
> 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/5c7603f420e3a75a5750b3e54cc=
95dfdbef7d6e2/src/Line.php#L166
> 2. https://github.com/ParvulaCMS/parvula/blob/dcb1876bef70caa14d09e212838=
a35cb29e23411/core/Models/Config.php#L46
>=20
> 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:
>=20
> $container->append($item);
>=20
>=20
> I expect largely they will do:
>=20
> $container[] =3D $item;
>=20
> 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?
>=20
> 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. Spl=
ObjectStorage,
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::c=
ontains(), 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, introducin=
g it for append() doesn't make sense to me.

It should be noted, we have other internal classes that have an append() me=
thod that is compatible with the current interface design.
One such example is AppendIterator, currently it doesn't implement ArrayAcc=
ess 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 me=
thods names for the same thing being aliased, and just thinking about it ag=
ain 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 offse=
t, which to me makes this a bad name.

Finally, the whole point of ArrayAccess is to make container types feel mor=
e like a normal array, so it seems perfectly normal for me to have append b=
e the name of it.
Yes there will be some classes that are incompatible with this interface, b=
ut 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 custo=
m container.
Both such custom container type implementations would be able to use the in=
terface 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 typ=
e (truly sad the lack of generics) to allow people to use the $container[] =
=3D $value syntax with said class.
Requiring a lot of projects to duplicate the implementations of their metho=
ds just to support this, and possibly hitting the problem we face with Arra=
yObject 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 abl=
e 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 val=
ue by reference to the append() method, which seems to indicate that they n=
eed to implement fetchAppend() to grab a reference and then assign the valu=
e.

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 n=
eeding 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