Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:127589 X-Original-To: internals@lists.php.net Delivered-To: internals@lists.php.net Received: from php-smtp4.php.net (php-smtp4.php.net [45.112.84.5]) by lists.php.net (Postfix) with ESMTPS id ED3D01A00BC for ; Wed, 4 Jun 2025 15:30:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=php.net; s=mail; t=1749050897; bh=CSxt27Hf+doYR92pf1LaNcmyudymdWu/pIbpub3JSnU=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=kzJR8vkUo4+9iKlwankh4S+QAyFIfYgnMXT3CQy4diK/NGgqDBNTvWVfZoYeLyMnZ So5LDJbHA8PSaNm0cWIbtE9RGCu94crOmsASjEmqQoIrt4hLLzbc3xz6HzbM6upLXc 4z+NuKBprVFB6llySfG4IPaGEPsDD6TFrnDLGmuAi6/0KYk2gS+6AsPocbUa5xX/Vg xJcVDagdFyaLVeb3ROYAvfLwQas581t1XWrpLzMOGEWlRlmUihLvJgZj8P5kZ0tGZO MR9w4KjRtHih4f0h7cfVqobS8AMr96nbksFoce6Dm4tQxqGsF2caokrMXuaTNXEzcc 2n9xoXhQNXjGg== Received: from php-smtp4.php.net (localhost [127.0.0.1]) by php-smtp4.php.net (Postfix) with ESMTP id AC13B180047 for ; Wed, 4 Jun 2025 15:28:16 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on php-smtp4.php.net X-Spam-Level: X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_40,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,DMARC_PASS,FREEMAIL_FROM, HTML_MESSAGE,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=4.0.1 X-Spam-Virus: Error (Cannot connect to unix socket '/var/run/clamav/clamd.ctl': connect: Connection refused) X-Envelope-From: Received: from mail-lf1-f50.google.com (mail-lf1-f50.google.com [209.85.167.50]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by php-smtp4.php.net (Postfix) with ESMTPS for ; Wed, 4 Jun 2025 15:28:16 +0000 (UTC) Received: by mail-lf1-f50.google.com with SMTP id 2adb3069b0e04-5532a30ac41so7961503e87.0 for ; Wed, 04 Jun 2025 08:30:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1749051018; x=1749655818; darn=lists.php.net; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=7MxLcFXg4/xIoM+iDBJvJGrElYarDCTpAIv4kWCenng=; b=NAfjfFHKZQA9y+mAhKGfnhhve0aELWXXeiDTb4hbw94TN96ST2vu1ynS5M+6Nm2hdL FFYjwKC2d9YZ0V0b5fJc+VUw+/mEj7tZ1aroXvgFSoFYXdaO9UhzE5S5+otnYSFAvpTv UhGFZEaL8ImWjaIQMIKAT3crqwMWTRQS615Zlly7pDiNwUUS8Nkv6kgjuAF/MpeELr8N jQAEZ3qugHxB7/AtlHFKI6b3D11f64q2z8JrOfNtxPlWkSCqkyUTgBce6Eb5gAA6kLyS SkmF7G45shgHV/JygQoSFgwr86LrCuByEdVabfTtQF5XQhu68Qj+mkbtpxCULwPxdTzE abhw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1749051018; x=1749655818; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=7MxLcFXg4/xIoM+iDBJvJGrElYarDCTpAIv4kWCenng=; b=aIZtac/othSDeEZ3CMXMIvsWAJnkmxRxdjnEWUab+XHjoR4E7xXFlafHLF8ipakBzG dJQ2fhVNtHrcKZe2e+JYKnG68oZ2MorQQASqtJCVX1oeMBBxWpG751h5z9nZNykCgqXk Vc1uOQleyYVHHmNfd9TkYbDpf0cqONMpyUYKSPeOIL+IREyG5OBs6ysuTJmGkPL+a2kY 9PRboZD6tuIhP+tEvrt0fzMHOQxYSugLzULNpdcOwRrzGsz7KW0JW7qQNkATAD5zpqR6 reBdRshnz1oQiuYTIksTv685ErKPXY7szV6zSXDP2hPlDNHIFbAEwpqgKysOzmc01QJF PSww== X-Forwarded-Encrypted: i=1; AJvYcCXmxutIYdqaJ0nZToP8W4eU1nyqP4jr2hZEhm9klGBDMji+La+l6R+31n+u1Nf7bNdw6HA6oOnyTwk=@lists.php.net X-Gm-Message-State: AOJu0Yyi/WPna/VM2mZFiQbI6J/z32UmZA/aYBH8592ZZTl5Ehdu/WTD 7oouekjRr3wHdO5CEJYW1semOB0Nq6RAYIu/aA8rXHKS6WSkzhPzESw2sj6ERtDP1oEgO1LXUd7 eUx67P3KBnOTB+7r38XGuuwRrzKILTxVloyfuBv0= X-Gm-Gg: ASbGncsc4BdGKkAte/ok9BFkvRt8IEoetG1TrLkK6RglxDNWmyCntIEboVkt5Oniflh eeqg+4Nx/OaeE3h/cPjhfdRPzYAvyAtUlj5G6Rj/e3ZdnhZHQm+HKEmbhe5VcEfaM8VXOm9UfI9 iR//9U/cXjSBbuwJny0vYwyZwdYTffDAnNvMXC4RleLY8= X-Google-Smtp-Source: AGHT+IFWUS6bDLyn4D5mHyCS7GYKlnKkASbo5ZeNh51fXySLIbdyFcnBd008I4FIZyqMQoRbsW+Hs3KvvGDQJfv4kxY= X-Received: by 2002:a05:651c:30d6:b0:30b:f274:d1e2 with SMTP id 38308e7fff4ca-32ac78ee63cmr10194801fa.1.1749051018330; Wed, 04 Jun 2025 08:30:18 -0700 (PDT) Precedence: bulk list-help: list-post: List-Id: internals.lists.php.net x-ms-reactions: disallow MIME-Version: 1.0 References: <58dd224548e23ef76ca7ad980a567e5b@bastelstu.be> In-Reply-To: <58dd224548e23ef76ca7ad980a567e5b@bastelstu.be> Date: Wed, 4 Jun 2025 17:30:05 +0200 X-Gm-Features: AX0GCFvgEOd27iFKao7VEPOQbiYFoQKKX6_e_TllJDd4DdIsKsiaXsF_UZ_ZXEY Message-ID: Subject: Re: [PHP-DEV] Re: [RFC] Clone with v2 To: =?UTF-8?Q?Tim_D=C3=BCsterhus?= Cc: Volker Dusch , php internals Content-Type: multipart/alternative; boundary="000000000000a446c90636c0acaa" From: nicolas.grekas+php@gmail.com (Nicolas Grekas) --000000000000a446c90636c0acaa Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Le mer. 4 juin 2025 =C3=A0 15:33, Tim D=C3=BCsterhus a = =C3=A9crit : > Hi > > Am 2025-06-03 16:24, schrieb Nicolas Grekas: > > - We decided to __clone before updating properties to avoid BC issues. > >> > > > > Not sure which BC issues you've in mind, especially as that's a new > > feature. > > As I see it, before or after wouldn't change anything as far as __clone > > implementations are concerned. > > While clone-with is a new feature, it is automatically supported on > every class. Currently calling `__clone()` is the first thing that > happens after cloning the object and the implementation of `__clone()` > might rely on the properties being unchanged from the original object. > If the reassignment of properties would happen before the call to > `__clone()` using clone-with with classes that have such a `__clone()` > implementation might result in subtle and hard to debug issues. > This argument is totally symmetric: currently calling __clone is both the first and the last thing that happens. There can be implementations that rely on properties being unchanged after the call to __clone, eg: function __clone() { $this->foo =3D clone $this->foo; $this->fooId =3D spl_object_id($this->foo); } The example is silly but that's to highlight the symmetry. We spotted that passing $this to __clone would allow inspecting the changes and implementing deep-cloning protection, but only if calling __clone happens after. My concern is that as is, the RFC would block this possible future improvement - because then, it would be a non-BC behavior change. > > That's a very common mistake about readonly: it's also very useful for > > mutable classes. Here is an example that the Symfony Request/Response > > objects might follow one day: > > > > class Response > > { > > public function __construct( > > public string $content =3D '', > > public *readonly* ArrayObject $headers =3D new ArrayObject(), > > ) { > > } > > > > public function __clone() { > > $this->headers =3D clone $this->headers; > > } > > } > > > > $r1 =3D new Response(); > > $r2 =3D clone $r1; > > $r3 =3D clone($r1, ['headers' =3D> new ArrayObject(['foo' =3D> 'bar'])]= ); > > > > Note how making "headers" readonly binds the headers to its owner > > object. > > That's very useful since it forbids swapping the headers object by > > another > > one in response objects. > > I might lack the Symfony background, but it's not clear to me how > preserving the object-identity, but not its contents is important here. > I'd like to note, though: > > - Given that ArrayObject allows for interior mutability, you could just > use `$r3->headers->exchangeArray(['foo' =3D> 'bar']);`. > - Exchanging the entire `ArrayObject` can only happen from within the > class itself, since `$headers` is implicitly `protected(set)`. > - You don't actually bind the `$headers` ArrayObject to the owner > object, since the ArrayObject might be referenced by multiple Response > objects, since passing it in the constructor is legal. The same would be > true if it would be `public(set)`. > Using ArrayObject was for a quick example. A stricter class can be implemented though but the deep-cloning issue would remain. "readonly" is also totally optional. I could have used a more trivial example: class Response { public ArrayObject $headers; public function __construct(array $headers) { $this->headers =3D new ArrayObject($headers); } public function __clone() { $this->headers =3D clone $this->headers; } } > > The issue I highlighted previously is that __clone will "clone > > $this->headers" even if the resulting value will be immediately trashed > > on > > the line that creates $r3. This can cascade to nested clones of course > > depending on what is being cloned. That's where I see a waste of > > CPU/mem > > "by-design", which is unavoidable at the moment. > > The same would be true if you reassign the property after the successful > cloning. > I was not suggesting that made any difference. This was about highlighting the deep-cloning issue. > > About the BC aspect I already shared a proposal that should avoid it. > > About > > the "very limited set of use cases", that's simply not true: people > > will > > start using the new syntax when consuming third-party classes, and > > authors > > of those classes will be "Sorry Outta Luck" when their users will start > > complaining about the perf hog I described, with no other options than > > telling "don't use that syntax, that perfhog is built-in"... > > I don't see that happening, because it would mean that folks suddenly > start modifying `public(set) readonly` properties during cloning. In > other cases, the RFC does not enable anything new. For non-readonly > properties, the user is already able to reassign (see above) and for > non-public(set) properties, the class itself controls both `__clone()` > and the property itself and thus can take the use-cases into account. On > other words, what Volker already said here: > > >> So no situation will be worse than it was before, while some use-cases > >> are > >> enabled now and some performance critical code will probably stay the > >> same > >> as it is now. > Fair, thanks for addressing this. > -------- > > > So my personal suggestion would to be for code that is doing custom > > things > >> during cloning and support PHP pre-clone-with as is and only refactor > >> once > >> 8.5 is the minimum requirement, in places where it improves the code. > >> > > > > I didn't answer Tim but he missed that in my example there is a second > > argument passed to clone(), which makes it a parse error currently. > > In that case you can use: > > if (PHP_VERSION_ID >=3D 80500) { > $b =3D \clone($a, ['foo' =3D> 'bar']); > } > > `\clone` is syntactically valid in PHP 8.0 or higher and a reference to > the function `clone` in the global namespace. Incidentally this kind of > cross-version compatibility is enabled by making `clone()` a function. > Nice, I got lost in possibilities and missed that, thanks. > > About the second aspect: if a class is designed with the API I gave as > > example above (that Response class), then the new syntax is going to be > > the > > only way to swap the headers while cloning. That means there will be > > cases > > where my snippet is going to be useful. I'm fine if you don't want to > > add > > If this syntax is the only way to swap headers while cloning, how would > the branch for PHP 8.4 or lower look like? As Volker suggested, you can > only switch to clone-with when PHP 8.5 is your minimum version, since > you would need the =E2=80=9Clegacy way of doing things=E2=80=9D for older= PHP versions > (and that one would still remain valid even with the changes this RFC > proposes). > Let's leave this as such, I'm fine with a wait-n-see approach to that concern. > > So what remains on my side is giving enough care to the deep-cloning > > case - > > see my previous messages of course. > > When building a solution it's important to understand the problem in > depth and given that we don't currently understand the problem (or even > see a problem), we are not in a position to build a solution. > Importantly though the RFC leaves sufficient design space for a =E2=80=9C= Future > Scope=E2=80=9D solution (e.g. one that passes the $withProperties array t= o > `__clone()` or the original object - which would enable your WeakMap > use-case but is orthogonal to clone-with). I have just added that to the > =E2=80=9CFuture Scope=E2=80=9D section: > > https://wiki.php.net/rfc/clone_with_v2?do=3Ddiff&rev2%5B0%5D=3D1748959452= &rev2%5B1%5D=3D1749034411&difftype=3Dsidebyside > I think you've got the issue: deep-cloning. It's the only real thing I'm talking about and it's missing from the RFC - well, except in the future scope now, but that's a bit light to raise proper awareness about the design choice IMHO; Also, the change doesn't tell about "or the original object", I was surprised to see that difference between your email and the RFC update. Any reason for that? This relates to my call-ordering concern above. Cheers, Nicolas --000000000000a446c90636c0acaa Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi

Le=C2=A0mer. 4 ju= in 2025 =C3=A0=C2=A015:33, Tim D=C3=BCsterhus <tim@bastelstu.be> a =C3=A9crit=C2=A0:
Hi

Am 2025-06-03 16:24, schrieb Nicolas Grekas:
> - We decided to __clone before updating properties to avoid BC issues.=
>>
>
> Not sure which BC issues you've in mind, especially as that's = a new
> feature.
> As I see it, before or after wouldn't change anything as far as __= clone
> implementations are concerned.

While clone-with is a new feature, it is automatically supported on
every class. Currently calling `__clone()` is the first thing that
happens after cloning the object and the implementation of `__clone()`
might rely on the properties being unchanged from the original object.
If the reassignment of properties would happen before the call to
`__clone()` using clone-with with classes that have such a `__clone()`
implementation might result in subtle and hard to debug issues.

This argument is totally symmetric: currently call= ing __clone is both the first and the last thing that happens. There can be= implementations that rely on properties being unchanged after the call to = __clone, eg:
function __clone() {
=C2=A0 =C2=A0 $this-&= gt;foo =3D clone $this->foo;
=C2=A0 =C2=A0 $this->fooId =3D= spl_object_id($this->foo);
}
The example is silly b= ut that's to highlight the symmetry.

We spotte= d that passing $this to __clone would allow inspecting the changes and impl= ementing deep-cloning protection,=C2=A0but only if calling __clone happens = after.=C2=A0My concern is that as is, the RFC would block this possible fut= ure improvement - because then, it would be a non-BC=C2=A0behavior change.<= /div>

=C2=A0
> That's a very common mistake about readonly: it's also very us= eful for
> mutable classes. Here is an example that the Symfony Request/Response<= br> > objects might follow one day:
>
> class Response
> {
>=C2=A0 =C2=A0 =C2=A0public function __construct(
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0public string $content =3D ''= ,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0public *readonly* ArrayObject $header= s =3D new ArrayObject(),
>=C2=A0 =C2=A0 =C2=A0) {
>=C2=A0 =C2=A0 =C2=A0}
>
>=C2=A0 =C2=A0 =C2=A0public function __clone() {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0$this->headers =3D clone $this->= ;headers;
>=C2=A0 =C2=A0 =C2=A0}
> }
>
> $r1 =3D new Response();
> $r2 =3D clone $r1;
> $r3 =3D clone($r1, ['headers' =3D> new ArrayObject(['fo= o' =3D> 'bar'])]);
>
> Note how making "headers" readonly binds the headers to its = owner
> object.
> That's very useful since it forbids swapping the headers object by=
> another
> one in response objects.

I might lack the Symfony background, but it's not clear to me how
preserving the object-identity, but not its contents is important here. I'd like to note, though:

- Given that ArrayObject allows for interior mutability, you could just use `$r3->headers->exchangeArray(['foo' =3D> 'bar'= ]);`.
- Exchanging the entire `ArrayObject` can only happen from within the
class itself, since `$headers` is implicitly `protected(set)`.
- You don't actually bind the `$headers` ArrayObject to the owner
object, since the ArrayObject might be referenced by multiple Response
objects, since passing it in the constructor is legal. The same would be true if it would be `public(set)`.

Usin= g ArrayObject was for a quick example. A stricter class can be implemented = though but=C2=A0the deep-cloning issue would remain. "readonly" i= s also totally optional. I could have used a more trivial example:

class Response
{
=C2=A0 =C2=A0 public ArrayObject $h= eaders;

=C2=A0 =C2=A0 public function __construct(= array $headers) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 $this->headers = =3D=C2=A0new ArrayObject($headers);
=C2=A0 =C2=A0 }

=C2=A0 =C2=A0= public function __clone() {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 $this->heade= rs =3D clone $this->headers;
=C2=A0 =C2=A0 }
}

