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

added support for milliseconds in DateInterval deserialization #1234

Merged
merged 1 commit into from
Jul 12, 2020

Conversation

ivoba
Copy link
Contributor

@ivoba ivoba commented Jul 10, 2020

Q A
Bug fix? yes
New feature? no
Doc updated no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #...
License MIT

This PR is basically the same as this: goetas-webservices/xsd2php-runtime#14

There is a bug in PHP for DateInterval construction and microseconds: https://bugs.php.net/bug.php?id=53831

This fixes the creation of DateInterval with microseconds like: P0Y0M0DT3H5M7.520S
Context: https://stackoverflow.com/questions/60435670/dateinterval-doesnt-accept-iso-8601-timeperiod-with-milliseconds

@ivoba ivoba force-pushed the fix-DateInterval-milliseconds branch 3 times, most recently from 5e1c7ae to 586f568 Compare July 10, 2020 12:45
$dateInterval = new \DateInterval($data);
// milliseconds are only available from >=7.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

This library requires at least php 7.2, so there is no need to check it

$visitor->method('visitString')->with('2017-06-18');

$element = new \SimpleXMLElement('<DateInterval>' . $dateInterval . '</DateInterval>');
$deserialized = $this->handler->deserializeDateIntervalFromXml($visitor, $element, [], $context);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can test the json version instead, so no need to deal with simplexml

@ivoba ivoba force-pushed the fix-DateInterval-milliseconds branch from 586f568 to ef8c948 Compare July 11, 2020 11:15
@ivoba
Copy link
Contributor Author

ivoba commented Jul 11, 2020

@goetas Adjusted the PR with the changes requested.

@goetas goetas merged commit 224f380 into schmittjoh:master Jul 12, 2020
@goetas
Copy link
Collaborator

goetas commented Jul 12, 2020

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants