Make WordPress Core

Opened 2 years ago

Closed 3 months ago

Last modified 3 months ago

#56358 closed defect (bug) (fixed)

PHP8 TypeError on current_time( 'timestamp' ) if timezone is set to GMT / 0

Reported by: toastercookie's profile toastercookie Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.7 Priority: normal
Severity: minor Version: 6.0
Component: Date/Time Keywords: php80 has-patch commit
Focuses: php-compatibility Cc:

Description

Just a minor bug to report, seems that if you have your timezone set to GMT, current_time( 'timestamp' ) throws a fatal error on PHP8, I think because 0 is being sent as a string. I can fix by changing my timezone to anything else.

PHP Fatal error: Uncaught TypeError: Unsupported operand types: string * int in /wp-includes/functions.php:75
Stack trace: #0 /RSSItemPull.php(286): current_time('timestamp')

Change History (26)

#1 @Rarst
2 years ago

Do you mean UTC? GMT is currently not an option in WordPress time zone settings and I can't reproduce it on PHP 8.1.

I do not see how this error could happen with current version of the current_time() code. Which WordPress version are you on?

I do see that option could return "0" there (for UTC+0 time zone). However, from quick check, multiplying that by an integer is a valid operation in PHP 8, it would type cast it to a number. https://3v4l.org/sGXiQ

Last edited 2 years ago by Rarst (previous) (diff)

#2 in reply to: ↑ description @SergeyBiryukov
2 years ago

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

Replying to toastercookie:

Just a minor bug to report, seems that if you have your timezone set to GMT, current_time( 'timestamp' ) throws a fatal error on PHP8, I think because 0 is being sent as a string. I can fix by changing my timezone to anything else.

I could not reproduce the error in my testing. As noted above, '0' should not be an issue, as it is still a numeric string.
Could you look into what get_option( 'gmt_offset' ) returns, or what is the value in the wp_options table?

I can only reproduce this with a non-numeric string like Europe/London, but that should be in the timezone_string option, not in gmt_offset. Does the issue still happen on a clean install, with all plugins disabled and a default theme (Twenty Twenty-Two) activated?

Replying to Rarst:

Do you mean UTC? GMT is currently not an option in WordPress time zone settings and I can't reproduce it on PHP 8.1.

It looks like the English (UK) locale translates UTC as GMT.

#3 @toastercookie
2 years ago

  • Resolution set to invalid
  • Status changed from new to closed

Changing the timezone and then changing it back seems to have fixed it for me, so there must have been something weird in my DB being returned. I can't reproduce this on my end anymore so I will just go ahead and close this! Sorry for wasting yall's time

#4 @SergeyBiryukov
2 years ago

  • Keywords php8 added
  • Milestone Awaiting Review deleted

No worries, thanks for the follow-up!

#6 @verygoode
9 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Encountered on a site, found that in some situations the gmt_offset option may get left empty -- seemingly due to plugin conflicts causing the save function to error out before gmt_offset gets stored.

Here are two example set of reproduction instructions using WordPress 6.4.3 and PHP 8.1.

Example 1

  1. Access /wp-admin/options-general.php and set a timezone with an offset like UTC+1
  2. Edit gmt_offset option in DB, set to empty
  3. Observe /wp-admin/options-general.php's Date Format option is now experiencing a critical error with the following fatal logged. Cannot save page via UI.
PHP Fatal error:  Uncaught TypeError: Unsupported operand types: string * int in /wordpress/core/6.4.3/wp-includes/functions.php:76
Stack trace:
#0 /wordpress/core/6.4.3/wp-includes/functions.php(180): current_time('timestamp', false)
#1 /wordpress/core/6.4.3/wp-admin/options-general.php(364): date_i18n('F j, Y')
#2 {main}
  thrown in /wordpress/core/6.4.3/wp-includes/functions.php on line 76

Example 2

This example demonstrates how a plugin might interfere with gmt_offset being set.

  1. Install and activate aryo-activity-log
  2. Navigate to /wp-admin/options-general.php and change Timezone to UTC and Start of Week to Sunday
  3. Encounter the following fatal
