Fix xml deserialization when xsi:nil="true" is set

[Bouwdie]

Add xml deserialization null check based on xsi:nil
Expand visitor interface with a null data detect function
Add default implementation for null detection in abstract visitor
Add round-trip test for serializing ad de-serializing null object

(Silence operator had to be used - even a count() on the attributes resulted in this error)

[Bouwdie]

@schmittjoh This lib rocks and serialization to xml works like a charm with xsi:nil included! The deserialization did nothing with it however, hence the fix! 👍

[goetas]

Hi @Bouwdie, thanks for your PR.

Your PR is not backward compatible (it adds a public method on an interface), and in the current roadmap of the serializer, can not be accepted. The 2.0 release date is not yet defined.

I see a value in your work and I'm interested in supporting this use case (also for the json usecase if not already implemented)... Can this be implemented keeping backward compatibility?

[Bouwdie]

Hi @goetas Thanks for your super quick reply! I fully agree that this change is a BC change. The reason it is in the interface is that it is not necessarily xml specific. I am doubting between two approaches here; moving it directly into the XmlDeserializer (which i don't like) or introduce a new interface. A new interface would be nice anyway since the method is not necessarily visitor specific.

BTW - The json use-case works outside the box since there is no additional layer like simplexml (null = null).

[goetas]

I like the interface approach, but consider that you will introduce probably a BC break anyway....

Consider this class

class Foo 
{
   /** @Type("int") **/
   public $id;
}

and this json (valid for xml too...)

{
  "id":null
}

$foo = $serializer->deserialize($json, Foo:class, 'json');

var_dump($foo->id)

The result will be:

Current code (v1.7.1):
int(0)

With your code:
NULL

[Bouwdie]

Indeed this would we a change of the current behaviour. It however would solely be a change in behaviour for the XML de-serialization. The fact that the xsi:nil is solely implemented in the serialization direction marks the XML de-serialization as incomplete. Serialization -> de-serialization with the same context should in my opinion hold the same result for both formats. Implementations that rely on $foo->id === '' is limited to XML de-serialization users and strict comparisons. Of-course we can not start assuming things but the use-cases for this behaviour are probably to handle the value as if it were null (instead of making a pr ;-)).

[goetas]

It however would solely be a change in behaviour for the XML de-serialization.

false, because of the changes on the abstractvisitor and graph navigator...

you can add a custom xml deserializer to handle xsd:nil... and forward everything else to the xml serializer...

[Bouwdie]

All of the changes are so that old functionality stays the same (that's the thought), except for the xml-deserializer. I could move this into a custom de-serializer downstream, and this nice lib would stay with this inconsistency. Or would you consider a custom xml-deserializer + docs into the 1.7 branch for now with a permanent fix in 2?

[kanariezwart]

you can add a custom xml deserializer to handle xsd:nil... and forward everything else to the xml serializer...

So the (XML) deserialization behavior will stay incorrect because of the BC break?
Writing your own DIY custom solution is always an option.
Making the JMS library work better is the higher ground right? If that means a proposed PR has a deeper impact and needs more attention, we shouldn't shun them.

[Bouwdie]

@goetas

• I removed the public method from the visitor interface into a new reusable interface
• Restored behaviour in the graph-navigator for the elseif branch in serialization mode
• Removed implementation from the abstract visitor entirely

Last thoughts:

• You are right it will be a BC change for xml deserialization when strict checking is used (which in itself already smelly since it would tie a user to the XML format directly)
• This fix will make the xml serialize-deserialize round-trip consistent.
• This fix will make behaviour for null deserialization consistent across formats
• The new test proves JSON null de-serialization worked before this fix

If you have any other suggestions please let me know 🍔 + thanks again for taking the time to maintain this library; it is highly appreciated! 👍

[goetas]

Hi, thanks for doing the changes to your PR, now looks definitively more clean. Please allow me some time to test and think about this PR since it introduces a couple of important changes.

[Bouwdie]

@goetas Any thoughts regarding the PR already? 😺