date_i18n() arguments are misleading in function and documentation

[Rarst]

date_i18n() is documented as following:

date_i18n( $dateformatstring, $unixtimestamp = false, $gmt = false )

@param string   $dateformatstring Format to display the date.
@param bool|int $unixtimestamp    Optional. Unix timestamp. Default false.
@param bool     $gmt              Optional. Whether to use GMT timezone. Default false.

Second and third arguments are highly problematic.

Let's look at this case:

var_dump( date_default_timezone_get() );     // string(3) "UTC" < WP core resets to this during boot
var_dump( get_option( 'timezone_string' ) ); // string(11) "Europe/Kiev" < site's setting
var_dump( get_option( 'gmt_offset' ) );      // float(2) < numeric offset

$now = strtotime( 'now' );
var_dump( $now ); // int(1478987901)

var_dump( date( DATE_W3C, $now ) );           // string(25) "2016-11-12T21:58:21+00:00" < PHP native UTC time, so far so good

var_dump( date_i18n( DATE_W3C, $now ) );      // string(25) "2016-11-12T21:58:21+02:00" < this is UTC time w/ timezone replaced (!!!)
var_dump( date_i18n( DATE_W3C, $now, true) ); // string(25) "2016-11-12T21:58:21+02:00" < still same, $gmt is meaningless

$now = $now + get_option( 'gmt_offset' ) * HOUR_IN_SECONDS; // "WordPress timestamp"
var_dump( date_i18n( DATE_W3C, $now, true) ); // string(25) "2016-11-12T23:58:21+02:00"  < actually "correct" time

Observations:

1. Function does not output correct "local" site time
2. Actual output consists of GMT time with site's timezone simply replacing real UTC timezone
3. $gmt argument has no effect since WP always resets timezone to GMT, it's never not GMT under normal operation
4. For correct output $unixtimestamp needs to be adjusted by timezone offset, making it anything but Unix timestamp.

This is probably way too grown in for any kind of a code change. But at the very least:

1. $unixtimestamp needs to be thoroughly documented/renamed as WP's own take on timestamp, incompatible with actual Unix timestamps.
2. $gmt could as well be deprecated since it does nothing and when it does something (timezone changed by third party code) it just further confuses the results.

[jdgrimes]

Related: #37910

[purpled]

This bit us today when we upgraded wordpress - we were calling date_i18n() and were bit by the fact the gmt flag is no longer respected when passing a timestamp. Please revert to the old behavior or comment better what the parameters do.

[Rarst]

Notably date_i18n() will not only consume "WordPress timestamp", but also output one if requested with U format, which is obviously highly inconsistent with date().

I doubt that this can be meaningfully fixed without backwards compatibility break, so probably needs to be thoroughly documented as well.

[Rarst]

Clarified arguments and fixed current narrow case of what GMT one does.

[Rarst]

I've added patch and fixed what GMT argument does at the moment, which is pretty narrow and useless though.

While it is possible to make GMT work here universally, it would need to wait until all other bugs in the functions are fixed and would be messy on top of already messy implementation.

I think at this point it would make more sense to introduce a new localisation function, based on actual Unix timestamp and feature parity with date(), then deprecate date_i18n().

[SergeyBiryukov]

Going to get the documentation changes in for starters.

The code change makes sense as well, would be great to have some unit tests though.

[SergeyBiryukov]

In 43380:

Docs: Improve documentation for date_i18n()'s second argument.

Despite previously being labeled as a Unix timestamp, in reality it's a sum of Unix timestamp and timezone offset in seconds.

Props Rarst.
See #38771.

[SergeyBiryukov]

This appears to be the only place in core that uses the $gmt argument: https://core.trac.wordpress.org/browser/tags/4.9.6/src/wp-admin/options-general.php?marks=218#L213

[Rarst]

A bit more context on problem with $gmt and pending code fix:

1. $gmt is only applied if timestamp is not provided. Otherwise it's ignored. Inline documentation needs to reflect that.
2. The output is currently broken by outputting UTC time with WP timezone, which produces incorrect current time. This is hard to notice since timezone isn't often included in output format (linked core usage doesn't have it).

I'll work on unit test and submit a new patch for the code fix part.

[Rarst]

Added unit test for gmt argument fix.

[flixos90]

I took a closer look at trac-38771.diff​. This is almost ready to go in I'd say. Three comments:

• "Whether to use GMT timezone, only applies if timestamp is not provided." could probably turned into two separate sentences for readability.
• The $gmt parameter documentation should also be updated for the date_i18n filter.
• Would it make sense for now to only set $timezone_string to "UTC" if it is generally being used (i.e. if the timezone_string option is set)? Not sure, but I could see it reducing the chance of issues with sites still relying on gmt_offset only. If we decide, it's good as is, I'd still make a minor adjustment and only look up the option if it's actually needed.

[Rarst]

Added improved inline doc for gmt argument.

[Rarst]

Would it make sense for now to only set $timezone_string to "UTC" if it is generally being used (i.e. if the timezone_string option is set)? Not sure, but I could see it reducing the chance of issues with sites still relying on gmt_offset only.

Not quite following this part. We only set it to UTC if we have time zones in format, so we are going to need it. And if we are in a GMT mode then we don't care about gmt_offset or timezone_string either, we are just producing UTC output regardless of WP settings.

[flixos90]

In 43389:

Date/Time: Fix usage of $gmt parameter in date_i18n() and clarify its behavior.

The docs for date_i18n() and its filter now correctly state that the $gmt parameter is only taken into account if no timestamp is provided. Furthermore, a bug with that parameter is fixed, as it is now ensured that the timezone used with it is UTC.

Props Rarst.
Fixes #38771.