PHP Fatal error:  Uncaught TypeError: Unsupported operand types: string * int in /wordpress/core/6.4.3/wp-includes/functions.php:76
Stack trace:
#0 /srv/htdocs/wp-content/plugins/aryo-activity-log/classes/class-aal-api.php(95): current_time('timestamp')
#1 /srv/htdocs/wp-content/plugins/aryo-activity-log/classes/class-aal-api.php(186): AAL_API->insert(Array)
#2 /srv/htdocs/wp-content/plugins/aryo-activity-log/hooks/class-aal-hook-options.php(105): aal_insert_log(Array)
#3 /srv/htdocs/wp-content/plugins/aryo-activity-log/hooks/class-aal-hook-options.php(96): AAL_Hook_Options->insert_log('start_of_week')
#4 /wordpress/core/6.4.3/wp-includes/class-wp-hook.php(324): AAL_Hook_Options->hooks_updated_option('start_of_week', '1', 0)
#5 /wordpress/core/6.4.3/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters(NULL, Array)
#6 /wordpress/core/6.4.3/wp-includes/plugin.php(517): WP_Hook->do_action(Array)
#7 /wordpress/core/6.4.3/wp-includes/option.php(894): do_action('updated_option', 'start_of_week', '1', 0)
#8 /wordpress/core/6.4.3/wp-admin/options.php(339): update_option('start_of_week', 0)
#9 {main}
  thrown in /wordpress/core/6.4.3/wp-includes/functions.php on line 76
  1. Observe that the gmt_offset option is empty in the database.
  2. Revisit /wp-admin/options-general.php, observe critical error message under Date Format option, lack of Save button. Observe the following fatal.
PHP Fatal error:  Uncaught TypeError: Unsupported operand types: string * int in /wordpress/core/6.4.3/wp-includes/functions.php:76
Stack trace:
#0 /wordpress/core/6.4.3/wp-includes/functions.php(180): current_time('timestamp', false)
#1 /wordpress/core/6.4.3/wp-admin/options-general.php(364): date_i18n('F j, Y')
#2 {main}
  thrown in /wordpress/core/6.4.3/wp-includes/functions.php on line 76

Resolving

To resolve, give the gmt_offset option a value.

E.g., via WP-CLI: wp option update gmt_offset 0

Version 3, edited 9 months ago by verygoode (previous) (next) (diff)

#7 @verygoode
9 months ago

With https://core.trac.wordpress.org/ticket/56358#comment:6, it seems current_time might need to be a bit more robust to avoid fatals when the gmt_offset option is empty.

Otherwise, the user is left with a broken /wp-admin/options-general.php and cannot adjust the Date Format option and can't save the page.

Last edited 9 months ago by verygoode (previous) (diff)

#8 @verygoode
9 months ago

  • Focuses php-compatibility added

#9 @verygoode
9 months ago

Similar issue with patch discussion, https://core.trac.wordpress.org/ticket/58986

And another similar report, https://core.trac.wordpress.org/ticket/60629

Last edited 9 months ago by verygoode (previous) (diff)

#10 @hellofromTonya
5 months ago

  • Keywords php80 added; php8 removed
  • Milestone set to 6.7
  • Owner set to hellofromTonya
  • Status changed from reopened to assigned

Related: #57035, #58986, #60629.

