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