Make dateI18n returns be affected by gmt parameter

[davilera]

Description

The PR adds a couple of tests to reproduce the issue #16218 reported by @iandunn and proposes a simple fix.

Types of changes

Bug fix.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[aduth]

I admit to not being too terribly familiar with this specific function or why it was passing the second argument previously. Maybe @iandunn could advise on whether the proposed changes here seem reasonable?

Here's relevant documentation from Moment on this parameter:

The utcOffset function has an optional second parameter which accepts a boolean value indicating whether to keep the existing time of day.

• Passing false (the default) will keep the same instant in Universal Time, but the local time will change.
• Passing true will keep the same local time, but at the expense of choosing a different point in Universal Time.

One use of this feature is if you want to construct a moment with a specific time zone offset using only numeric input values:

I also observe that between the original patches at Trac#39222 (last patch by @ocean90) and its introduction to Gutenberg in #841 by @youknowriad , only the latter included this argument, and it's not clear if there was a reason for this disparity. I realize this has been many years, but perhaps one or the other of the two might have some recollection to whether it is necessary.

[davilera]

I was asking because I think we might need to change this line too, as I did in my commit, but I'm not sure.

[davilera]

@aduth, I've been investigating this issue and here's what I've found (spoiler alert: it's a mess).

If I'm not mistaken, we want our JavaScript functions to "mimic" what we see in PHP (even though I'm not sure it's the best approach), so I thought I'd run a few tests on PHP and see the results I get in the hopes of better understanding how things currently work. Just to make sure we're on the same page, my setup is as follows:

