serializer
serializer copied to clipboard
schmittjoh
[WIP] Allow deserializing null
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | yes |
| Doc updated | no |
| BC breaks? | yes |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | - |
| License | MIT |
Alternative implementation of https://github.com/schmittjoh/serializer/pull/1004
The main trick here are the interaction between the changes in the graph navigator and the visitor
The only problem I see with this that it will make all fields in the request deserialize null, not sure if that is a good thing.
@kunicmarko20 I've found some time to work on this. Te current approach allows to set the property type as nullable (like php 7.4 typed props).
Something as:
class BlogPost
{
/**
* @Type("?string")
*/
private $id = 'what_a_nice_id';
/**
* @Type("?DateTime")
*/
private $createdAt;
/**
* @Type("?array<JMS\Serializer\Tests\Fixtures\Comment>")
*/
private $comments2;
/**
* @Type("array<string,?string>")
*/
private $metadata;
}
When ? is used and setDeserializeNull is specified, then null properties are deserialized as null instead of having the default object value.
Well, this looks amazing. Now you can choose to deserialize null for everything or per prop?
If $context->shouldDeserializeNull() is not called, ? is just ignored.
When $context->shouldDeserializeNull() is called then the props having ? in the type declaration will be null if the JSON contains null.
ah, so you need to enable it and then select which properties this works on?
ah, so you need to enable it and then select which properties this works on?
yes. main reason backward compatibility.
Thinking it twice, having ? could be enough, so shouldDeserializeNull() could be not needed....
just trying to understand if there might have other implications...
Ye, it looks like shouldDeserializeNull is not needed anymore since you need to add ? to your configuration and that should be BC?
Ye, it looks like shouldDeserializeNull is not needed anymore since you need to add ? to your configuration and that should be BC?
No, the ? is not a BC break. This solution is good enough IMO.
I still need to "let some time pass" to see if there is something I'might have missed, since this was the result of some "late night" coding :smile:
What could be an issue or a problem with merging this? Also the target doesn't have to be master anymore
Well, it is not a BC break, so no need to merge it on master? (assuming master is next major and not minor?)
The master branch is targeting 3.1 and not 4.x.
There are not yet any plans for a new major, so I'm keeping one branch less to manage
