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