• My PC is on Europe/Madrid (at the moment it's GMT+1)
• My WordPress is set to America/New_York
• PHP's default timezone (date_default_timezone_get()) is UTC
• My WordPress site language is set to Spanish

I'll be using the same values used in issue #16218 reported by @iandunn:

• UNIX timestamp 1560855600
• Equivalent datetime in UTC: 2019-06-18T11:00:00.000Z

PHP's date function

According to the documentation, date takes two arguments: a string $format and an optional int $timestamp. Since we're talking about a UNIX timestamp, the concrete value is "timezone free" and the result one expects from calling this function is "a string formatted according to the given format string using the given integer timestamp" using "PHP's default timezone:"

date( 'Y F d H:i e', 1560855600 );
» 2019 June 18 11:00 UTC

If we change PHP's default timezone, the date changes accordingly:

date_default_timezone_set( 'America/New_York' );
date( 'Y F d H:i e', 1560855600 );
» 2019 June 18 07:00 America/New_York

It's important to notice that, in both cases, the month is shown in English (June).

WordPress' date_i18n function

According to the documentation, date_i18n can take up to three arguments: a string $format, an optional int|bool $timestamp_with_offset, and an optional bool $gmt. It's important to mention that one should only set $gmt to true if $timestamp_with_offset is set to false.

IMHO, what this function does is a little bit complicated (pardon my ignorance):

Retrieves the date in localized format, based on a sum of Unix timestamp and timezone offset in seconds.

If the locale specifies the locale month and weekday, then the locale will take over the format for the date. If it isn’t, then the date format string will be used instead.

Note that due to the way WP typically generates a sum of timestamp and offset with strtotime, it implies offset added at a current time, not at the time the timestamp represents. Storing such timestamps or calculating them differently will lead to invalid output.

Let's take a look at several examples and see what we get:

Case 1. Using a unix timestamp

date_i18n( "Y F d H:i e", 1560855600 );
» 2019 junio 18 11:00 America/New_York

Case 2. Using strtotime with a ISO 8601 formats:

With 2019-06-18T11:00:00.000Z, I would expect 2019 junio 18 07:00 America/New_York but this is what I get instead:

date_i18n( "Y F d H:i e", strtotime( '2019-06-18T11:00:00.000Z' ) );
» 2019 junio 18 11:00 America/New_York

I know that 2019-06-18T11:00:00.000Z is 2019-06-18 13:00 Europe/Madrid, so let's see what we get with a date string localized in Madrid:

date_i18n( "Y F d H:i e", strtotime( '2019-06-18T11:00:00.000Z' ) );
» 2019 junio 18 11:00 America/New_York

Sure, it's coherent with the previous example... but that's not what I'd expect 😕 Apparently, it's returning the UTC equivalent date (in UTC it's "11am") but with WordPress' timezone (America/New_York). Is this correct? No idea.

If we ignore for a moment how time is being converted, it's important to notice that the month is now written in Spanish (i.e. the language in my WordPress site): junio.

My thoughts

I think JavaScript's date* functions are a mess right now and very difficult to understand. In my opinion, we need to address the following three issues:

1. What's the timezone of a date used as an argument of any of these functions?

In JavaScript, it looks like the accepted dates can be Date|string|Moment|null. Most of the times, such a date will have a clear timezone (for instance, 2019-06-18T11:00:00.000Z is UTC and 2019-06-18 11:00 Europe/Madrid is GMT+0200), especially if they're already a Date or a moment object.

But if the given date doesn't specify the timezone (like, for instance, the string "2019-06-18 11:00") we have to decide the "default" timezone of the argument. I'd recommend it's the timezone defined in WordPress so, in my example, "2019-06-18 11:00" would be equivalent to "2019-06-18 11:00 America/New_York".

2. What's the timezone of the resulting string we get when formatting a given date with these functions?

The functions that take a date as an argument and format it into a different string should take into account that the resulting string has to be in a specific timezone. Most of the times, the resulting timezone will either be UTC or the timezone set in WordPress. In general, though, it can be any timezone.

The target timezone could be another argument in our function: date( string dateFormat, Date|string|Moment|null dateValue, string dateTimezone ).

3. Should the resulting string be translated or not?

date_i18n seems to translate words into the appropriate locale (June became junio in my installation). I believe this should be the only difference between date and dateI18n.

Of course, we could argue if we need yet an extra argument to specify a different language other than the one used in WordPress (just a s we accepted other timezones in the previous point).

What are your thoughts on this, guys?

[davilera]

@aduth, I've spent some time digging into this issue and I think I have a clearer understanding now.

Essentially, when working with the @wordpress/date package we just need two things:

1. What timezone (or utcOffset) is used in the dateValue argument of our functions.
2. What timezone should be used in the formatted result.

Timezone/UTC offset in an argument

Usually, the timezone we use in our arguments is explicit. That is, if we use a moment object, the UTC offset is built in it (I think that's also the case with JS built-in Date objects). If, on the other hand, they're strings, the offset can also be explicit:

• 2020-01-09T15:00:00.000Z is UTC
• 2020-01-09 17:00+0200 is UTC+2

but sometimes it's not and, therefore, the date is ambiguous:

• 2020-01-09 10:00. Is it 10am UTC? Is it 10am in the user's browser/computer? Is it 10am in WordPress’ timezone?

SOLUTION: When the given date is ambiguous, we should assume it uses WordPress' timezone.

Timezone used in a formatted result

According to the documentation, this is how the functions are supposed to work:

• date regardless of the timezone that uses the dateValue argument, the function returns a formatted result in WordPress' timezone.
• gmdate regardless of the timezone that uses the dateValue argument, the function returns a formatted result in UTC.
• dateI18n behaves as both date and gmdate, but it translates any "words" included in the result. If gmt is false (default), it behaves as date; if it's true, it behaves as gmdate.
• format, unlike the previous functions, does take into account the timezone used in the dateValue argument. So, if the argument is in UTC, the result is in UTC; if it's in +0200, the result is in UTC+2, and so on. Notice that if dateValue is ambiguous and doesn't specify a specific timezone, we should assume it uses WordPress' (as I proposed above).

Next Steps

So, if we all agree on this, I can prepare a test suite that validates this behavior in my PR so that the current implementation fails (as originally reported in #16218). Then, I'll work on a fix.

[ocean90]

It might be good idea to connect with @Rarst since he did/is doing some great work to fix date handling on the PHP side, see https://make.wordpress.org/core/2019/09/23/date-time-improvements-wp-5-3/ for example.

[Rarst]

I am not entirely sure about JS context of the issue, but on PHP side date_i18n() is pretty much insane, illogical, incompatible with Unix timestamps, and on its way to be eventually deprecated.

WordPress 5.3+ replacement and now "main" date formatting function is wp_date(), date_i18n() is now a legacy wrapper around it (preserving its crazy behaviors for BC).

[davilera]

As discussed in our last JS meeting, @aduth, I've applied the minimum number of changes required and implemented a bunch of tests that verify the expected behavior. In particular, tests:

• Function dateI18n should format date into a UTC date if gmt is set to true
• Function dateI18n should format date into a date that uses site’s timezone if gmt is set to false

validate that the original issue (#16218) is fixed. Therefore, this PR is ready to be merged.

Now, there's only one issue left, which is what should we do if the given date has an ambiguous timezone (i.e. has no timezone), as in 2020-01-20 or 2020-01-20 10:00. I think the default behavior should be: "assume it's in WordPress’ timezone"... but I'd like to address this in a different issue+PR, if that makes sense.

[Rarst]

Function dateI18n should format date into a UTC date if gmt is set to true
Function dateI18n should format date into a date that uses site’s timezone if gmt is set to false

Do note that this isn't consistent with how date_i18n() actually behaves in PHP.

#16218 (comment)

Not sure what the design for JS counterpart is supposed to be, but if it diverges from PHP original while keeping a very close name it would need to be thoroughly documented that behavior is not identical between the two.

[davilera]

this isn't consistent with how date_i18n() actually behaves in PHP.

You mean the third param (gmt) should only be used if the second one (date) is not set? You're right, the JS function is (in this sense) inconsistent with its PHP counterpart. But the JS doc is already different from PHP, so I think it's clear that they behave (slightly) different. And if one calls the JS function as they would in PHP (namely, dateI18n( format, undefined, true )), it'd behave as it does in PHP.

With that said, I'd personally implement these functions a little bit differently. If we want to keep date and gmdate (so that they mimic PHP's built-in functions), I think we should have dateI18n and gmdateI18n. Or maybe we have to rethink this completely and only have a few functions that let us specify source and target timezones, as well as whether the dates should be localized or not.

[Rarst]

As above, just giving some context on PHP side of things. :) Not plugged into the process of how the JS versions are designed and what expectations are. But from my point of view date_i18n() is a mess (we are getting rid of in practice), so careful with making an analogue there.

Ending up with two crazy functions that aren't consistent with each other would be... suboptimal outcome. :)

[aduth]

My understanding of the historical context is that these functions are intended to essentially be direct ports of the equivalent PHP functionality (or at least the solutions which existed at the time). Whether we'd want to reevaluate these options, especially as new solutions are emerging in PHP, I think it would be fair to consider that, though separately to the effort here. For the sake of backward-compatibility, I think the focus here should be to strive to align the two as much as possible, even if the aligned behavior is suboptimal.

But the JS doc is already different from PHP, so I think it's clear that they behave (slightly) different.

Can you elaborate on what you're considering to be the differences? Is it something you think we can't change without being considered backwards-incompatible? Would it be enough to say that the current behavior is buggy (especially if we're promoting it as intending to align to the corresponding PHP behavior)?

[Rarst]

Let's put this side by side for clarity.

dateI18n	date_i18n	wp_date
dateFormat string: PHP-style formatting string.	$format (string) (Required) Format to display the date.	$format (string) (Required) PHP date format.
dateValue (Date|string|Moment|null): Date object or string, parsable by moment.js.	$timestamp_with_offset (int|bool) (Optional) A sum of Unix timestamp and timezone offset in seconds. Default value: false	$timestamp (int) (Optional) Unix timestamp. Defaults to current time. Default value: null
gmt boolean: True for GMT/UTC, false for site's timezone.	$gmt (bool) (Optional) Whether to use GMT timezone. Only applies if timestamp is not provided. Default value: false	$timezone (DateTimeZone) (Optional) Timezone to output result in. Defaults to timezone from site settings. Default value: null

So, yes, JS and PHP is already out of sync, since date_i18n() only operates with WP timestamps. Also that quirk with GMT mode depending on timestamp not being provided.

On one hand aligning JS version with it is replicating crazy behavior. On other hand it's not aligned already.

Bonus question — what happens when someone provides timestamp to JS version? Is it supposed to treat it like Unix timestamp or like WP timestamp?

wp_date() is saner all around. It works explicitly with standard Unix timestamp and it allows to set arbitrary time zone for output, not deal with semi-broken GMT mode.

[davilera]

Can you elaborate on what you're considering to be the differences?

@Rarst did a pretty good job at summarizing the differences. Essentially, in PHP the second argument (date) is not exactly a date, but the result of a weird function, and the third one (gmt) should only be used if the second one is set to false. In JS the date is, well, a date (compatible with momentjs) and the third argument can be used even if the date arg is set (but, if it isn't, it behaves like PHP).

Personally, I like the approach followed by wp_date in PHP: give it a format string and a date, and you'll get the date in WordPress' timezone. Want a different output timezone? Tell it which one and it'll use it!

But there's a thing missing there, @Rarst and @aduth.

If my understanding is correct, wp_date localizes the output string. That is, if my site is in Spanish, all dates will be in Spanish. What if I wanted the date with no translations? I'd have to use PHP built-in functions to achieve that goal... and notice date and gmdate don't have the timezone argument, so I'll probably have to use a combination of PHP functions to get "the same" wp_date provides.

My solution/My preferred scenario

If I were to implement these functions in JavaScript right now, I'd create a function named date with the same args as wp_date in PHP that is exactly the same as its PHP counterpart but doesn't translate any strings. And then I would create another function named dateI18n that's exactly like this new date that does translate the strings (and thus behaves like wp_date in PHP, assuming, again, that wp_date translates the dates into the appropriate locale). And then, if we really want both APIs to match, I'd have wp_date (no translations) and wp_date_i18n (translations) in PHP....

[davilera]

As discussed during JavaScript Chat - January 21, 2020, I've modified this PR so that date and dateI18n have a signature like wp_date does in PHP (i.e. it includes a timezone argument). I also included a new gmdateI18n that's equivalent to gmdate, making a more consistent API.

Notice: dateI18n already had a third argument (gmt) which, when set to true, should format a date into UTC (ignoring WordPress' timezone). For compatibility reasons, the new timezone third argument also behaves like this when set to true, even though it's not explicitly documented in docs.

The PR also include several new tests that verify the expected behavior.

Can any of you, @aduth and @Rarst, review this?

[aduth]

Thanks for the continued work here. I left a few remarks, most of which aren't so much concerned with the implementation itself, but rather small recommendations about documentation.

[aduth]

The timezone argument isn't consistent with the documented type, in two ways:

• It's optional, but the type doesn't indicate this (related documentation)
• The first line of code in the function tests if timezone === true, but true is not a valid documented value for timezone. We should include boolean if it's expected.

Small formatting nit-picks, but relevant if there's a concern of running out of available space for the description texts:

1. The enclosing parentheses on union types are unnecessary (the timezone types as currently implemented could be expressed {string|number|null})
2. You have a few unnecessary extra spaces after your longest type tag @param {(Date|string|Moment|null)} dateValue. There only need be a single space here.

[aduth]

I see now a few lines earlier from this that it's mentioned in the documentation about the special treatment for the boolean variable for the sake of backwards-compatibility. One thing I might be worried about is that as we transition to stricter type-checking (#18838), the type-checker may struggle to reconcile or otherwise reject the fact that we document the function as not accepting a boolean, only to then proceed to test a boolean.

I think it may just be worth including boolean in the documented type, both for this reason and because it's still technically accepted. Between the documentation of the function (and perhaps an additional note in the argument description), it should be clear enough that boolean is effectively deprecated, even if still supported.

[davilera]

Fixed in c3d5fe4.

• timezone in dateI18n now includes boolean and explicitly states that "is effectively deprecated, but still supported for backward compatibility reasons."
• Also fixed union types and spacing.

[aduth]

There's still some issue here; these functions have optional arguments, but null is not a correct way to denote an optional argument (via reference documentation mentioned in #18982 (comment)).

[aduth]

Looking at the current build failure and knowing this to be related to issues yesterday with Travis + Composer, the branch may need a rebase. I tried restarting the build, but it clearly wasn't enough.

It may also be an opportunity to correct a mistake I made in squashing commits, where I accidentally made myself the author of the commit.

[aduth]

Thanks for the patience in working through the conversation and iterations here 👍 Glad to see it finally land.

[iandunn]

🎉 , great job @davilera :)

[davilera]

Thanks @aduth, @iandunn, and everybody else. You all helped me a lot!