-
Notifications
You must be signed in to change notification settings - Fork 8k
Fix timezone offset with seconds losing precision #20764
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
base: PHP-8.3
Are you sure you want to change the base?
Conversation
There are two issues: 1. The 'e' formatter doesn't output the seconds of the timezone even if it has seconds. 2. var_dump(), (array) cast, serialization, ... don't include the timezone second offset in the output. This means that, for example, serializing and then unserializing a date object loses the seconds of the timezone. This can be observed by comparing the output of getTimezone() for `$dt` vs the unserialized object in the provided test.
| case TIMELIB_ZONETYPE_OFFSET: { | ||
| int seconds = offset->offset % 60; | ||
| if (seconds == 0) { | ||
| length = slprintf(buffer, sizeof(buffer), "%c%02d:%02d", | ||
| ((offset->offset < 0) ? '-' : '+'), | ||
| abs(offset->offset / 3600), | ||
| abs((offset->offset % 3600) / 60) | ||
| ); | ||
| } else { | ||
| length = slprintf(buffer, sizeof(buffer), "%c%02d:%02d:%02d", | ||
| ((offset->offset < 0) ? '-' : '+'), | ||
| abs(offset->offset / 3600), | ||
| abs((offset->offset % 3600) / 60), | ||
| abs(seconds) | ||
| ); | ||
| } |
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.
nit: these lines mix tabs and spaces
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.
Yeah this was already the case to align the switch case. It's a bit annoying to work with.
derickr
left a comment
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 think I would like to see a test for a negative timezone offset as well, where seconds are present.
| int seconds = offset % 60; | ||
| size_t size; | ||
| const char *format; | ||
| if (seconds == 0) { |
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.
nit: Extra new line between declarations and code please.
There are two issues:
$dtvs the unserialized object in the provided test.