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

__getExperimentalSettings timezone offset may be a string or integer #21977

Closed
jeremyfelt opened this issue Apr 29, 2020 · 9 comments
Closed
Labels
[Package] Date /packages/date

Comments

@jeremyfelt
Copy link
Member

jeremyfelt commented Apr 29, 2020

Describe the bug

The value provided by wp.date.__getExperimentalSettings.timezone.offset differs in type depending on the site's timezone setting.

  • A timezone of America/Los_Angeles set through General Settings in the WordPress admin results in an integer offset value of -7 (currently in DST).
  • A timezone of UTC-7 results in a string offset value of "-7".
  • A timezone of UTC results in an integer offset value of 0.
  • A timezone of UTC+0 results in a string offset value of "0".

If this data is passed directly to something like moment().utcOffset(), which expects either an integer or a string formatted like "-07:00", then the returned object will not contain the expected offset information.

Any of these values can be passed through Number() or converted to minutes via * 60, which moment also understands, so I could see how this isn't necessarily a bug bug, but having this documented here may help someone else who started questioning the world a bit. :)

Expected behavior
In a perfect world, wp.date.__getExperimentalSettings.timezone.offset would always be an integer. In a more perfect world, computers would never think we mean "-7" when we type -7. 😀

Edited March 25, 2021: I'm not sure how __getExperimentalOffset() got into my head, but this should all be __getExperimentalSettings().timezone.offset. I've edited the title and description to clarify.

@talldan talldan added [Package] Date /packages/date Needs Testing Needs further testing to be confirmed. labels May 1, 2020
@davilera
Copy link
Contributor

davilera commented Jun 5, 2020

I can confirm this issue and, as pointed out by @jeremyfelt, the value provided by wp.date.__getExperimentalOffset.timezone.offset should always be a number.

Tests in packages/date pass because the timezone is never a string (I know because I wrote them). I didn't realize that sometimes WordPress returned that as a string, so I failed to capture that behavior...

There's two options to fix this one:

  1. Either we tweak setSettings to make sure the this number is not a string (notice it doesn't have to be an integer; it can be a float such as 1.5).
  2. Or we fix WordPress core itself so that it initializes this value correctly.

I personally think WordPress should always provide a consistent value (namely, a number and not a string). But, regardless of that, should we also add a safe guard in wp.date?

@ryelle
Copy link
Contributor

ryelle commented Sep 28, 2020

I just ran into this when trying to figure out why a site shows different times when using "Vancouver" vs "UTC-7". I've pushed a PR that converts the offset to a number before using it in buildMoment #25684 — it seemed safer to make the conversion at this point, in case other plugins are filtering that value, or the library is used outside of WordPress.

@skorasaurus skorasaurus removed the Needs Testing Needs further testing to be confirmed. label Dec 1, 2020
@jeremyfelt jeremyfelt changed the title __getExperimentalOffset timezone offset may be a string or integer __getExperimentalSettings timezone offset may be a string or integer Mar 25, 2021
@ciampo
Copy link
Contributor

ciampo commented Aug 23, 2022

cc @noisysocks as he's made some changes in #43005 related to this issue

@noisysocks
Copy link
Member

noisysocks commented Aug 26, 2022

In #43005 I updated the types to reflect reality, but agree it should always be a number.

  • Either we tweak setSettings to make sure the this number is not a string (notice it doesn't have to be an integer; it can be a float such as 1.5).
  • Or we fix WordPress core itself so that it initializes this value correctly.

Doing both sounds good to me 🙂

@ciampo
Copy link
Contributor

ciampo commented Aug 26, 2022

Doing both sounds good to me 🙂

Sounds good to me too!

Has anyone involved in this conversation got any capacity to work on this? It looks like some work had already been started in #25684 (which got approved, but never merged)

@noisysocks
Copy link
Member

I fixed the call to setSettings() in Core in https://core.trac.wordpress.org/ticket/56459.

@Mamaduka
Copy link
Member

@noisysocks, does this mean we can close #25684?

@noisysocks
Copy link
Member

I think #25684 addresses the first option in #21977 (comment).

@jeremyfelt
Copy link
Member Author

  • A timezone of America/Los_Angeles set through General Settings in the WordPress admin results in an integer offset value of -7 (currently in DST).
  • A timezone of UTC-7 results in a string offset value of "-7".
  • A timezone of UTC results in an integer offset value of 0.
  • A timezone of UTC+0 results in a string offset value of "0".

I've tested all of these with wp.date.__experimentalGetSettings().timezone.offset and (since 6.1) wp.date.getSettings().timezone.offset and everything looks good. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Date /packages/date
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants