Make WordPress Core

Opened 21 months ago

Closed 21 months ago

Last modified 4 months ago

#57998 closed defect (bug) (fixed)

current_time function: GMT offset is not always an integer

Reported by: reputeinfosystems's profile reputeinfosystems Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.2 Priority: normal
Severity: normal Version: 6.2
Component: Date/Time Keywords: has-patch has-unit-tests
Focuses: Cc:

Description (last modified by sabernhardt)

Hello,

We found a bug in the WordPress 6.2 RC with the function current_time located in wp-includes\functions.php. Before 6.2 RC, the line under this function was like as below.

<?php
    return $gmt ? time() : time() + (int)( get_option( 'gmt_offset' ) * HOUR_IN_SECONDS );
?>

While in WordPress 6.2 RC, it gets changes and integer typecasting moved inside the brackets.

<?php
    return $gmt ? time() : time() + ( (int) get_option( 'gmt_offset' ) * HOUR_IN_SECONDS );
?>

This creates time difference when the site timezone is like '+11:30', '+05:30', etc. When we call the function with timestamp parameter

<?php current_time('timestamp'); ?>

in WordPress 6.1.1 or lower, it returns me the gmt_offset 11.5 ( for +11:30 ), 5.5 ( for +05:30 ) and calculate properly without making any difference. 

While from WordPress 6.2, first it'll convert the gmt_offset to integer and that considers 30 minutes less than original. Before WordPress 6.2, the gmt_offset first converted into seconds and then it typecasted to integer but with 6.2 first it typecasting gmt_offset to integer and then converting to seconds so it makes the 30 minutes difference on above examples.

We would suggest that if it's needed to type cast only gmt_offset (inside the bracket as per WordPress 6.2) then either use float or double so that the above timezone makes no difference and calculate proper timestamp.

Suggested change:

<?php
    return $gmt ? time() : time() + ( (float) get_option( 'gmt_offset' ) * HOUR_IN_SECONDS );
?>

Thanks

Change History (27)

#1 @mukesh27
21 months ago

Hi there! thanks for ticket.

The above change committed in https://core.trac.wordpress.org/changeset/55054.

Ping @SergeyBiryukov @audrasjb for thoughts.

#2 @sabernhardt
21 months ago

  • Description modified (diff)
  • Summary changed from Issue in current_time function to current_time function: GMT offset is not always an integer

#3 @reputeinfosystems
21 months ago

@SergeyBiryukov @audrasjb I think my solution will work for both situations. Pleas do the needful.

#4 @Rarst
21 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #57035.

Cheers, this does look wrong. I'll reopen issue that did a faulty fix to keep discussion in one place.

#5 @audrasjb
21 months ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone set to 6.1.2

As discussed with @hellofromTonya, reopening to handle the specific timezone issue, as #57035 was about a PHP8 compat problem :)

Moving for 6.2.1 consideration, as it appears it is currently not critical enough to delay WordPress 6.2.

#6 @audrasjb
21 months ago

  • Resolution duplicate deleted
  • Status changed from closed to reopened

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


21 months ago

#8 @sabernhardt
21 months ago

  • Milestone changed from 6.1.2 to 6.2.1

#9 @hellofromTonya
21 months ago

The questions being considered are:

  • How severe is this regression?
  • What are its impact on users?

The answers to these questions will help to determine if this can be fixed in 6.2.1 or if it must be fixed now before the 6.2 final release (causing the release to be delayed until tomorrow).

Some context:

  • Regression introduced by [55054] / #57035 to fix a fatal error in PHP 8+.
  • 10,973+ plugins use this function including:
    • The Events Calendar
    • MailPoet
    • Jetpack
    • WooCommerce
  • A quick scan of their usages shows these impacts:
    • Incorrect queries in commerce stores
    • Incorrect human readable times
    • Incorrect time for task triggers (such as MailPoet for cron workers)
  • Impacts partial-hour timezones such as +05:30 UTC.

Are there other impacts? Of those identified, how severe are they?

In discussing with @audrasjb, we're currently thinking the severity is not severe enough to warrant delaying the 6.2 final release by a day, but rather to do a fast 6.2.1 release.

What do you think?

Version 1, edited 21 months ago by hellofromTonya (previous) (next) (diff)

#10 follow-up: @Rarst
21 months ago

Time slippage can cause broken date permalinks if the posts are near midnight and shift into a different day.

#11 in reply to: ↑ 10 @hellofromTonya
21 months ago

Replying to Rarst:

Time slippage can cause broken date permalinks if the posts are near midnight and shift into a different day.

Thanks @Rarst.

As a Component Maintainer, what are your thoughts of when to fix it?

#12 @hellofromTonya
21 months ago

A note from @SergeyBiryukov he made in comment:14:ticket:57035:

Interesting, I made a note recently in comment:4:ticket:57030 that gmt_offset can be a float, but somehow completely forgot about that here. Good catch!

#13 @reputeinfosystems
21 months ago

@hellofromTonya I think this is serious for plugins which are appointment or events plugins like our BookingPress. As soon as people update to 6.2, all appointments and events will be booked incorrectly and I think most of plugin will have to release immediate patch.

#14 @Rarst
21 months ago

Time slippage was probably the most reported impact when we tightened the Date/Time core code and third party code messing with time zones shifted things around for people.

I am not really familiar with core release process.

