[RFC] Clone with v2

> On Thu, May 15, 2025, at 17:32, Tim Düsterhus wrote: >> Hi >> >> Am 2025-05-15 14:14, schrieb Rob Landers: >> > For example, if you have a Money type, you'd want to be able to ensure >> > it cannot be negative when updating via `with()`. This is super >> > important for ensuring constraints are met during the clone. >> >> That's why the assignments during cloning work exactly like regular >> property assignments, observing visibility and property hooks. >> >> The only tiny difference is that an “outsider” is able to change a >> `public(set) readonly` property after a `__clone()` method ran to >> completion and relied on the property in question not changing on the >> cloned object after it observed its value. This seems not to be >> something relevant in practice, because why would the exact value of the >> property only matter during cloning, but not at any other time? >> >> Best regards >> Tim Düsterhus >> > > Hey Tim, > >> why would the exact value of the >> property only matter during cloning, but not at any other time? > > For example, queueing up patches to store/db to commit later; during > the clone, it may register various states to ensure the patches are > accurate from that point. That's just one example, though, and it > suggests calling __clone *before* setting the values is the right > answer. > > I think Larry's idea of just using hooks for validation is also pretty > good. As Larry said, the only thing you can really do is throw an > exception, and the same would be true in a constructor as well. > > — Rob

> However, readonly properties are "protected(set)" by default, so the > situations where this can happen are actually quite limited: > > - Code inside the class itself (private scope) can reasonably be > considered to be "opting in" to the feature it's using. > - Code in a sub-class (protected scope) can by default over-ride the > constructor anyway. > - Code outside the class (public scope) will fail unless the property is > explicitly "readonly public(set)", which would be pointless if it was > always initialised in the constructor. > > > So the only case I can think of where something surprising could happen is: > > 1. A public or protected readonly property is initialised in a > constructor marked "final" > 2. A sub-class adds code that uses "clone with" to set that property to > a new value > > The question then is, how worried are we about that scenario? >

> On Mon, 19 May 2025 at 17:13, Nicolas Grekas > <nicolas.grekas+php@gmail.com> wrote: > > > > > > > > Le lun. 19 mai 2025 à 16:30, Andreas Hennings <andreas@dqxtech.net> a > écrit : > >> > >> On Fri, 16 May 2025 at 21:59, Nicolas Grekas > >> <nicolas.grekas+php@gmail.com> wrote: > >> > > >> > > >> > > >> > Le jeu. 15 mai 2025 à 16:06, Larry Garfield <larry@garfieldtech.com> > a écrit : > >> >> > >> >> On Thu, May 15, 2025, at 1:22 AM, Stephen Reay wrote: > >> >> > >> >> > I may be missing something here.. > >> >> > > >> >> > So far the issues are "how do we deal with a parameter for the > actual > >> >> > object, vs new properties to apply", "should __clone be called > before > >> >> > or after the changes" and "this won't allow regular readonly > properties > >> >> > to be modified". > >> >> > > >> >> > Isn't the previous suggestion of passing the new property arguments > >> >> > directly to the __clone method the obvious solution to all three > >> >> > problems? > >> >> > > >> >> > There's no potential for a conflicting property name, the > developer can > >> >> > use the new property values in the order they see fit relative to > the > >> >> > logic in the __clone call, and it's inherently in scope to write > to any > >> >> > (unlocked during __clone) readonly properties. > >> >> > >> >> I did some exploratory design a few years ago on this front, looking > at the implications of different possible syntaxes. > >> >> > >> >> > https://peakd.com/hive-168588/@crell/object-properties-part-2-examples > >> >> > >> >> What that article calls "initonly" is essentially what became > readonly. The second example is roughly what this RFC would look like if > the extra arguments were passed to __clone(). As noted in the article, the > result is absolutely awful. > >> >> > >> >> Auto-setting the values while using the clone($object, ...$args) > syntax is the cleanest solution. Given that experimentation, I would not > support an implementation that passes args to __clone and makes the > developer figure it out. That just makes a mess. > >> >> > >> >> Rob makes a good point elsewhere in thread that running __clone() > afterward is a way to allow the object to re-inforce validation if > necessary. My concern is whether the method knows it needs to do the extra > validation or not, since it may be arbitrarily complex. It would also > leave no way to reject the changes other than throwing an exception, though > in fairness the same is true of set hooks. Which also begs the question of > whether a set hook would be sufficient that __clone() doesn't need to do > extra validation? At least in the typical case? > >> >> > >> >> One possibility (just brainstorming) would be to update first, then > call __clone(), but give clone a new optional arg that just tells it what > properties were modified by the clone call. It can then recheck just those > properties or ignore it entirely, as it prefers. If that handles only > complex cases (eg, firstName was updated so the computed fullName needs to > be updated) and set hooks handle the single-property ones, that would > probably cover all bases reasonably well. > >> > > >> > > >> > I like where this is going but here is a variant that'd be even more > capable: > >> > > >> > we could pass the original object to __clone. > >> > >> My proposal earlier was to pass the original object _and_ the values > >> that were passed to the clone call, by reference. > >> > >> And this would happen before those values are assigned to the object. > >> > >> class MyClass { > >> public function __construct( > >> public readonly int $x, > >> public readonly int $y, > >> public readonly int $z, > >> ) {} > >> public function __clone(object $original, array &$values): void { > >> // Set a value directly, and modify it. > >> if (isset($values['x'])) { > >> $this->x = $values['x'] * 10; > >> // Prevent that the same property is assigned again. > >> unset($values['x']); > >> } > >> } > >> } > >> > >> $obj = new C(5, 7, 9); > >> $clone = clone($obj, x: 2, y: 3); > >> assert($clone->x === 20); // x was update in __clone(). > >> assert($clone->y === 3); // y was auto-updated after __clone(). > >> assert($clone->z === 9); // z was not touched at all. > > > > > > I'm not sure I understand, there might be missing bits to your idea, eg > where is visibility enforced? why is pass-by-ref needed at all? > > Pass-by-ref makes me think this is a bad idea already :) > > Maybe we are looking at different problems to be solved. > To me, the main questions are: > - Could a __clone() method want to behave differently depending which > property values are passed with the clone call? >

