Add option to mark properties as required

[seyfahni]

This will then throw an exception while deserializing if the JSON data does not contain the property at all instead of keeping it uninitialized.

[khepin]

Hi, sorry I missed this, looks like my notifications weren't properly setup. Looking at it this week.

[khepin]

Hi, I'm gonna keep your PR open for now but not merge it while I ponder on it. I understand where this is coming from and I realize this is not a lot of code. My main concern is that so far we haven't added anything related to data validation inside of pjson and we might prefer to keep those concerns separate.

[seyfahni]

Hm, yeah. I was thinking in the same direction, but there is not much better alternatives for that though and it mostly means that I'd have to parse the JSON twice.

I'd also be fine if it just threw an exception in JsonSerialize line 103, cause there's the actual location that a not properly initialized object is created. And without using reflection it is just impossible to check for uninitialized properties.

Nonetheless I see your point and you can decide either way.

[khepin]

You can check for uninitialized properties with isset. For example:

class A
{
    public string $name;
}

$a = new A;
if (isset($a->name)) {
    echo $a->name;
}

This will not throw any error, even though going through echo $a->name would throw Typed property A::$name must not be accessed before initialization. So reflection isn't needed there and this could indeed be part of a validation step independent of deserializing.

[seyfahni]

I feel so dumb right now, never thought about using isset() on a readonly property (that I had in my case, but being typed causes the behavior)... Anyways, thank you.

[seyfahni]

Validation post deserialization still isn't really viable: While having required properties (like I added here) could be checked after the fact, invalid types are already throwing exceptions right now.

So those would either needed to be caught or avoided by setting the property null or leaving it uninitialized. Currently the TypeErrors already act as some kind of validation.

And secondly, with isset() you cannot distinguish between null and uninitialized:

class A
{
    public ?string $name;
}
$a1 = new A;
$a1->name = null;
$a2 = new A;
echo isset($a1->s) ? 't' : 'f'; // f
echo isset($a2->s) ? 't' : 'f'; // f
echo $a1->s; // outputs nothing
echo $a2->s; // throws TypeError

Now, as I am working with readonly classes I simply cannot validate-and-set-null the value, as it might either be null (error when writing the property) or uninitialized (error when reading the property).

[khepin]

Indeed. I also checked for default values but those don't play well with readonly properties. And of course is_null unlike isset will actually access the prop and therefore throw an exception.

So then as you said, it's back to reflection, or doing some weird try / catch trickery.

@mattgrande there's good points in here. Mind giving me your thoughts on this?

[mattgrande]

I'm not opposed to this level of validation, but I'm moderately concerned about opening the door to it... that being said, I really see the points about isset vs is_null not being reliable.

I mostly just don't want a full validation system.

[khepin]

@seyfahni can you sign the CLA ?

As this is a Square hosted project, individual contributors are required to sign the CLA in order to contribute. Once you have signed, we'll be able to move forward with the PR.

Then we can move forward with this PR.

[seyfahni]

@khepin I have signed it.

[khepin]

Great, I've got the confirmation for it too. Are you able to fix those merge conflicts?

[seyfahni]

Yeah, I will try to get it done tomorrow.

[khepin]

Going to merge this, but I will update it to throw a custom exception before making a release.