Make WordPress Core

Opened 19 months ago

Last modified 3 months ago

#57035 reopened defect (bug)

Error in current_time() function when using timestamp and no value for gmt_offset

Reported by: nick_thegeek's profile Nick_theGeek Owned by: audrasjb's profile audrasjb
Milestone: 6.6 Priority: normal
Severity: normal Version: 5.3
Component: Date/Time Keywords: has-patch has-unit-tests needs-testing php80 dev-feedback
Focuses: Cc:

Description (last modified by SergeyBiryukov)

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 (36)

This ticket was mentioned in PR #3581 on WordPress/wordpress-develop by NicktheGeek.


19 months ago
#1

  • Keywords has-patch added

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

#2 @SergeyBiryukov
19 months ago

  • Component changed from General to Date/Time
  • Description modified (diff)
  • Keywords php8 added
  • Milestone changed from Awaiting Review to 6.2

#3 @SergeyBiryukov
19 months ago

Hi there, welcome back to WordPress Trac! Thanks for the report.

The change makes sense to me at a glance. The (int) casting was added in [45856] / #40653. The suggested placement does look more correct.

Last edited 18 months ago by SergeyBiryukov (previous) (diff)

#4 @johnbillion
18 months ago

  • Version changed from 6.1 to 5.3

#5 @Nick_theGeek
18 months ago

May I ask why version was changed to 5.3?

#6 @johnbillion
18 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 @Nick_theGeek
18 months ago

Makes sense. Thank you! I had misunderstood that to mean the target version for inclusion.

#8 @audrasjb
17 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 @audrasjb
17 months ago

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

In 55054:

Date/Time: Prevent errors in current_time() when using timestamp and no value for gmt_offset.

This changeset moves typecasting to affect the get_option value, which ensures that when math is done it does not generate any error. In PHP 7.4 and earlier the previous implementation was dismissed as a warning, but in PHP 8+ it would have throw a fatal error.

Follow-up to [45856].

Props Nick_theGeek, SergeyBiryukov, johnbillion.
Fixes #57035.

#11 @SergeyBiryukov
16 months ago

#57623 was marked as a duplicate.

#12 @Rarst
14 months ago

#57998 was marked as a duplicate.

#13 @Rarst
14 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.

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

#14 @SergeyBiryukov
14 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 @audrasjb
14 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 @Rarst
14 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 @hellofromTonya
14 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?

  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
14 months ago

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
14 months ago

  • Keywords dev-feedback added

Marking [55598] for double committer signoff to backport to the 6.2 branch.

#20 @desrosj
14 months ago

  • Keywords commit added

[55598] looks good for backport to 6.2.

#21 @hellofromTonya
14 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.

#22 @hellofromTonya
14 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.


14 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 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/57035

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


11 months ago

#25 @oglekler
11 months ago

  • Keywords needs-testing early added

This ticket was discussed in the recent bug scrub. early was suggested due to the impact which time can have. And it needs testing as well.

@hellofromTonya any updates on this ticket, information for testing etc.?

Props: @Clorith @costdev

#26 @audrasjb
11 months ago

  • Milestone changed from 6.3 to 6.4

Given we added early tag, this ticket should be in the next milestone. Moving to 6.4.

#27 @hellofromTonya
10 months ago

  • Keywords php80 added; php8 removed

Reviewed this ticket to determine if it is an incompatibility with PHP 8.0 and should be listed in the known incompatibilities for WP 6.3.

Findings: no, this is not an incompatibility issue with PHP 8.0 as the error (though lower than fatal) happens in < PHP 8.0. Therefore, the php-compatibility focus should not be added to the ticket.

The php80 keyword is due to the error severity as PHP 8.0 increased the error from Warning to Fatal.

#28 @hellofromTonya
9 months ago

  • Keywords early removed

I've removed the early keyword. Why?

The handbook explains the keyword as:

This keyword should only be applied by committers. The keyword signals that the ticket is a priority, and should be handled early in the next release cycle.

early means the commits need to happen during the early part of the Alpha cycle.

Though it would be great to prepare the work for commit as early as possible, commits for this ticket should not be constrained to only happen during the early Alpha phase. Rather, defect commits can happen up to 6.4 RC 1.

To aid in 6.4 tracking and scrubs, I've removed the early keyword.

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

#29 @rleeson
9 months ago

  • Keywords changes-requested removed

This PR #5094 addresses this and other instances in Core where the gmt_offset option is used. I submitted the PR and resolution primarily against Ticket #58986.

I'm unsure how to appropriately tag and notate this?

#30 @oglekler
8 months ago

There is no update, it still needs testing, copying the link for visibility. https://core.trac.wordpress.org/ticket/58986#comment:10

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


8 months ago

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


7 months ago

#33 @nicolefurlan
7 months ago

  • Keywords dev-feedback added
  • Milestone changed from 6.4 to 6.5

This ticket was discussed in today's bug scrub.

We decided to move this (and #58986) to 6.5. Per @hellofromTonya:

I'm concerned about both of these tickets going in this late in the release due to the lack of test coverage and testing. Why? Casting with inval() for the one and (float) for the other may have side effects. Thinking it needs a contextual dive into how the code got to the current state to avoid potentially introducing a regression.

@SergeyBiryukov or @rarst if you have time to review and have confidence in this fix, don't hesitate to pull it back into the 6.4 milestone.

Props to @hellofromTonya :)

#34 @rleeson
7 months ago

Can you please qualify what is the "lack of test coverage" referenced above?

The PR adds test class Tests_Date_CurrentGmtOffset to cover various option values:

  • Non-numeric string => 0
  • Boolean true => 0
  • Uninitialized/Unset Option => 0
  • Float => Float

This does replace a call inside of current_time which then casts to int, though this only was a preexisting cast and its set of tests also continue to pass.

If there is some additional context or criteria about what needs to be added, I can add some more tests.


RE: The functionality choices and interface - This could add a temporary variable storage and type check prior to avoid any potential issues with an array or object stored to the option.

Also, can contributors start adding return types to functions? As a new function, it seems a good candidate.

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


4 months ago

#36 @swissspidy
3 months ago

  • Milestone changed from 6.5 to 6.6
Note: See TracTickets for help on using tickets.