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

[Nick_theGeek]

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.

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

[SergeyBiryukov]

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.

[Nick_theGeek]

May I ask why version was changed to 5.3?

[johnbillion]

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.

[Nick_theGeek]

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

[audrasjb]

Hello and thanks for the ticket and patch @Nick_theGeek,

The patch looks good to me and it works fine.
Self assigning for commit.

[audrasjb]

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.

Committed in https://core.trac.wordpress.org/changeset/55054

[SergeyBiryukov]

#57623 was marked as a duplicate.

[Rarst]

#57998 was marked as a duplicate.

[Rarst]

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.

[SergeyBiryukov]

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!

[audrasjb]

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.

[Rarst]

I am not following how is this "fixed", when the fix broke the function and needs to be rolled back or reworked.

[hellofromTonya]

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!

[hellofromTonya]

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.

[hellofromTonya]

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

[desrosj]

[55598] looks good for backport to 6.2.

[hellofromTonya]

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.

[hellofromTonya]

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!

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

[oglekler]

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

[audrasjb]

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

[hellofromTonya]

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.

[hellofromTonya]

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; else, the ticket gets bumped.

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.

[rleeson]

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?

[oglekler]

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

[nicolefurlan]

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 :)

[rleeson]

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.

[oglekler]

This ticket was discussed during bug scrub.

Due to uncertainty on how to proceed, I am moving this ticket to Future releases. It can be return to the next available milestone when there will be concrete solution. Thank you!

Add props to @hellofromTonya

[hellofromTonya]

Given a solution was reverted, I'm thinking this area needs a deeper contextual dive as well as consideration and testing to raise confidence of no side effects or regressions being introduced. It'll also need more test coverage (happy and unhappy paths).

The challenges:

• robust solution to handle all timezones include partial hour timezones.

  The gmt_offset can be a float/double for partial-hour timezones such as +5:30 UTC for India and Sri Lanka.
  

• review of what values gmt_offset should and could have.
• and cross-version compatibility for the PHP versions WP supports.

I'm also curious of how this is solved in other projects.

Pulling it into 6.7 and self-assigning with intent to help shepherd the effort for this ticket and #58986.

Also marking the current patches as changes-requested as there's more work to do. That said, I'm yet convinced any of the patches are the way forward. I think that's the place to start -> figure out the way forward and then build a patch for it.

[hellofromTonya]

Related: #56358, #58986, #60629.

All of these tickets are dealing with the same underlying issue.

@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/

Closing this in favor of ​https://github.com/hellofromtonya/wordpress-develop/pull/5

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

[peterwilsoncc]

The ​linked pull request has had the various changes applied and is good for commit.

It casts the gmt_offset to a float to account for partial hour time zones, such as ACST, IST, and the casts the calculated offset in seconds to an integer.

[peterwilsoncc]

In 58923:

Date/Time: Prevent type errors in current_time().

Prevents a potential type error when calling current_time( 'timestamp' ) by casting get_option( 'gmt_offset' ) to a float prior to performing calculations with the value.

This mainly accounts for incorrect storage of values, such as an empty string or city name.

Follow up to [45856], [55054], [55598].

Props hellofromtonya, peterwilsoncc, rarst, costdev, Nick_theGeek, SergeyBiryukov, johnbillion, desrosj, reputeinfosystems, audrasjb, oglekler.
Fixes #57035.

Merged r58923 / ​https://github.com/WordPress/wordpress-develop/commit/6e7b1243f30af7b85d7dff44df4c86d852dec7df