Crell
commented
8 months ago OK, I've looked into this a bit and the fix is... annoying.
The first issue is a couple of isset() calls that should be array_key_exists(). That's easy enough to fix.
However, the next issue is that the Deformatter methods are typed to their respective types... non-nullably. So, in this case, deserializeString(): string|DeformatterResult. It cannot return null, even if the body is adjusted to allow for nullable values. (Which is just tweaking another conditional.)
However, changing that return type is an API change. Technically any 3rd party Deformatters in the wild should continue to work, since they'd be narrowing the return type from the interface so it's type-compatible. My concern would be for any 3rd party Importers, as they may not expect a null value to be returned. Since importValue() returns mixed, it's probably safe, but still technically an inter-component API change.
We also cannot make the change at the Importer level, because the Importer by design cannot introspect the incoming data; that is exclusively the job of the Formatter.
And everything above applies to all Deformatter type methods, not just string.
Hm. Or maybe we could attempt to call deserializeNull() from ScalarExporter::importValue() if the field is nullable? No, because deserializeNull() throws an exception if the value isn't actually a null. Damnit, Exceptions, you suck as much as null!
I'll have to think on this. I cannot think of a fix other than an API change on the Deformatter. That might be acceptable, as I don't expect there are many custom Importers out there that would operate on scalars, but still, I'm not wild about it.
If anyone else has suggestions for alternate solutions, please share.
I've pushed up the first batch of fixes for folks to see, for reference.
Unit test for https://github.com/Crell/Serde/issues/53
Resolves #53