Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nullable values #54

Merged
merged 4 commits into from
May 8, 2024
Merged

Nullable values #54

merged 4 commits into from
May 8, 2024

Conversation

waahhhh
Copy link
Contributor

@waahhhh waahhhh commented Feb 27, 2024

Unit test for #53

Resolves #53

@Crell
Copy link
Owner

Crell commented Mar 3, 2024

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.

@Crell
Copy link
Owner

Crell commented May 8, 2024

OK, this seems to work, including for the new tests. As I feared, there was some impact on Importers, but there's no way around that.

Good grief I hate nulls...

@Crell Crell merged commit f3c429b into Crell:master May 8, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deserializing NULL values is ignored
2 participants