Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:127584
Precedence: bulk
MIME-Version: 1.0
Date: Wed, 04 Jun 2025 15:33:42 +0200
To: Nicolas Grekas <nicolas.grekas+php@gmail.com>
Cc: Volker Dusch <volker@tideways-gmbh.com>, php internals
 <internals@lists.php.net>
Subject: Re: [PHP-DEV] Re: [RFC] Clone with v2
In-Reply-To: <CAOWwgp=h6BG=NB1KNasZfFBkDb=Ldi+KBaTLt-wCFzR4MzvWXA@mail.gmail.com>
References: <CAMzRQTdePFXUMWO38g=gQA2vbqL5zp=o1cPt00ec41g95xys=g@mail.gmail.com>
 <CAMzRQTe_vn1a-ji9M2NXnBgJm3eHOGTHozWpM1jGzOht_sNs4w@mail.gmail.com>
 <CAMzRQTe-moWS4acwUHesA=K_pb+z+J8H_aMHCesyZL3qjG7z-g@mail.gmail.com>
 <CAOWwgpkO8VasKSP5QMC0kwN58OihdTc+JeQ_s_XsNnJBZz9E7w@mail.gmail.com>
 <CAMzRQTesOuWirnY6E47bSdRKiq7xuTFLB72vi6ycncb6Y8F0uw@mail.gmail.com>
 <CAOWwgp=h6BG=NB1KNasZfFBkDb=Ldi+KBaTLt-wCFzR4MzvWXA@mail.gmail.com>
Message-ID: <58dd224548e23ef76ca7ad980a567e5b@bastelstu.be>
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
From: tim@bastelstu.be (=?UTF-8?Q?Tim_D=C3=BCsterhus?=)

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.

> 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 = '',
>         public *readonly* ArrayObject $headers = new ArrayObject(),
>     ) {
>     }
> 
>     public function __clone() {
>         $this->headers = clone $this->headers;
>     }
> }
> 
> $r1 = new Response();
> $r2 = clone $r1;
> $r3 = clone($r1, ['headers' => new ArrayObject(['foo' => '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' => '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)`.

> 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.

> 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.

--------

> 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 >= 80500) {
         $b = \clone($a, ['foo' => '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.

> 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 “legacy way of doing things” for older PHP versions 
(and that one would still remain valid even with the changes this RFC 
proposes).

> 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 “Future 
Scope” 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 
“Future Scope” section: 
https://wiki.php.net/rfc/clone_with_v2?do=diff&rev2%5B0%5D=1748959452&rev2%5B1%5D=1749034411&difftype=sidebyside

Best regards
Tim Düsterhus