<= /div>
=C2=A0
> The issue I highlighted previously is that __clone will "clone > $this->headers" even if the resulting value will be immediatel= y trashed
> on
> the line that creates $r3. This can cascade to nested clones of course=
> depending on what is being cloned. That's where I see a waste of <= br> > CPU/mem
> "by-design", which is unavoidable at the moment.

The same would be true if you reassign the property after the successful cloning.

I was not suggesting that made= any difference.
This was about highlighting the deep-cloning iss= ue.

=C2=A0
> About the BC aspect I already shared a proposal that should avoid it. =
> About
> the "very limited set of use cases", that's simply not t= rue: people
> will
> start using the new syntax when consuming third-party classes, and > authors
> of those classes will be "Sorry Outta Luck" when their users= will start
> complaining about the perf hog I described, with no other options than=
> telling "don't use that syntax, that perfhog is built-in"= ;...

I don't see that happening, because it would mean that folks suddenly <= br> start modifying `public(set) readonly` properties during cloning. In
other cases, the RFC does not enable anything new. For non-readonly
properties, the user is already able to reassign (see above) and for
non-public(set) properties, the class itself controls both `__clone()`
and the property itself and thus can take the use-cases into account. On other words, what Volker already said here:

>> So no situation will be worse than it was before, while some use-c= ases
>> are
>> enabled now and some performance critical code will probably stay = the
>> same
>> as it is now.