Each of these tickets is dealing with the same issue, which I summarized in [57035#comment:39]:

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 these tickets.

This ticket was mentioned in PR #7233 on WordPress/wordpress-develop by @peterwilsoncc.


4 months ago
#11

  • Keywords has-patch added

Follow up to r58923 / https://github.com/WordPress/wordpress-develop/commit/6e7b1243f30af7b85d7dff44df4c86d852dec7df

In multiple locations throughout the code base the gmt_offset value is used in it's original form, this can lead to type errors.

Core-56358
Core-58986
Core-60629

I suspect a helper function may be the solution or adding some type casting to the option_gmt_offset filter.

cc @rarst as I can ping you with a review request in the UI.

#12 @peterwilsoncc
4 months ago

I've created a PR for this and the related tickets to cast the gmt offset throughout the code base. I'm happy to hear suggestions for alternative approaches too.

@Rarst commented on PR #7233:


4 months ago
#13

@Rarst as I can not ping you with a review request in the UI.

That would be because I am set to unavailable, due to being busy with war. 😬

To my memory most of nuance with gmt_offset type is when localized WP installs initiate it to string or something.

I would rather be refactoring it the heck out of usage, it's double-old API with opaque filter overrides and we don't _want_ it used at all.

@peterwilsoncc commented on PR #7233:


3 months ago
#14

I would rather be refactoring it the heck out of usage, it's double-old API with opaque filter overrides and we don't _want_ it used at all.

I've been thinking about this. I think it's a good enhancement to move in this direction but for the purpose of the PHP 8 compatibility would rather take a KISS approach and fix the code with the light touch.

That will allow for a more considered approach for any refactoring rather than doing it in a half baked manner.

@hellofromTonya commented on PR #7233:


3 months ago
#15

Sorry for my delay in responding. The changes fully align to the changes made in r . They explicitly type cast to the needed data type before performing a math operation. Can also see it in action across PHP versions https://3v4l.org/rW2Ob#veol.

I suspect a helper function may be the solution

Repeating code is not optimal. But given the goal @Rarst shared is to "refactoring it the heck out of usage", introducing a helper function now means deprecating it later.

or adding some type casting to the option_gmt_offset filter.

What if the consuming code expects and uses it as a string and is not doing a math operation using it? Such a change would need exploration.

Given the bigger picture goal of getting rid of it, baby steps, i.e. tiny one-step-at-a-time, to resolve the immediate issues.

@hellofromTonya commented on PR #7233:


3 months ago
#16

@peterwilsoncc how's the test coverage for the affected functions?

@peterwilsoncc commented on PR #7233:


3 months ago
#17

how's the test coverage for the affected functions?

@hellofromtonya The REST API's post controller is well tested and includes the specific case of shimming the post modified date in WP_Test_REST_Posts_Controller::test_create_post_draft().

The others not so much:

This ticket was mentioned in PR #7233 on WordPress/wordpress-develop by @peterwilsoncc.


3 months ago
#18

Follow up to r58923 / https://github.com/WordPress/wordpress-develop/commit/6e7b1243f30af7b85d7dff44df4c86d852dec7df

In multiple locations throughout the code base the gmt_offset value is used in it's original form, this can lead to type errors.

Core-56358
Core-58986
Core-60629

I suspect a helper function may be the solution or adding some type casting to the option_gmt_offset filter.

cc @rarst as I can not ping you with a review request in the UI.

This ticket was mentioned in PR #7233 on WordPress/wordpress-develop by @peterwilsoncc.


3 months ago
#19

Follow up to r58923 / https://github.com/WordPress/wordpress-develop/commit/6e7b1243f30af7b85d7dff44df4c86d852dec7df

In multiple locations throughout the code base the gmt_offset value is used in it's original form, this can lead to type errors.

Core-56358
Core-58986
Core-60629

I suspect a helper function may be the solution or adding some type casting to the option_gmt_offset filter.

cc @rarst as I can not ping you with a review request in the UI.

@Rarst commented on PR #7233:


3 months ago
#20

I skipped over helper function possibility. Don't think it's that involved of operation, I was thinking something like wp_timezone()->getOffset(timezone_open('UTC')). Just talking from my head, hadn't worked through that one.

#21 @hellofromTonya
3 months ago

  • Keywords commit added
  • Status changed from assigned to reviewing

PR 7233 is ready for commit. Spoke with @peterwilsoncc who will do the commit.

Copying my approval in the PR:

[As I previously noted](https://github.com/WordPress/wordpress-develop/pull/7233#issuecomment-2327431927), this baby step resolves the immediate issue and uses the existing pattern committed in https://core.trac.wordpress.org/changeset/58923.

Ideally these changes would have full test coverage. That said, in talking with @peterwilsoncc, he shared:

The tests are there for current features and the upgrade tests should catch anything in the ugrade routine

Getting this committed early enough gives a longer soak time to hopefully uncover any issues.

I think this is ready enough for commit ✅

This ticket was mentioned in PR #7233 on WordPress/wordpress-develop by @peterwilsoncc.


3 months ago
#22

Follow up to r58923 / https://github.com/WordPress/wordpress-develop/commit/6e7b1243f30af7b85d7dff44df4c86d852dec7df

In multiple locations throughout the code base the gmt_offset value is used in it's original form, this can lead to type errors.

Core-56358
Core-58986
Core-60629

I suspect a helper function may be the solution or adding some type casting to the option_gmt_offset filter.

cc @rarst as I can not ping you with a review request in the UI.

#23 @peterwilsoncc
3 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 59064:

Date/Time, PHP Compat: Prevent type errors using GMT offset option.

Prevents a potential type errors when making use of the gmt_offset option by casting the value 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 [58923].

Props chaion07, hellofromtonya, kirasong, mhshohel, mukesh27, nicolefurlan, nihar007, nurielmeni, oglekler, peterwilsoncc, prionkor, rajinsharwar, rarst, rleeson, sabernhardt, SergeyBiryukov, swissspidy, toastercookie, verygoode.
Fixes #56358, #58986, #60629.

This ticket was mentioned in PR #7233 on WordPress/wordpress-develop by @peterwilsoncc.


3 months ago
#25

Follow up to r58923 / https://github.com/WordPress/wordpress-develop/commit/6e7b1243f30af7b85d7dff44df4c86d852dec7df

In multiple locations throughout the code base the gmt_offset value is used in it's original form, this can lead to type errors.

Core-56358
Core-58986
Core-60629

I suspect a helper function may be the solution or adding some type casting to the option_gmt_offset filter.

cc @rarst as I can not ping you with a review request in the UI.

#26 @hellofromTonya
3 months ago

#62078 was marked as a duplicate.

Note: See TracTickets for help on using tickets.