Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#37634 closed defect (bug) (fixed)

Wrong usage of $gmt parameter in date_i18n() in options-general.php

Reported by: swissspidy's profile swissspidy Owned by: jorbin's profile jorbin
Milestone: 4.7 Priority: normal
Severity: normal Version: 3.1
Component: Administration Keywords: good-first-bug has-patch
Focuses: Cc:

Description

For showing the universal time on the "General Settings" screen, the following code is used:

date_i18n( $timezone_format, false, 'gmt' )

However, the third parameter, $gmt is actually a boolean has been that way ever since 0.71.

It should therefore look like this:

date_i18n( $timezone_format, false, true )

It currently only works because of this line in date_i18n, where gmt is implicitly true and thus gmdate is used:

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

Introduced in [9616].

Attachments (1)

37634.patch (608 bytes) - added by fronaldaraujo 8 years ago.
Passing true instead of 'gmt'

Download all attachments as: .zip

Change History (8)

#1 @jdgrimes
8 years ago

Actually, there is no difference between date() and gmdate() in WordPress, so that line can be removed. At the time 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 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].

I'd suggest that every reference to gmdate() in core be changed to date() to avoid this confusion.

#2 @jdgrimes
8 years ago

Note that the $gmt parameter is still needed for these lines in date_i18n():

<?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 );
    }

I guess maybe we'll want to open a separate ticket for that though.

#3 follow-up: @swissspidy
8 years ago

Feel free to open a new ticket for that :-) Such a change would need lots of tests beforehand though.

Simplifying date_i18n would probably also help for #18146.

@fronaldaraujo
8 years ago

Passing true instead of 'gmt'

#4 @fronaldaraujo
8 years ago

  • Keywords has-patch added; needs-patch removed

Attached a patch that fixes the last parameter.

#5 @swissspidy
8 years ago

  • Milestone changed from Awaiting Review to 4.7

#6 @jorbin
8 years ago

  • Owner set to jorbin
  • Resolution set to fixed
  • Status changed from new to closed

In 38502:

Administration: Use a bool when when a bool is called for

The third parameter of date_i18n is a bool. Currently, it's a weak check, otherwise the call showing the universal time on the "General Settings" screen would be messed up. Use an actual bool so we call our own functions correctly.

Fixes #37634.
Props fronaldaraujo.

#7 in reply to: ↑ 3 @jdgrimes
8 years ago

Replying to swissspidy:

Feel free to open a new ticket for that :-) Such a change would need lots of tests beforehand though.

Simplifying date_i18n would probably also help for #18146.

I've opened #37910.

Note: See TracTickets for help on using tickets.