Change __sleep serialization for uninitialized typed properties #5027
Conversation
Proposed fix for https://bugs.php.net/79002 **This will change the behavior of php 7.4 applications that use __sleep with typed properties.** I think this is an improvement, because if `serialize()` returned a string, I'd expect unserialize() to return equivalent data unless wakeup/unserialize/__unserialize threw or misbehaved. Previously, a TypeError would be thrown on unserialize. PHP 7.0 is able to handle leading 0s in the number of properties of an object. I haven't tested earlier versions.
Not sure what you mean here. How are defaults related to this? |
| num_elements--; | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think we should be special-casing typed properties here. Instead we should always omit the property if it doesn't exist, rather than setting it to a null value.
Instead of populating a hashtable of property names and then directly serializing. This has the advantage of a) detecting duplicate properties more precisely and b) gives us the ability to discard values without rewriting the serialization string after the fact for GH-5027.
|
I really don't like the rewriting of the serialization string that is going on here. I've just landed some old __sleep() refactorings I had lying around, which create a proper HT of __sleep properties. Ideally, the patch should now be as simple as dropping this line: Line 837 in 0d35f8e |
My reason for going with this approach instead of always omitting the undefined property from __sleep was that the below example would behave differently in php 7.3 and 7.4 if a typeless property was unset() or never added in 7.4. I'm not aware of any frameworks that deliberately rely on that and ignore the notice, but if it does get changed, I'd want it mentioned in upgrading notes.
Aside: Ideally, adding a <?php
class HasProp {
public $first = 'value';
public $p = 2;
public function __sleep() {
return ['first', 'p'];
}
}
$x = new HasProp();
unset($x->p);
var_dump($x);
var_dump($s = serialize($x));
/*
Before, the result of unserialize():
object(HasProp)#2 (2) {
["first"]=>
string(5) "value"
["p"]=>
NULL
}
After removing zend_hash_add(ht, name, &EG(uninitialized_zval));
object(HasProp)#2 (2) {
["first"]=>
string(5) "value"
["p"]=>
2
}
*/
var_dump(unserialize($s)); |
For the above example, it's related to the question of how it would handle |
|
I understand the concern. However, I think that the correct behavior here is either "unset properties represented as null" or "unset properties not represented", but not something in the middle that depends on whether the property in question is typed. The main issue I see with your example above is that |
That would break my expectations of how it works now, for code deployments that add brand new properties to existing objects, as well as for the properties that are omitted from Suppose that the application using (Or that $myIsUninitialized already exists, but is omitted from |
I agree, but think adding |
What if that property is getting initialized in the constructor rather than via a default value? I don't think unserialization can really work reliably if the class structure changed in the meantime. |
If addition of new properties is indeed a concern, then I agree that adding |
Making True could become the default in 9.0 or when all officially supported php versions support unserializing |
|
Even if we go with |
That would make writing a
|
|
A priori I would assume that an attempt to reference an uninitialized property in __sleep() is an application bug, where some other piece of code did not properly initialize the objects (I mean, that's why those exceptions exist in the first place). There can certainly be code that has explicitly uninitialized properties and quite intentionally wishes to preserve them, but that sounds like a rather exotic use-case to me. I certainly would not expect an average __sleep() implementation to ever deal with uninitialized properties. |
|
Another perspective on this is that __sleep is basically a legacy way to write __serialize and if you write that, you'd get either a notice or an Error, depending on whether it's typed. |
I'd agree that that's exotic and I don't expect to see this in code I use. This is more for bringing up any possible counter-arguments for adding a new way My concerns would be other php maintainers objecting to a rare case starting throwing in 7.4.2, but I don't personally object to throwing for uninitialized typed properties instead of my PR. |
|
I've opened #5050 for the "throw Error" variant of this. I'm not overly concerned about BC at this point, as this both a) already throws a notice and b) is very likely to throw an Error on unserialization, so it's not likely that this is going to break any code. |
Proposed fix for https://bugs.php.net/79002
This will change the behavior of php 7.4 applications that use __sleep
with uninitialized typed properties.
I think this is an improvement, because if
serialize()returned a string,I'd expect unserialize() to return equivalent data unless
wakeup/unserialize/__unserialize threw or misbehaved.
Previously, a TypeError would be thrown on unserialize if the property couldn't be null.
Additionally, if a typed property was nullable, then the unserialized data would be null instead of undefined if there was no default.
__sleepmethod (handling this seems out of scope).PHP 7.0 is able to handle leading 0s in the number of properties of an object.
I haven't tested earlier versions.