#57998 closed defect (bug) (fixed)
current_time function: GMT offset is not always an integer
Reported by: | reputeinfosystems | Owned by: | 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 )
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)
#2
@
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
@
21 months ago
@SergeyBiryukov @audrasjb I think my solution will work for both situations. Pleas do the needful.
#4
@
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
@
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.
This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.
21 months ago
#9
@
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?
#10
follow-up:
↓ 11
@
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
@
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
@
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 afloat
, but somehow completely forgot about that here. Good catch!
#13
@
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
@
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
@
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 tofloat
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
@
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?
- 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.
- The impacted user base includes India, Australia, and others, i.e. large population areas.
- Even if a fast 6.2.1 were released, the impacts could already have been felt by business owners, customers, extenders, and users.
- 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
@
21 months ago
- Owner set to hellofromTonya
- Resolution set to fixed
- Status changed from reopened to closed
In 55598:
#20
@
21 months ago
Wonder if could cast to float?
return $gmt ? time() : time() + ( (float) get_option( 'gmt_offset' ) * HOUR_IN_SECONDS );
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
#25
Closing this in favor of https://github.com/hellofromtonya/wordpress-develop/pull/5
@hellofromTonya commented on PR #4264:
4 months ago
#26
Whoopsie, I misunderstood 🤦 @peterwilsoncc's PR are suggestions to apply to this PR.
Hi there! thanks for ticket.
The above change committed in https://core.trac.wordpress.org/changeset/55054.
Ping @SergeyBiryukov @audrasjb for thoughts.