WordPress.org

Make WordPress Core

Opened 10 months ago

Closed 8 months ago

Last modified 7 months ago

#37910 closed enhancement (fixed)

Remove confusing legacy logic in date_i18n()

Reported by: jdgrimes Owned by: swissspidy
Milestone: 4.7 Priority: normal
Severity: normal Version: 3.0
Component: Date/Time Keywords: has-patch commit
Focuses: Cc:

Description

In #37634 it was pointed out that date_i18n() contains the following line:

$datefunc = $gmt? 'gmdate' : 'date';

However, there is actually no difference between date() and gmdate() in WordPress, so that line can be removed. At the time that this logic was added, WordPress was experimenting with setting the default PHP time based on the site's timezone. But in the end it was decided to just always have the default PHP timezome be UTC/GMT. Which means that date() and time() always return UTC values, and thus using gmdate() is unnecessary. (I discovered this just the other day when I was researching a date/time-related bug in a plugin.) See #9588 and [12727].

Retaining this legacy logic is confusing, since it implies that there is some difference in WordPress between using date() and gmdate(), when there really is none. date() should just be used instead.

Note that although that particular line in the function is unnecessary, the $gmt parameter is still needed for these lines:

<?php
    if ( false === $i ) {
        if ( ! $gmt )
            $i = current_time( 'timestamp' );
        else
            $i = time();
        // we should not let date() interfere with our
        // specially computed timestamp
        $gmt = true;
    }

However, they could be simplified to just:

<?php
    if ( false === $i ) {
        $i = current_time( 'timestamp', $gmt );
    }

We should probably consider removing every reference to gmdate() in core in favor of just using date(), to avoid this confusion. But maybe that would ultimately be for a separate ticket.

Attachments (4)

37910.diff (1.8 KB) - added by jdgrimes 10 months ago.
Remove legacy logic and always use date() in date_i18n()
37910.2.diff (4.5 KB) - added by swissspidy 9 months ago.
37910.3.diff (5.2 KB) - added by swissspidy 9 months ago.
37910.4.diff (1.7 KB) - added by ocean90 8 months ago.

Download all attachments as: .zip

Change History (27)

@jdgrimes
10 months ago

Remove legacy logic and always use date() in date_i18n()

#1 @jdgrimes
10 months ago

  • Keywords has-patch added

It looks like gmdate() is used quite a few places in core (~50), so if we want to address that, it should be in a separate ticket I guess.

#2 @SergeyBiryukov
10 months ago

  • Milestone changed from Awaiting Review to 4.7

#3 @SergeyBiryukov
10 months ago

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

#4 @swissspidy
10 months ago

  • Keywords needs-unit-tests added

This ticket was mentioned in Slack in #core by stevenkword. View the logs.


9 months ago

@swissspidy
9 months ago

#6 @swissspidy
9 months ago

37910.2.diff adds some initial tests

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


9 months ago

#8 @swissspidy
9 months ago

  • Owner changed from SergeyBiryukov to swissspidy
  • Status changed from reviewing to accepted

@swissspidy
9 months ago

#9 @swissspidy
9 months ago

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

#10 @swissspidy
8 months ago

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

In 38804:

Date/Time: Remove some legacy logic in date_i18n().

Since there's no difference between using date() and gmdate() in WordPress, we can simply use the former in date_i18n() to reduce its complexity.

Adds tests.

Props jdgrimes for initial patch.
Fixes #37910.

#11 @pento
8 months ago

The unit tests for this have a race condition, and are failing intermittently, see #38381.

#12 @swissspidy
8 months ago

In 38871:

Date/Time: Fix unit tests after [38804].

The tests for date_i18n() need to use a delta for comparing timestamps.

Fixes #38381. See #37910.

#13 @johnbillion
8 months ago

  • Keywords needs-testing added; has-patch has-unit-tests commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Tests_Date_I18n::test_adjusts_format_based_on_timezone_string() broke when Europe moved out of DST last night.

The test was updated in [39012] and [39013], but this needs to be investigated to see what exactly broke, or whether the test is faulty.

This ticket was mentioned in Slack in #core by helen. View the logs.


8 months ago

#15 follow-up: @swissspidy
8 months ago

Tests_Date_I18n::test_adjusts_format_based_on_timezone_string() was added to cover this part of `date_i18n()` which replaces date format strings based on the site's timezone. So it's not really about comparing the time, but about checking whether that replacement works as expected. Hence the unusual date format string.

The logic in date_i18n() is still valid, but the test is faulty. I need to replace it with something a bit more flexible.

#16 in reply to: ↑ 15 ; follow-up: @ocean90
8 months ago

Replying to swissspidy:

The logic in date_i18n() is still valid, but the test is faulty. I need to replace it with something a bit more flexible.

What about using a timezone which has the same UTC offset all year like America/Regina?

#17 in reply to: ↑ 16 @swissspidy
8 months ago

Replying to ocean90:

Replying to swissspidy:

The logic in date_i18n() is still valid, but the test is faulty. I need to replace it with something a bit more flexible.

What about using a timezone which has the same UTC offset all year like America/Regina?

That… makes sense.

@ocean90
8 months ago

#18 @swissspidy
8 months ago

  • Keywords has-patch added; needs-testing removed

@ocean90 Thanks for the patch! You might not have known, but uploading an attachment doesn't trigger a notification :P

#19 @ocean90
8 months ago

  • Keywords commit added

Right, I was waiting for the build which took a bit longer and so I forget to comment. And here's a little script to show that America/Regina has the same offset: https://3v4l.org/L59eg

This ticket was mentioned in Slack in #core by helen. View the logs.


8 months ago

#21 @swissspidy
8 months ago

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

In 39167:

Date/Time: Improve date_i18n() timezone tests.

This uses a timezone without DST for the tests covering date_i18n().

Fixes #37910.

#22 @Rarst
8 months ago

I would argue that this should be normalized on gmdate() on the contrary. If any third party code changes PHP timezone (yeah, it shouldn't but in practice happens all the time), all of that will be waaaay off in context of other issues with date_i18n().

Also see #38771

Last edited 8 months ago by Rarst (previous) (diff)

#23 @swissspidy
7 months ago

@Rarst Yeah unfortunately the same goes other date functions as well, see #38940.

Note: See TracTickets for help on using tickets.