> __clone() and values passed to __clone()? >

> - Should we prevent or allow double-write to a readonly property? That > is, if one write happens in __clone(), and the other write happens > automatically due to the property value passed to clone(..). >

> And to clarify my proposal: > - Everything is the same as in the RFC (except points below) > - Same as in the RFC, the __clone() method is called _after_ the > original object values have been copied over, but _before_ any of the > property values passed as arguments to clone($obj, ...$values) are > assigned. > - Same as in the RFC, the values passed to clone($obj, ...$values) are > assigned automatically after the __clone() method. > - Unlike the RFC, the __clone() method can see (and validate) the > values that were passed to __clone($obj, ...$values) > - Unlike the RFC, the __clone() method can _alter_ the values passed > to __clone($obj, ...$values) before they are assigned. > - As in the RFC, readonly properties can be written only once on > clone. The __clone() method can prevent a double write by unsetting > that key in $values. >

> Consequence: > By leaving the __clone() method empty, all values are assigned > automatically, as in the RFC. > > > where is visibility enforced? > > Exactly as in the RFC. > When clone($obj, ...$values) is called, and before __clone() is > invoked, php has to verify which of the properties are legal to be > updated in this way, based on property visibility and readonly status, > and depending on the scope from which it is called. > > Actually this raises some questions that I did not think of before: > - After __clone() is invoked, does php need to validate again? > - What happens to private properties that are not accessible from the > scope of the __clone() method? Are they also passed in the $values > array? >

> > > Also, WDYT of my simpler proposal itself? Wouldn't it cover all use > cases? > > First, I want to make sure I understand correctly: > - Unlike the RFC, we want to call __clone() _after_ the values from > clone($obj, ...$values) are assigned. > (I assume this because otherwise the two objects would be identical, > and the original object would be useless) > - Unlike the RFC, we pass the original object as a parameter to __clone(). >

> > Tbh I am not sure if the use cases I think of are relevant or not :) > I mostly think of it in terms of functional completeness, without > trying to speculate why a developer would want to do this or that > during __clone(). > > Let's look at the "benefits" section from your earlier mail. > > > The benefits I see: > > - Allow implementing this validation logic you're talking about. > > Having __clone() called after the values are assigned, as you propose, > makes it possible to run an integrity check on the object itself. On > the other hand, this may leave us with a short moment of possibly > "bad" property values. >

> to validate the values array, not the object. >

> > - Allow to skip deep-cloning of already updated properties (that's a > significant drawback of the current proposal - deep cloning before setting > is a potential perf/mem hog built into the engine) : guarding deep-cloning > with a strict comparison would be enough. > > With __clone() called after the values are assigned, and with access > to the original object, we can check $this->prop === $old->prop to see > whether a specific property was updated (and therefore should not be > deep cloned). > With __clone() called before the values are assigned, and with access > to the values array, we would check isset($values['prop']) instead. > (or really array_key_exists()) > In general this would produce the same result, unless a property was > assigned the same value that it had before. >

> filled calculated values. > With access to the old object we would have to check $this->prop !== > $old->prop to then see which dependent properties need to be reset or > recalculated. > With access to the values we would check isset($values['prop']) instead. > > > - Allow implementing WeakMap that are able to clone weak-properties as > objects are cloned. > > I don't feel informed or qualified to talk about this one.. > > So, with __clone() called after and with the original object, we > compare old and new when looking for a specific property. > With access to an array of updated (or to be updated) properties, we > could iterate over the changes, or we could check whether the > changelist is empty, which is less obvious to do by comparing old and > new instance. >

> Note that I don't see the need for operations like in your example > (transforming a value while cloning). > > This looks like the job of a setter or a hook instead, not a cloner. > > Tbh, most of the classes I wrote that would benefit from a "clone > with" did not really need a __clone() method, because everything in > there was already immutable. > To me, the scenarios where we want both are quite speculative, so this > is why my examples might not be the most realistic. >

>> > The benefits I see: > >> > - Allow implementing this validation logic you're talking about. > >> > - Allow to skip deep-cloning of already updated properties (that's a > significant drawback of the current proposal - deep cloning before setting > is a potential perf/mem hog built into the engine) : guarding deep-cloning > with a strict comparison would be enough. > >> > - Allow implementing WeakMap that are able to clone weak-properties > as objects are cloned. > >> > > >> > On this last aspect, I think it's new to the discussion but it's > something I've always found very limiting when using weak-map: let's say > some metadata are stored about object $foo in a weakmap, it's currently not > possible to track those metadata across clones without using some nasty > tricks. If __clone were given the original object, it's be easy to > duplicate meta-data from $foo to $clone. > >> > > >> > I have just one concern significant with adding an argument to > __clone: it'dbe a BC break to mandate this argument at the declaration > level, and adding one right now generates an error with current versions of > PHP. > >> > However, I think we could (and should if confirmed) provide some > FC/BC layer by allowing one to use func_get_args() in __clone. The engine > could then verify at compile time that __clone has either one non-nullable > "object" argument, or zero. > >> > >> This seems reasonable. > >> > > > > I'd be happy to know what Volker and Tim think about this? I read they > excluded any change to __clone in the RFC, but I think it should still be > possible to discuss this, especially if it provides the path to the desired > solution. >