-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Fix c and r formats, add tests. #27002
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
ntsekouras
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.
Tested with Post Date and Latests Posts blocks and look good. Thanks @vcanales 👍 !
Thank you for bringing it up! |
ntsekouras
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.
Tested with the new changes and still works nicely for the two blocks. @youknowriad is working on fixing the tests so you should rebase.
|
Merging with unrelated PHP test failures. |
| ), // Offsets the time to the correct timezone | ||
| "yyyy-MM-dd'T'HH:mm:ssXXX", | ||
| { | ||
| timeZone: getActualTimezone(), // Adds the timezone offset to the Date object that will be formatted. |
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.
Hi @vcanales, we are forcing the output to be in the website timezone. The c format may be used in wp.date.format for example. That function should never change the timezone the date passed to it contains. So the c format should just output things without any manipulations of timezones.
I added unit tests that show the issue at #27272.
We also have a bug in the logic in this function, if we pass wp.date.format a date that is already on the website timezone we receive a date with different hours (wrong output).
I guess c format should just map to "yyyy-MM-dd'T'HH:mm:ssXXX" and it should be up to other functions like dateI18n etc to manipulate the timezones if they need that.
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.
Thank you for revisiting this!
I guess c format should just map to "yyyy-MM-dd'T'HH:mm:ssXXX" and it should be up to other functions like dateI18n etc to manipulate the timezones if they need that.
I believe you are correct, but there are some caveats. Some of these additional timezone offsets were necessary because Date objects are interpreted by the browser as local time, unless we explicitly offset them. These roundabout timezone settings are trying to get the Browser to read the time as-is instead of applying the Browser timezone.
All of the above is necessary because WordPress timestamps do not include any timezone information at all, the post's date attribute is in the site's timezone, date_gmt is in GMT, and neither come with a suffix indicating this fact, so it's on @wordpress/date to set the timezone for Date, otherwise the Browser will think that they're all local times. This means that the date formatting functions, if we don't do any timezone offsetting on @wordpress/date, will only work if the site's timezone is the same as the Browser's, otherwise they'll be inaccurate. This was surfaced on at least one issue even before the refactor: #15673
I think this means that we might want to refactor the tests—removing all indication of timezones on the timestamps we test against will actually reflect data we will receive from the API. From there, we have to figure out how to reliably offset timezones on @wordpress/date.
I want to note also that adding timezone information to the timestamps on the API side would help to reduce a lot of the timezone juggling (getting the site's timezone from settings, trying to apply it on Date, offsetting the Browser's timezone if they're not the same, etc) on @wordpress/date.
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've created a code sandbox with a bunch of examples, showing why some of these manual offsets are necessary even if we want to keep the date intact when formatting, explicitly when the browser's timezone is different from the site's:
https://codesandbox.io/s/boring-microservice-d9rpb?file=/src/index.js
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 this means that we might want to refactor the tests—removing all indication of timezones on the timestamps we test against will actually reflect data we will receive from the API. From there, we have to figure out how to reliably offset timezones on @wordpress/date.
Hi @vcanales,
I agree with the tests we have don't cover the use cases of WordPress. But we should not remove all indications of timezones. Our documentation specifies that dates can contain timezones and a plugin developer may pass date's with timezones. We are publishing an npm module that may even be used outside WordPress. So I think all the tests we have are valid and should be kept. We should probably add more tests with dates that don't contain a timezone and probably use a mocking library to simulate local timezones like https://www.npmjs.com/package/timezone-mock.
We currently have:
- format, that should format a date without translations or any timezone manipulation whatsoever.
- dateI18n this function received a date if the date did not contain a timezone, it assumed the date to be in the users timezone, and formated the date to be in the sitetime zone or the timezone argument.
- gmdateI18n manipulates dates in UTC timezone.
In WordPress, the main use case is receiving a date without timezone that we should assume is on the website timezone and format it keeping the website timezone. We don't have any function that does that and that's the reason we had bugs for some time.
In #26276 I'm finding that and adding a function that assumes a date is in WordPress timezone contrary to dateI18n that assumes user, and gmdateI18n that assumes UTC.
Raised here. The issue was related to having to offset the actual time, and the timezone offset on the Date object as well.
How has this been tested?
Screenshots
Types of changes
Checklist: