Hi Volker,
Sorry for the delay in answering, it's been a long week-end AFK on my side.
Le mer. 28 mai 2025 à 16:52, Volker Dusch <vol...@tideways-gmbh.com> a
écrit :
> Hi Nicolas,
>
> Thank you for the great email and for thinking this through. Getting input
> from the folks that maybe will use this feature the most is very valuable,
> and I appreciate that you're taking the time to do this early.
>
> On Mon, May 26, 2025 at 4:37 PM Nicolas Grekas <
> nicolas.grekas+...@gmail.com> wrote:
>
>> - To me, the main technical drawback of the RFC is that it builds on
>> mandatory deep-cloning, which means wasted CPU/mem resources by design.
>> This has to be mentioned in the RFC IMHO so that voters can make an
>> informed decision.
>>
>> About this last point, it's the one that makes me undecided about my
>> voting decision on the RFC.
>> I shared my concern and a solution in another thread, where I also
>> address the BC concern that is mentioned in the RFC. Since this concern has
>> been addressed, I think this should be considered more closely. As is,
>> that's a big omission to me - recognizing and addressing the issue with
>> deep-cloning.
>>
>
> Quoting from your other email:
>
> > 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.
>
> To make sure we're on the same page: This would only come into effect if
> both a __clone method exists and clone($thing, $withProperties) is used.
>
Correct.
- 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.
> - For readonly classes, given their dependencies are likely to be readonly
> as well, they don't need to be deep cloned in most cases?
>
- Therefore, I think combining __clone and clone-with will be a rare
> occasion, but all performance issues can be avoided by only using one of
> the two approaches in classes where that matters.
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 don't think this is/will be/should be a rare design, nor do I think this
should be ignored in the design of the proposed RFC.
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.
- This RFC leaves future modifications to __clone open, so we could discuss
> a follow-up of passing the properties to __clone if we have a strategy to
> handle the BC issues, but for now, I see this as adding too much complexity
> for a very limited set of use cases that already are solved by the people
> that have them.
>
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"...
> 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.
>
> > - writing code with a wide range of supported PHP versions means we'll
> start using construct such as:
> > if (PHP_VERSION_ID>=80500) {
> > $b = 'clone'($a, [...]); // note 'clone' is a string here, so that the
> line is valid syntax on PHP <= 8.4
> > else {
> > // another code path for PHP 8.4
> > }
>
> Tim has already addressed the \namespace fallback and that this doesn't
> apply here, and that no performance penalty is introduced into any existing
> codebases.
>
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.
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
it to the RFC of course - at least there's this thread for history and this
could be documented later on somewhere else.
Note that the new syntax being the only way to swap the headers while
cloning means that all similarly-designed classes will *have to* live by
the deep-cloning issue. As a corollary, authors that will want to avoid the
deep-cloning issue will be forced to skip such designs. And that's a shame
because that design looks desirable to me (and authors might realize the
issue after-the-fact, which is even worse...)
So what remains on my side is giving enough care to the deep-cloning case -
see my previous messages of course.
Nicolas
