-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fixed #74298 - IntlDateFormatter->format() doesn't return microseconds/fractions #2432
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
Conversation
Hmpf, luckily MariaDB / MySQL and PostgreSQL do 6 digits precision (I am using datetime(6) as a revisioning candidate/unique key part). So AFAIU PHP's datetime objects do (unless buggy) the same; however ICU parsing/formatting - by spec - cuts at 3 digits, right? THanks for caring @ PR. |
@inoas yes, 3 digits precision relates only to ICU |
@@ -137,7 +139,8 @@ U_CFUNC int intl_datetime_decompose(zval *z, double *millis, TimeZone **tz, | |||
return FAILURE; | |||
} | |||
|
|||
*millis = U_MILLIS_PER_SECOND * (double)Z_LVAL(retval); | |||
datetime = Z_PHPDATE_P(z); | |||
*millis = U_MILLIS_PER_SECOND * ((double)Z_LVAL(retval) + datetime->time->f); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that the timestamp is retrieved through a getTimestamp()
method while the fraction is obtained directly from the structure feels like a breach of abstraction. On the other hand, it does not seem like DateTime
has a direct way of getting the fraction (or timestamp with fraction).
@derickr Do you have any thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikic @derickr I was thinking about this too, but at the same time I took this idea from here https://github.com/andrewnester/php-src/blob/1bf8cfbb9a136ee64303206446ec7e2e97f7ec4c/ext/intl/common/common_date.cpp#L150
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think getTimestamp should have been call as a PHP function at all here. THen again, timelib should have a function for returning timestamp/fraction, and all the other fields really.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derickr I think the idea here might have been (I don't know) that people might extend DateTime::getTimestamp()
in userland. If this is not something we want to respect, then directly fetching the value from the underlying structure is of course much better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikic agree, I thought that it's because of extending of DateTime::getTimestamp()
. From my perspective it seems reasonable
Any news on this? |
Merged as 1ce355a, thanks! |
This is awesome! ❤️ x 💯 |
FIx for https://bugs.php.net/bug.php?id=74298
udat_format
supports formatting only milliseconds (3 digits of fractions).Here is the statement from documentation:
Appends zeros if more than 3 letters specified. Truncates at three significant digits when parsing.