On Wed, Aug 13, 2025, at 11:36 AM, Nicolas Grekas wrote: > Le mer. 13 août 2025 à 17:34, Nicolas Grekas > <nicolas.grekas+...@gmail.com <mailto:nicolas.grekas%2b...@gmail.com>> > a écrit : >> >> >> Le mer. 13 août 2025 à 14:33, Tim Düsterhus <t...@bastelstu.be> a écrit : >>> Hi >>> >>> On 8/13/25 13:58, Nicolas Grekas wrote: >>> > I wish we had some process to revert that decision. Now is the less costly >>> > time to do it, even if it's already late. >>> >>> There is always the option of ”not implementing” the decision. Reverting >>> would mean another RFC that would also require a 2/3 majority. >> >> I'm happy to do it this way if that's the consensus. >> >> >>> > The discussion period failed to raise the points made by Jakub in the PR ( >>> > https://github.com/php/php-src/pull/19435) and failed a serious impact >>> > analysis IMHO. >>> > This should be enough to raise a flag and allow us to reconsider. >>> > I wonder if people that voted in favor of deprecating __sleep/wakeup would >>> > still vote the same now? >>> >>> I already participated in the PR discussion and having slept over it, I >>> think it would be helpful to consider both __sleep() and __wakeup() >>> separately in this discussion of whether or not to actually follow >>> through with doing the deprecation. >>> >>> As I've mentioned in the PR, `__sleep()` is actually broken when >>> inheritance is involved and there's also a trivial way to implement >>> `__serialize()` while preserving compatibility: >>> >>> public function __serialize() { >>> return get_mangled_object_vars($this); >>> } >> >> Well, the way to introduce __serialize while maintaining compatibility with >> both child classes and existing payloads is to copy/paste the same >> boilerplate over and over: >> >> public function __serialize(): array >> { >> $data = []; >> foreach ($this->__sleep() as $key) { >> $data[$key] = $this->$key; >> } >> return $data; >> } >> >> And then, this has exactly the same issue regarding private properties, >> without any better way since this has to call __sleep to account for child >> classes. >> This is just moving the concern elsewhere without any benefit. >> On their side, child classes that do have an issue with private properties >> can already implement __serialize on their side, there's nothing to fix >> here: __sleep works well, and there's already an upgrade path when it's not >> enough. >> >> >>> So while this one might result in some amount of migration work, it's >>> reasonably easy to do. >>> >>> `__wakeup()` on the other hand is not broken and migrating to >>> `__unserialize()` is non-trivial. >>> >>> So deprecating `__sleep()` with PHP 8.5 still makes sense to me. For >>> `__wakeup()` I don't have a strong opinion either way and I've also >>> intentionally abstained from voting on this deprecation. >> >> implementing __unserialize to satisfy a proper upgrade path for payload and >> child classes might be done this way: >> >> public function __unserialize(array $data): void >> { >> foreach ($data as $key => $value) { >> $this->{("\0" === $key[0] ?? '') ? substr($key, 1 + >> strrpos($key, "\0")) : $key} = $value; >> } >> $this->__wakeup(); >> } >> >> Again, this doesn't work with private properties. >> >> The previous code was better and safer, and contained less boilerplate. >> >> I made a draft PR for SYmfony here: >> https://github.com/symfony/symfony/pull/61407 >> I'm not sure the implementation is correct yet. >> That's just to show the kind of impact this could have. >> >> Deprecating only sleep and not wakeup would at least fix the complexity of >> the __unserialize replacement. >> But then, this would fail the purpose of the RFC, which was to get rid of >> sleep/wakeup altogether. >> >> This deprecation is a net loss on every aspect. >> >> Nicolas > > Here is the result of my investigation; the kind code that everybody > that cares about FC/BC for child classes and payloads will have to use > (above PR is up to date): > > public function __serialize(): array > { > $data = []; > foreach ($this->__sleep() as $key) { > try { > if (($r = new \ReflectionProperty($this, > $key))->isInitialized($this)) { > $data[$key] = $r->getValue($this); > } > } catch (\ReflectionException) { > $data[$key] = $this->$key; > } > } > return $data; > } > public function __unserialize(array $data): void > { > \Closure::bind(function ($data) { > foreach ($data as $key => $value) { > $this->{("\0" === $key[0] ?? '') ? substr($key, 1 + > strrpos($key, "\0")) : $key} = $value; > } > $this->__wakeup(); > }, $this, static::class)($data); > }
Eew. :-) I support the goal of having only one serialization process for devs to think about, but it does need to have a clear and graceful transition for existing code. The above code does not strike me as "clear and graceful transition." >From the RFC: > Having multiple serializations methods which can be superseded by newer > methods is somewhat confusing and add unnecessary complexity to the language. I agree, which is why I voted Yes on that part. > Because the __serialize()/__unserialize() mechanism is a straight up > improvement over __sleep()/__wakeup() we propose to deprecate the latter in > favour of the former. This claim now appears to be incorrect, given Nicolas' investigation. (Ie, it's not a straight up improvement, nor a drop-in replacement.) I would be in favor of holding off implementing this deprecation until a better transition process is found, and/or an RFC to reverse it. (I defer to the RMs on the process they want to follow.) (And I think this does lend still more weight to Juliette's frequent request for more robust impact analysis for deprecations. Not because deprecations are bad, but because we should know what the impact is so we know how to mitigate it effectively.) --Larry Garfield
