Opened 7 months ago
Last modified 2 months ago
#57035 reopened defect (bug)
Error in current_time() function when using timestamp and no value for gmt_offset
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.3 | Priority: | normal |
Severity: | normal | Version: | 5.3 |
Component: | Date/Time | Keywords: | has-patch php8 changes-requested has-unit-tests |
Focuses: | Cc: |
Description (last modified by )
This may be a but of an edge case, but if the gmt_offset is not set correctly a string is returned and then used in math to multiply against HOUR_IN_SECONDS.
In PHP 7 and earlier this is dismissed as a warning so processing continues, but in PHP 8 this throws a fatal error.
It looks like the function has some typecasting, but it is placed in the wrong spot.
// Don't use non-GMT timestamp, unless you know the difference and really need to. if ( 'timestamp' === $type || 'U' === $type ) { return $gmt ? time() : time() + (int) ( get_option( 'gmt_offset' ) * HOUR_IN_SECONDS ); }
Should be
// Don't use non-GMT timestamp, unless you know the difference and really need to. if ( 'timestamp' === $type || 'U' === $type ) { return $gmt ? time() : time() + ( (int) get_option( 'gmt_offset' ) * HOUR_IN_SECONDS ); }
Again, this is potentially an edge case. Another ticket, #56358, was reported and closed when the user discovered a work around, but I think this fix is pretty simple and will solve for weird edge cases that can break sites in php8.
Change History (23)
This ticket was mentioned in PR #3581 on WordPress/wordpress-develop by NicktheGeek.
7 months ago
#1
- Keywords has-patch added
#2
@
7 months ago
- Component changed from General to Date/Time
- Description modified (diff)
- Keywords php8 added
- Milestone changed from Awaiting Review to 6.2
#3
@
7 months ago
#6
@
7 months ago
The Version
field is used to indicate the version of WordPress where the bug was first introduced, it looks like it's been present since 5.3.
#7
@
7 months ago
Makes sense. Thank you! I had misunderstood that to mean the target version for inclusion.
#8
@
5 months ago
- Keywords commit added
Hello and thanks for the ticket and patch @Nick_theGeek,
The patch looks good to me and it works fine.
Self assigning for commit
.
#9
@
5 months ago
- Owner set to audrasjb
- Resolution set to fixed
- Status changed from new to closed
In 55054:
@audrasjb commented on PR #3581:
5 months ago
#10
Committed in https://core.trac.wordpress.org/changeset/55054
#13
@
2 months ago
- Resolution fixed deleted
- Status changed from closed to reopened
As reported in #57998 the fix is faulty because gmt_offset
can be a float-like value for partial-hour offsets (e.g. +11:30). The product of offset and hour in seconds should be integer (as expressed by original logic), but the offset should not.
Probably needs unit test for the case, if it slipped through the existing ones.
#14
@
2 months ago
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!
#15
@
2 months ago
- Keywords commit removed
- Resolution set to fixed
- Status changed from reopened to closed
The release squad is currently discussing the issue (check #6-2-release-leads
channel for more info, or this thread).
By the way, as discussed with @hellofromTonya, it would be better to use #57998 for this discussion, as #57035 was opened to address a different issue (PHP8 compat issue, which is fixed now).
Closing as fixed for now, and reopening #57998 for 6.2.1 to handle the timezone issue.
#16
@
2 months ago
I am not following how is this "fixed", when the fix broke the function and needs to be rolled back or reworked.
#17
@
2 months ago
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening to revert [55054].
The discussion from comment:17:ticket:57998
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!
#19
@
2 months ago
- Keywords dev-feedback added
Marking [55598] for double committer signoff to backport to the 6.2 branch.
#22
@
2 months ago
- Keywords changes-requested needs-unit-tests added; dev-feedback commit removed
- Milestone changed from 6.2 to 6.3
Resetting the keywords and moving to 6.3 after the revert.
With the gained knowledge of partial-hour timezones using float 'gmt_offset'
(see #57998), the patch will need adjustments.
Also the existing tests for current_time()
need additional datasets to test for a variety of timezones, especially those using 'timestamp'
or 'U'
. Data providers can be used.
Thank you everyone for your contributions!
This ticket was mentioned in PR #4264 on WordPress/wordpress-develop by @hellofromTonya.
2 months ago
#23
- Keywords has-unit-tests added; 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/57035
Previously the typecasting to (int) happened after math was done, which will always be an integer. However the get_option can return an empty string, so that php8 results in a fatal error.
This moves the typecasting to affect the get_option value, which will ensure that when math is done it doesn't generate a fatal error.
Trac ticket: 57035