Fair, thanks for= addressing this.
=C2=A0
--------

> So my personal suggestion would to be for code that is doing custom > things
>> during cloning and support PHP pre-clone-with as is and only refac= tor
>> once
>> 8.5 is the minimum requirement, in places where it improves the co= de.
>>
>
> I didn't answer Tim but he missed that in my example there is a se= cond
> argument passed to clone(), which makes it a parse error currently.
In that case you can use:

=C2=A0 =C2=A0 =C2=A0if (PHP_VERSION_ID >=3D 80500) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0$b =3D \clone($a, ['foo' =3D> = 'bar']);
=C2=A0 =C2=A0 =C2=A0}

`\clone` is syntactically valid in PHP 8.0 or higher and a reference to the function `clone` in the global namespace. Incidentally this kind of cross-version compatibility is enabled by making `clone()` a function.
<= /blockquote>

Nice, I got lost in possibilities and misse= d that, thanks.
=C2=A0
> About the second aspect: if a class is designed with the API I gave as=
> example above (that Response class), then the new syntax is going to b= e
> the
> only way to swap the headers while cloning. That means there will be <= br> > cases
> where my snippet is going to be useful. I'm fine if you don't = want to
> add

If this syntax is the only way to swap headers while cloning, how would the branch for PHP 8.4 or lower look like? As Volker suggested, you can only switch to clone-with when PHP 8.5 is your minimum version, since
you would need the =E2=80=9Clegacy way of doing things=E2=80=9D for older P= HP versions
(and that one would still remain valid even with the changes this RFC
proposes).

