Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow null values in nullable properties #25

Merged
merged 7 commits into from
Aug 25, 2023
Merged

Allow null values in nullable properties #25

merged 7 commits into from
Aug 25, 2023

Conversation

Crell
Copy link
Owner

@Crell Crell commented Aug 12, 2023

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:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@withinboredom
Copy link
Contributor

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
Copy link
Owner Author

Crell commented Aug 13, 2023

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
Copy link
Contributor

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
Copy link

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
Copy link

cebe commented Aug 16, 2023

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

  • Not specified would mean that there is no default value for a property
  • But a "default": null, would mean that null is the default value for that property.

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 Crell changed the title Null improvement, maybe Allow null values in nullable properties Aug 20, 2023
@Crell
Copy link
Owner Author

Crell commented Aug 20, 2023

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
Copy link
Owner Author

Crell commented Aug 25, 2023

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. 😄

@Crell Crell merged commit 51af699 into master Aug 25, 2023
8 checks passed
@Crell Crell deleted the null-improvement branch August 25, 2023 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants