WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 15 months ago

Last modified 11 months ago

#38771 closed enhancement (fixed)

date_i18n() arguments are misleading in function and documentation

Reported by: Rarst Owned by: SergeyBiryukov
Milestone: 5.1 Priority: normal
Severity: normal Version: 4.7
Component: Date/Time Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

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.

Attachments (3)

Clarified_date_i18n_args_and_fixed_GMT_one_.patch (2.2 KB) - added by Rarst 18 months ago.
Clarified arguments and fixed current narrow case of what GMT one does.
trac-38771.patch (3.5 KB) - added by Rarst 15 months ago.
Added unit test for gmt argument fix.
trac-38771.diff (3.2 KB) - added by Rarst 15 months ago.
Added improved inline doc for gmt argument.

Download all attachments as: .zip

Change History (17)

#1 @jdgrimes
3 years ago

Related: #37910

#2 @swissspidy
3 years ago

  • Milestone changed from Awaiting Review to Future Release

#3 @purpled
3 years ago

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.

#4 @Rarst
19 months ago

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
18 months ago

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

#5 @Rarst
18 months ago

  • Keywords has-patch added

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().

#6 @SergeyBiryukov
15 months ago

  • Milestone changed from Future Release to 5.0
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#7 @SergeyBiryukov
15 months ago

  • Keywords needs-unit-tests added

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.

#8 @SergeyBiryukov
15 months ago

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.

#10 @Rarst
15 months ago

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
15 months ago

Added unit test for gmt argument fix.

#11 @flixos90
15 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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
15 months ago

Added improved inline doc for gmt argument.

#12 @Rarst
15 months ago

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.

#13 @flixos90
15 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

#14 @johnbillion
11 months ago

  • Milestone changed from 5.0 to 5.1
Note: See TracTickets for help on using tickets.