RFC: Clone with v2

[TimWolla]

RFC: https://wiki.php.net/rfc/clone_with_v2

see TimWolla#6 for a preliminary review.

[iluuu1994]

Note that I did not (yet) carefully look at all tests.

[iluuu1994]

Do we need to unlock all properties again? After all, multiple writes to to readonly properties are forbidden within __clone. This problem can still occur with something like:

class C {
    public function __construct(
        private readonly string $prop,
    ) {}
    public string $a { set { $this->prop = $value; } }
    public string $b { set { $this->prop = $value; } }
}

var_dump(clone(new C('foo'), [
    'a' => 'a',
    'b' => 'b',
]));

[iluuu1994]

Regardless, this can be skipped if !old_object->ce->clone as nothing happened between unlocking and this path.

[TimWolla]

Do we need to unlock all properties again?

I found it easiest to just unlock them again, instead of trying to find the property fields corresponding to the “with” properties.

Regardless, this can be skipped if !old_object->ce->clone as nothing happened between unlocking and this path.

This condition is already there, or did I misunderstand?

[iluuu1994]

I found it easiest to just unlock them again, instead of trying to find the property fields corresponding to the “with” properties.

My statement wasn't very precise. I was more wondering whether we need to unlock them at all. Namely, isn't it at least somewhat expected a property written to by __clone shouldn't also be overwritten again? It wasn't clearly specified how this would be handled in the RFC from what I can see. Regardless, I understand why you're doing it so probably just disregard.

This condition is already there, or did I misunderstand?

No, I just missed it. Apologies.

[iluuu1994]

Nit: It seems like scope is unused when the properties array is empty. Maybe it's better to pass NULL here? Same in zend_objects_clone_members().

[iluuu1994]

Isn't this problematic for overridden clone_obj that reuse zend_objects_clone_obj? In that case old_object->handlers->clone_obj != zend_objects_clone_obj would hit, and we'd recurse infinitely.

[TimWolla]

Yes. There are none in php-src and generally speaking it seems unlikely that there are any, since when there is a need to overwrite clone there likely also is a need to overwrite create (which explicitly isn't supported):

/* assume that create isn't overwritten, so when clone depends on the

• overwritten one then it must itself be overwritten */

Most of the custom clone handlers just use zend_objects_clone_members().

---

This PR tries to minimize the disruption for existing code, but if there is an external extension that does what you suggest, then it's easy enough for the author to adjust it to add an explicit clone_with handler.

[iluuu1994]

Alternatively, you could do the inverse: Keep clone_obj as is, call it whenever properties are not provided or when the array is empty. When it isn't, clone_obj_with is invoked, which can call clone_obj to clone the object, and adjust the properties afterwards. This way, an extension could simply continue to provide clone_obj and the property part would just work. This approach seems more logical to me.

[TimWolla]

which can call clone_obj to clone the object, and adjust the properties afterwards

This is likely dangerous, since some internal classes might expect their (readonly) properties not to change, which leads to some C structures related to a property not being properly adjusted. Random\Randomizer is currently not cloneable at all, but it stores both a public readonly \Random\Engine $engine property exposed to userland and also the raw “engine pointer” for more efficient access.

The implementation also prefers the clone_with handler, since I would like to see the old clone_obj hook to go away in the future to remove the duplication.

[iluuu1994]

This is likely dangerous, since some internal classes might expect their (readonly) properties not to change

Can this already be broken with __clone in a user-declared sub-class? I can't see cases other than enums preventing the declaration of __clone. The property would need to be private(set) to prevent this.

[iluuu1994]

Suggested change

	set (string $value) {
	set {

NIt: Let's stay consistent with the code style in existing tests.