Make WordPress Core

Opened 10 years ago

Closed 5 years ago

Last modified 5 years ago

#25768 closed defect (bug) (fixed)

date_i18n() is wrong for certain formats, escapes sequences and daylight saving time

Reported by: raubvogel's profile raubvogel Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version: 3.7.1
Component: Date/Time Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

date_i18n() returns wrong values in the following cases:

  • If time zone is not GMT date_i18n( 'c', time() ) does not return the correct difference to GMT part, e.g., 2013-10-30T14:00:00+00:00 instead of 2013-10-30T14:00:00+01:00.
  • The time zone offset is sometimes wrong:
    /*
    * Test is called in time zone Europe/Berlin at a date where daylight saving time is off!
    * (Daylight saving time in Europe/Berlin was switched off at 2013-10-27)
    */
    // results 2013-10-29T22:15:03+01:00 while daylight saving time (Europe/Berlin) is off; result is correct
    echo(date_i18n('Y-m-d\TH:i:sP', 1383084903));
    // results 2013-10-11T19:37:20+01:00 while daylight saving time (Europe/Berlin) is on; result is wrong! should be +02:00!
    echo(date_i18n('Y-m-d\TH:i:sP', 1381520240));
    
  • date_i18n( 'D | \D | \\D | \\\D | \\\\D | \\\\\D | \\\\\\D' ) does not escaping properly.

The implementation of date_i18n() is not very well at all. Therefore it was completely rewritten solving the issues above. Performance should be better too. I added some tests too.

This ticket is a result of the discussion in ticket #20973. You can find a more detailed discussion there.

Attachments (6)

functions.php.patch (7.4 KB) - added by raubvogel 10 years ago.
tests.php (1.3 KB) - added by raubvogel 10 years ago.
core-fixes.php (5.0 KB) - added by raubvogel 10 years ago.
Fixes date_i18n() by filtering (no core modification).
wp-timezone-simple-date-i18n.diff (4.8 KB) - added by remcotolsma 6 years ago.
Introduced wp_timezone() function, simplified date_i18n() function.
wp-timezone-simple-date-i18n-v2.diff (4.8 KB) - added by remcotolsma 6 years ago.
Retry: Introduced wp_timezone() function, simplified date_i18n() function.
date-i18n.patch (9.7 KB) - added by Rarst 5 years ago.
Needs wp_timezone() merged.

Download all attachments as: .zip

Change History (18)

@raubvogel
10 years ago

#1 follow-up: @raubvogel
10 years ago

A further issue with date_i18n() that is not fixed with my patch is: If get_option('timezone_string') is empty, date_i18n() ignores the time zone.

I believe, it’s not easy to fix this. One cannot simply find a time zone string by gmt offset (get_option('gmt_offset')) because it seems that there does not exist a time zone string for each possible offset (have a look timezone_name_from_abbr() too).

@raubvogel
10 years ago

Fixes date_i18n() by filtering (no core modification).

#2 follow-up: @nacin
10 years ago

  • Keywords needs-unit-tests added; dev-feedback removed
  • Milestone changed from Awaiting Review to Future Release

Hi raubvogel, thanks for the great effort here.

I'm order for this to proceed, I think we're going to need a significant amount of test coverage, including tests that pass for the existing code as well (to help guard against regressions). It would be best if we had PHPUnit tests for this: http://make.wordpress.org/core/handbook/automated-testing/.

#3 in reply to: ↑ 2 @raubvogel
10 years ago

Replying to nacin: Thanks for Your attention! Indeed, we need much testing … At the moment I have not enough time to write automated tests :-( But in a few weeks maybe. However, it would be great if date_i18n() could be fixed.

Last edited 10 years ago by raubvogel (previous) (diff)

#4 @chriscct7
8 years ago

  • Keywords dev-feedback added
  • Severity changed from major to normal

#5 in reply to: ↑ 1 @SergeyBiryukov
8 years ago

Replying to raubvogel:

A further issue with date_i18n() that is not fixed with my patch is: If get_option('timezone_string') is empty, date_i18n() ignores the time zone.

Related: #34835

@remcotolsma
6 years ago

Introduced wp_timezone() function, simplified date_i18n() function.

@remcotolsma
6 years ago

Retry: Introduced wp_timezone() function, simplified date_i18n() function.

#6 @Rarst
5 years ago

  • Keywords has-unit-tests added; needs-testing needs-unit-tests dev-feedback removed

Added unit tests and refreshed patch (awesome work @remcotolsma !) with latest take on wp_timezone() from #24730 and some extra handling for legacy use cases.

I imagine bulk of new implementation will get moved to an upcoming wp_date() and date_i18n() will be reduced to a legacy wrapper.

This ticket was mentioned in Slack in #core-datetime by rarst. View the logs.


5 years ago

#8 @Rarst
5 years ago

#43530 was marked as a duplicate.

@Rarst
5 years ago

Needs wp_timezone() merged.

#9 @SergeyBiryukov
5 years ago

  • Milestone set to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#10 @SergeyBiryukov
5 years ago

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

In 45882:

Date/Time: Rewrite and simplify date_i18n() using wp_timezone() to address multiple issues with certain date formats and timezones, while preserving some extra handling for legacy use cases.

Improve unit test coverage.

Props Rarst, remcotolsma, raubvogel.
Fixes #25768.

#11 @SergeyBiryukov
5 years ago

In 45884:

Date/Time: Revert unintended changes from [45882].

Props TimothyBlynJacobs.
See #25768.

#12 @SergeyBiryukov
5 years ago

In 45902:

Date/Time: Correct delta comparison in test_should_return_wp_timestamp().

See #25768.

Note: See TracTickets for help on using tickets.