Let's leave this as such= , I'm fine with a wait-n-see approach to that concern.
=C2=A0=
> So what remains on my side is giving enough care to the deep-cloning <= br> > case -
> see my previous messages of course.

When building a solution it's important to understand the problem in depth and given that we don't currently understand the problem (or even=
see a problem), we are not in a position to build a solution.
Importantly though the RFC leaves sufficient design space for a =E2=80=9CFu= ture
Scope=E2=80=9D solution (e.g. one that passes the $withProperties array to =
`__clone()` or the original object - which would enable your WeakMap
use-case but is orthogonal to clone-with). I have just added that to the =E2=80=9CFuture Scope=E2=80=9D section:
https://wiki.php.net/rfc/clone_with_v2?do= =3Ddiff&rev2%5B0%5D=3D1748959452&rev2%5B1%5D=3D1749034411&difft= ype=3Dsidebyside

I think you've got the issue: deep= -cloning.
It's the only real thing I'm talking about and = it's missing from the RFC - well, except in the future scope now, but t= hat's a bit light to raise proper awareness about the design choice IMH= O;

Also, the change doesn't tell about "o= r the original object", I was surprised to see that difference between= your email and the RFC update. Any reason for that?
This relates= to my call-ordering concern above.

Cheers,
<= div>Nicolas
--000000000000a446c90636c0acaa--