Crell / Serde

Robust Serde (serialization/deserialization) library for PHP 8.
Other
299 stars 14 forks source link

Allow null values in nullable properties #25

Closed Crell closed 1 year ago

Crell commented 1 year ago

Description

Another attempt at #15, to allow nullable values. Updating the serializing side was easy. Updating the deserializing side is much harder, and is not yet working.

I am also torn on if this is even a good idea. This cannot be done without an API change for formatters, and some fairly invasive changes. The idea that null == uninitialized is baked in pretty deep, and in all honesty I'm not sure in what situations that's a wrong assumption.

Can someone provide a concrete common use case that would make this necessary, that cannot already be solved another way (like a default value)? Because at the moment I'm very tempted to won't-fix this.

I also started a Mastodon thread.


Based on the discussion here and on Mastodon, I've decided to go ahead with this change. It's an API break, but not a huge one, and the code changes weren't quite as invasive as I was fearing. I am unsure if this also needs an attribute-based way to make null be skipped when serializing. Thoughts?

Types of changes

What types of changes does your code introduce? Put an x in all the boxes that apply:

withinboredom commented 1 year ago

A good use case is a memoized value that only persists during a single request. One cannot simply do:

if($this->memo_value === null) {}

Luckily this works just fine:

$this->memo_value ??= $this->some_complex_thing();

However, that's pretty much the only use case I've run into, personally, where you intentionally store null, so you write code that expects null. I just have a simple iterator on deserialization that checks if it is nullable and not set, if it isn't set, it sets it to null. Works simple enough for me. Maybe having a simple hook on deserialization would make it a bit simpler (it might already exist, I haven't checked).

Crell commented 1 year ago

If you have a computed, memoized field, I'd expect it to not get serialized in the first place. I mean if you have a property that is null, or a property that is uninitialized, right now both of those will get simply omitted from the serialized output. Plus, if you try to deserialize a string where one of the properties is set to null, it will get skipped and the property will end up with either its default value or just uninitialized.

I'm still not convinced that isn't the correct behavior; the object may then be in an invalid state, but full on validation is out of scope. There is the requireValue setting already if you want to enforce that the property not be uninitialized.

withinboredom commented 1 year ago

Ah, yep, these were excluded properties, so they wouldn't get serialized, but upon deserialization, they were unset, despite the default value being null (IIRC). Might be a separate issue altogether, but either way, worked around it just fine.

repli2dev commented 1 year ago

Let's look into it from another angle... The expected behaviour of internal PHP JSON deserialization function does not consider null to become undefined... the object is created and null is assigned.

var_dump(json_decode('{"a": null}'));
object(stdClass)#1 (1) {
  ["a"]=>
  NULL
}

From that I would argue that the another JSON deserialization library should meet basically the same axioms.


Even more the serialize/deserialize functions distinguish between null and undefined

class A {
    public string $foo;
}

$a = new A();

var_dump($a);
var_dump(serialize($a));
var_dump(unserialize('O:1:"A":0:{}'));
object(A)#1 (0) {
  ["foo"]=>
  uninitialized(string)
}
string(12) "O:1:"A":0:{}"
object(A)#2 (0) {
  ["foo"]=>
  uninitialized(string)
}
cebe commented 1 year ago

I have a case to consider here, which is default from JSON Schema:

https://datatracker.ietf.org/doc/html/draft-fge-json-schema-validation-00#section-6.2

So if you are using this library to serialize and deserialize a JSON schema, could this be a problem? (Have not checked your code)

Crell commented 1 year ago

Based on the discussion here and on Mastodon, I've decided to go ahead with this change. It's an API break, but not a huge one, and the code changes weren't quite as invasive as I was fearing. I am unsure if this also needs an attribute-based way to make null be skipped when serializing. Thoughts?

Crell commented 1 year ago

For now I'm going to merge this in its current form. I won't be tagging a release just yet as there's a few other things I want to look into first, but it will get released eventually. :smile: