Handle null data when retrieving value

[joe-boudreau]

[Issue]

During deserialization, in some cases the following error gets thrown:

array_key_exists(): Argument #2 ($array) must be of type array, null given
#0 /var/www/vendor/square/pjson/src/Json.php(67): array_key_exists(string(length=9), NULL)
#1 /var/www/vendor/square/pjson/src/JsonSerialize.php(100): Square\Pjson\Json->retrieveValue(NULL,object(ReflectionNamedType))
...

This is because retrieveValue accepts ?array but array_key_exists does not.

Scenarios where I've seen this occur:

• The model field is defined as non-nullable AND it requires internal deserialization itself but the data received for that field is `null

  • So if the field is string $foo and we receive "foo": null then it will stay uninitialized currently
  • But if the field is Category $foo and Category has #[Json] annotated fields then this error will get thrown
  • This fixes the library behaviour to treat both scenarios the same. i.e. the field will stay uninitialized

• In the case of nested property paths, if any of the containing fields are null

  • For example, if the received JSON has data:null for the below:

  #[Json(["data", "name"])]
  protected $data_name;
  

[Fix]

• Do null checks while traversing the $pathBits to the field inside retrieveValue
• After arriving at the field, modify the null checking afterwards to immediately return if the final field is null.
  • The caller JsonSerialize::fromJsonData has the necessary logic to deal with the returned null. Namely, if the type allows null, set the field. If not, leave it uninitialized

[Tests]

Added tests to demonstrate the fix.

Prior to the fix:

1) Square\Pjson\Tests\DeSerializationTest::testNonNullableFieldWithNullValue
TypeError: array_key_exists(): Argument #2 ($array) must be of type array, null given

/Users/jboudreau/Development/pjson/src/Json.php:73
/Users/jboudreau/Development/pjson/src/JsonSerialize.php:101
/Users/jboudreau/Development/pjson/src/Json.php:132
/Users/jboudreau/Development/pjson/src/JsonSerialize.php:101
/Users/jboudreau/Development/pjson/src/JsonSerialize.php:55
/Users/jboudreau/Development/pjson/tests/DeSerializationTest.php:717

2) Square\Pjson\Tests\DeSerializationTest::testNullParentObjectForNestedPath
TypeError: array_key_exists(): Argument #2 ($array) must be of type array, null given

/Users/jboudreau/Development/pjson/src/Json.php:73
/Users/jboudreau/Development/pjson/src/JsonSerialize.php:101
/Users/jboudreau/Development/pjson/src/JsonSerialize.php:55
/Users/jboudreau/Development/pjson/tests/DeSerializationTest.php:735

[joe-boudreau]

I copied the behaviour from the Php81DeSerializationTest class. I think it's better to be more explicit about the returned value so the tests are self-documenting

[khepin]

PR checks are not passing

[joe-boudreau]

@khepin fixed!

[khepin]

So, I'm not sure this is the correct approach (leaving it uninitialized) when we receive null.
If I compare this to the fact that we could have received anything here like null, a string, an int, a float , an array or a bool and the only valid thing to get is a dict / map. Then in the other cases we wouldn't skip it as unassigned right?
We'd throw an error and say "Hey, you wanted a Schedule shaped thing, but what we got was a bool ". Which admittedly is not a well handled case by this library.
Question is: should null be different?

[joe-boudreau]

@khepin I hear what you're saying. In principal I agree that receiving null when the field isn't declared as nullable is the same as receiving any other incompatible data type. e.g. a bool for an array or a string for a class type.

Right now, we throw a typeError in those incompatible data scenarios, and I think we should consider throwing errors when non-nullable field has a null value too (or is not present at all in the JSON). Basically what we talked about a few weeks ago. I'm going to work on a PR for this separately.

But for this current PR, I did some more testing and I realized that this error only gets thrown in a more specific scenario than I originally thought. I updated the PR description but copying the change here:

• The model field is defined as non-nullable AND it requires internal deserialization itself but the data received for that field is null

  • So if the field is string $foo and we receive "foo": null then it will stay uninitialized currently
  • But if the field is Category $foo and Category has #[Json] annotated fields then this error will get thrown
  • This fixes the library behaviour to treat both scenarios the same. i.e. the field will stay uninitialized

So the library currently behaves this way for scalar type fields because it doesn't have to call retrieveValue again on those fields, but if the field is a more complex type than this PHP Error gets thrown.

Barring any other changes, I think it makes sense to handle null for both those field types the same way - what do you think?