Make WordPress Core

Opened 21 months ago

Closed 5 weeks ago

#57030 closed defect (bug) (fixed)

Condition is not strictly checked on options-general.php file

Reported by: rakibwordpress's profile rakibwordpress Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.6 Priority: normal
Severity: normal Version: 3.0
Component: Administration Keywords: has-patch changes-requested has-unit-tests close
Focuses: coding-standards Cc:

Description

In options-general.php file it is 0 is being compared with another variable which is $current_offset. As 0 is numeric value this needs to be compared.

Change History (10)

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


21 months ago
#1

Trac ticket:

#2 follow-up: @hellofromTonya
17 months ago

  • Version changed from trunk to 3.0

Hello @rakibwordpress,

Welcome back to WordPress Core's Trac!

I'm doing some triage for open tickets against trunk to see if the defect was introduced in the 6.2.0 cycle. For this one, the code is from 3.0.0, introduced via [12507] / #11558. Changing the Version number.

Also a note for consideration:
Will $current_offset always be an integer 0? Or could it be a string '0'? The current code will type juggle to compare the values without comparing the data types.

What I'm wondering (haven't looked deeply into it):
Is there a strict requirement that 0 must be an integer data type?

If no, then === could cause a backwards-compatibility issue or break in the code when it's a string 0.

#3 @SergeyBiryukov
17 months ago

  • Component changed from General to Administration
  • Focuses coding-standards added; administration removed
  • Milestone changed from Awaiting Review to 6.3
  • Owner set to SergeyBiryukov
  • Status changed from new to assigned

#4 in reply to: ↑ 2 ; follow-up: @SergeyBiryukov
17 months ago

Replying to hellofromTonya:

Also a note for consideration:
Will $current_offset always be an integer 0? Or could it be a string '0'? The current code will type juggle to compare the values without comparing the data types.

This gets even more interesting, as the value can also be float or false, via wp_timezone_override_offset() hooked to pre_option_gmt_offset in wp-includes/default-filters.php.

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


13 months ago

#6 @oglekler
13 months ago

  • Keywords changes-requested needs-unit-tests added

This ticket was discussed in the recent bug scrub and due to lack of activity it was decided to move it into future release.

This ticket needs some additional work and unit test as well. It can be moved to the next available milestone when it will be ready.

Props: @Clorith @costdev

Last edited 13 months ago by oglekler (previous) (diff)

#7 @costdev
13 months ago

  • Milestone changed from 6.3 to Future Release

Moving to Future Release per the above comment.

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


5 weeks ago
#8

  • Keywords has-unit-tests added; needs-unit-tests removed

Trac ticket: https://core.trac.wordpress.org/ticket/57030

## Description

Fixes a strict comparison issue in options-general.php where the $current_offset variable was not being strictly compared as an integer. This issue could lead to unexpected behavior when comparing the gmt_offset option, especially when it could be interpreted as different types such as string or float.

### Changes Made

  • Adjusted the comparison of $current_offset to ensure it is strictly compared as an integer 0.
  • Updated logic to handle cases where gmt_offset may vary in type (integer, string, or float).
  • Introduced unit tests to validate the behavior across different gmt_offset scenarios.

### Added Unit Tests

  • Created gmtOffset.php file to house unit tests specifically targeting the gmt_offset behavior.
  • Covered scenarios such as integer 0, string '0', negative float values, and invalid string inputs for gmt_offset.

#9 in reply to: ↑ 4 ; follow-up: @rajinsharwar
5 weeks ago

  • Keywords close added

I guess this was solved via #60700 as the strict checks have already been implemented. Maybe we can close this.

Replying to SergeyBiryukov:

This gets even more interesting, as the value can also be float or false, via wp_timezone_override_offset() hooked to pre_option_gmt_offset in wp-includes/default-filters.php.

I guess when the value is false, it currently works fine with the logic of $tzstring = 'UTC+' . $current_offset. And, I do not think the value will ever be float, given that the code is wrapped with the check of empty( $tzstring ).

In wp_timezone_override_offset() it states that it "Overrides the gmt_offset option if we have a timezone_string available". So, if the timezone is not available, I do not think a float will be returned.

Marking this as a candidate for close.

#10 in reply to: ↑ 9 @SergeyBiryukov
5 weeks ago

  • Milestone changed from Future Release to 6.6
  • Resolution set to fixed
  • Status changed from assigned to closed

Replying to rajinsharwar:

I guess this was solved via #60700 as the strict checks have already been implemented. Maybe we can close this.

Good catch, thanks! This is now resolved in [57833] / #60700. I forgot about this ticket, sorry for that.

Replying to SergeyBiryukov:

This gets even more interesting, as the value can also be float or false, via wp_timezone_override_offset() hooked to pre_option_gmt_offset in wp-includes/default-filters.php.

I guess when the value is false, it currently works fine with the logic of $tzstring = 'UTC+' . $current_offset. And, I do not think the value will ever be float, given that the code is wrapped with the check of empty( $tzstring ).

You're right. In my testing, $current_offset is actually a string here, since wp_timezone_override_offset() returning false in case of an empty timezone string means that get_option() proceeds to retrieve the value from the database. Now that the value is cast to an integer, the strict comparison works as expected.

Note: See TracTickets for help on using tickets.