Hi
Le mer. 4 juin 2025 à 15:33, Tim Düsterhus <t...@bastelstu.be> a écrit :
> 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 = clone $this->foo;
$this->fooId = 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 = '',
> > 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)`.
>
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 = new ArrayObject($headers);
}
public function __clone() {
$this->headers = 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 >= 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.
>
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 “legacy way of doing things” 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 “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
>
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