The volume of issues would be less if only partial-hour time zones are affected, but in my opinion the severity is a lot here to just send out in stable release.

#15 @hellofromTonya
21 months ago

Thank you @Rarst and @reputeinfosystems for sharing these additional impacts. I agree that these are more significant than original thought as they can impact bookings, date permalinks, and potentially stores. Though the regression affects partial-hour timezones, these timezones include India, Australia, Sri Lanka, and more.

Currently discussing whether to revert for 6.2.0 or fix in 6.2.1 in Make/Core slack.

Also here's the regression in action along with a possible fix https://3v4l.org/tgP8m.

This ticket was mentioned in PR #4264 on WordPress/wordpress-develop by @hellofromTonya.


21 months ago
#16

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

Fixes partial-hour timezone calculation in current_time() when 'timestamp' is passed. The gmt_offset can be a float/double for partial-hour timezones such as +5:30 UTC for India and Sri Lanka.

How?

  • Changing the int type cast to float for option value.
  • Then restoring the int type cast of that calculation, i.e. restoring it before changeset 55054.

Trac ticket: https://core.trac.wordpress.org/ticket/57998

#17 @hellofromTonya
21 months ago

  • Milestone changed from 6.2.1 to 6.2

After discussing in Make/Core with the Release Squad and Committers, there is consensus to revert [55054] now to ship in 6.2.0.

Why revert?

  1. The additional impacts shared by @Rarst and @reputeinfosystems highlighted that this regression can impact bookings, date permalinks, and potentially stores for users in these partial-hour timezones.
  2. The impacted user base includes India, Australia, and others, i.e. large population areas.
  3. Even if a fast 6.2.1 were released, the impacts could already have been felt by business owners, customers, extenders, and users.
  4. And then there are the unknowns or as @desrosj noted:

    I also think that impossible to anticipate the full impact of this change.

Why not fix in 6.2.0?
There's not enough time in the cycle to validate the fix truly fixes the issue without causing an additional issue or side effect. Thus, a fix is too risky this late in the cycle. More time is needed to discuss, add more test coverage, and consider other edge cases and scenarios.

The revert will impact the original ticket that introduced the changeset, i.e. #57035.

Once reverted, then the regression reported in this ticket is fixed as the code is restored to pre-6.2 state. Thus, this ticket can be closed work continuing in #57035 with this new knowledge.

Huge props to @reputeinfosystems for reporting this regression and @Rarst and @reputeinfosystems for sharing the additional impacts!

#18 @hellofromTonya
21 months ago

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

In 55598:

Date/Time: Revert [55054].

This changeset introduced a regression for partial-hour timezones such as +05:30 UTC which is India and Sri Lanka. How? These timezones are in float. The change made in [55054] type casted them to integer which dropped the decimal for the partial-hour, making the time inaccurate. For example, +05:30 UTC (India and Sri Lanka)'s 'gmt_offset' is 5.5, but with the changeset, it was changed to 5.

Reverting the changeset restores the original state of current_time() and thus resolves the regression.

Props reputeinfosystems, Rarst, hellofromTonya, desrosj, audrasjb, sergeybiryukov, costdev, priethor, francina, nekojonez, codingchicken, cbringmann.
See #57035.
Fixes #57998.

#19 @hellofromTonya
21 months ago

In 55599:

Date/Time: Revert [55054].

This changeset introduced a regression for partial-hour timezones such as +05:30 UTC which is India and Sri Lanka. How? These timezones are in float. The change made in [55054] type casted them to integer which dropped the decimal for the partial-hour, making the time inaccurate. For example, +05:30 UTC (India and Sri Lanka)'s 'gmt_offset' is 5.5, but with the changeset, it was changed to 5.

Reverting the changeset restores the original state of current_time() and thus resolves the regression.

Props reputeinfosystems, Rarst, hellofromTonya, desrosj, audrasjb, sergeybiryukov, costdev, priethor, francina, nekojonez, codingchicken, cbringmann.
Reviewed by desrosj.
Merges [55598] to the 6.2 branch.
See #57035.
Fixes #57998.

#20 @spacedmonkey
21 months ago

Wonder if could cast to float?

return $gmt ? time() : time() +  ( (float) get_option( 'gmt_offset' ) * HOUR_IN_SECONDS );

#21 @hellofromTonya
21 months ago

In 55600:

Help/About: Add Field Guide link.

In the About page, replaces the # placeholder with the link to the Field Guide.

Follow-up to [55573], [55449], [55499], [55545].

Props vladytimy, ocean90.
Fixes #57998.

#22 @hellofromTonya
21 months ago

Sorry the above changeset was for a different ticket.

This ticket was mentioned in Slack in #core-test by ironprogrammer. View the logs.


21 months ago

@peterwilsoncc commented on PR #4264:


4 months ago
#24

@hellofromtonya I took the liberty of merging in trunk to resolve the merge conflict following the earlier revert.

@Rarst Are you able to link your GitHub and WordPress profiles so we can be sure you're given props correctly? https://make.wordpress.org/core/2020/03/19/associating-github-accounts-with-wordpress-org-profiles/

@hellofromTonya commented on PR #4264:


4 months ago
#26

Whoopsie, I misunderstood 🤦 @peterwilsoncc's PR are suggestions to apply to this PR.

Note: See TracTickets for help on using tickets.