Skip to content

Test for null or empty strings when deserializing dates#1600

Merged
scyzoryck merged 1 commit into
schmittjoh:masterfrom
gnat42:check-empty-dates
Jun 2, 2025
Merged

Test for null or empty strings when deserializing dates#1600
scyzoryck merged 1 commit into
schmittjoh:masterfrom
gnat42:check-empty-dates

Conversation

@gnat42

@gnat42 gnat42 commented May 17, 2025

Copy link
Copy Markdown
Contributor
Q A
Bug fix? yes
New feature? no
Doc updated no
BC breaks? not sure
Deprecations? no
Tests pass? yes
Fixed tickets #...
License MIT

Some APIs send empty dates instead of null or omitting the field. For example

{
"someDateField": ""
}

Currently only "someDateField": null works. Changing the null test to an empty test allows for the system to skip deserializing empty date fields.

@scyzoryck

Copy link
Copy Markdown
Collaborator

Hi!
Thanks for the Pull Request! It looks like an interesting edge case. I wonder what is current outcome of the empty string or 0 deserialization?
Can you cover this case with some additional test cases? I wonder if it makes sens to make it as an optin feature - as it can be potentially and BC for some projects (depending on how it behaves currently). 😓

@gnat42 gnat42 force-pushed the check-empty-dates branch from c70361a to a0cea63 Compare May 20, 2025 14:59
@gnat42

gnat42 commented May 20, 2025

Copy link
Copy Markdown
Contributor Author

I've added tests.

I was wondering about the BC, I'm not 100% sure, however I would think that anyone running into this issue may have created their own DateTime handler for deserialization. If they didn't they would get an exception from line 275 of DateHandler.php, which is what happened to us. The change makes those custom handlers obsolete if that is all they do. Otherwise, I'm not sure what behaviour one would expect from this. I get its a change so I'm fine either way but it seems like a safe fix imo

@scyzoryck scyzoryck merged commit 410da8b into schmittjoh:master Jun 2, 2025
24 checks passed
@scyzoryck

Copy link
Copy Markdown
Collaborator

Thanks for contribution!

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.

2 participants