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

Fix date-time type format #20

Merged
merged 3 commits into from
Oct 11, 2019
Merged

Fix date-time type format #20

merged 3 commits into from
Oct 11, 2019

Conversation

sunspikes
Copy link
Contributor

Fixes #19

The the RFC3339 date-time can be with or without milliseconds and with or without specific timezone offsets.

With PHP milliseconds support was added in DateTimeInterface::RFC3339_EXTENDED (https://www.php.net/manual/en/class.datetimeinterface.php#datetime.constants.rfc3339_extended) but this doesn't support the other format without milliseconds. So we need to check both.

https://3v4l.org/r059V

@lezhnev74
Copy link
Contributor

Awesome! Can you please add a test for this?

@coatesap
Copy link

@lezhnev74 / @sunspikes - I've actually just written a test for the PR I was working on - is there a way I can contribute it to your branch?

@sunspikes
Copy link
Contributor Author

@lezhnev74 Done!

@sunspikes
Copy link
Contributor Author

@coatesap sorry, i missed your comment. May be you can add those extra tests in a new PR ?

@sunspikes
Copy link
Contributor Author

sunspikes commented Oct 11, 2019

Unfortunately DateTimeInterface::RFC3339_EXTENDED is supported only in PHP 7.3.0+, so i had to use the custom validator

@coatesap
Copy link

Nice work @sunspikes 👍. I've pushed my branch up as a PR #21, but I think your approach of unit testing the StringDateTime class directly is better. Also, it's good if it supports microseconds as well, as other users might need that.

@coatesap
Copy link

@sunspikes - out of interest - does the format Y-m-d\TH:i:s.uP (with the "u") not work on PHP 7.2?

Copy link
Member

@scaytrase scaytrase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but some minor changes needed.

Do we have any kind of functional test that validator works correctly with new string formats?.

src/Schema/TypeFormats/StringDateTime.php Outdated Show resolved Hide resolved
@coatesap
Copy link

@sunspikes - out of interest - does the format Y-m-d\TH:i:s.uP (with the "u") not work on PHP 7.2?

Looking at https://3v4l.org/jSDKd it does seem to work fine, which would both avoid the need for a new class and also supports microseconds out of the box.

@sunspikes
Copy link
Contributor Author

@sunspikes - out of interest - does the format Y-m-d\TH:i:s.uP (with the "u") not work on PHP 7.2?

Afaics, with milliseconds it fails for PHP 7.2 https://travis-ci.org/thephpleague/openapi-psr7-validator/jobs/596524264

@coatesap
Copy link

Do we have any kind of functional test that validator works correctly with new string formats?.

@sunspikes - Feel free to take the functional test from #21 if you want

@coatesap
Copy link

Afaics, with milliseconds it fails for PHP 7.2 https://travis-ci.org/thephpleague/openapi-psr7-validator/jobs/596524264

Wasn't that using DateTime::RFC3339_EXTENDED which equates to Y-m-d\TH:i:s.vP with a "v"?

@sunspikes
Copy link
Contributor Author

sunspikes commented Oct 11, 2019

Afaics, with milliseconds it fails for PHP 7.2 https://travis-ci.org/thephpleague/openapi-psr7-validator/jobs/596524264

Wasn't that using DateTime::RFC3339_EXTENDED which equates to Y-m-d\TH:i:s.vP with a "v"?

Yes it is but it fails with DateTime::createFromFormat https://3v4l.org/HW2sL , a quick googling lead me to this :/ https://bugs.php.net/bug.php?id=75577

@coatesap Thank you, i've added your tests cases too

@sunspikes
Copy link
Contributor Author

LGTM, but some minor changes needed.

Do we have any kind of functional test that validator works correctly with new string formats?.

I've added the functional tests from the other PR, courtesy of @coatesap

@coatesap
Copy link

coatesap commented Oct 11, 2019

Yes it is but it fails with DateTime::createFromFormat

Yes, but my suggestion was just to use DateTime::createFromFormat('Y-m-d\TH:i:s.uP', ...) instead of using the constant, as this should work on PHP 7.2 upwards, and includes support for microseconds.

@sunspikes
Copy link
Contributor Author

Yes it is but it fails with DateTime::createFromFormat

Yes, but my suggestion was just to use DateTime::createFromFormat('Y-m-d\TH:i:s.uP', ...) instead of using the constant, as this should work on PHP 7.2 upwards, and includes support for microseconds.

@coatesap Sorry i missed that point. Yes we could do that. But in this case we need to consider one more scenario from https://tools.ietf.org/html/rfc3339#section-5.6

  NOTE: ISO 8601 defines date and time separated by "T".
  Applications using this syntax may choose, for the sake of
  readability, to specify a full-date and full-time separated by
  (say) a space character.

But this can also be included with an extra check.

@scaytrase
Copy link
Member

But this can also be included with an extra check.

Can you try this? I hope this would be much easier to read later

@sunspikes
Copy link
Contributor Author

@scaytrase done!, yes, i think it's better than maintaining one more class for this.

Copy link
Member

@scaytrase scaytrase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@coatesap
Copy link

Nice, that's a really clean implementation 👍

@scaytrase scaytrase merged commit 5916227 into thephpleague:master Oct 11, 2019
@scaytrase
Copy link
Member

Thanks @sunspikes and @coatesap

@scaytrase
Copy link
Member

https://github.com/thephpleague/openapi-psr7-validator/releases/tag/0.3.3

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.

Date-time format should allow optional milliseconds
4 participants