Make WordPress Core

Opened 9 months ago

Closed 6 months ago

Last modified 6 months ago

#58814 closed defect (bug) (fixed)

Intensionally inset general->timezone_string field wrong format makes error in 'Time Format' & 'Date Format'

Reported by: mrinal013's profile mrinal013 Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.4 Priority: normal
Severity: normal Version:
Component: Options, Meta APIs Keywords: has-screenshots has-patch has-unit-tests needs-testing has-testing-info commit
Focuses: Cc:

Description

How to reproduce

  1. Go to WP Admin/Settings->General.
  2. Open the browser console and find the timezone_string select field's selected option.
  3. Change the value that is not valid. For example, 0/test
  4. Save the settings.
  5. Then I see the error in the Time Format and Date Format fields.

I find this bug in the published version. Even with the trunk branch of wordpress-develop.

https://www.loom.com/share/546fd9ba1f97419a9a65b8e9885c0773?sid=7e7b6625-bab2-491d-b728-2591f015f1df

Change History (19)

#1 @costdev
9 months ago

  • Keywords close 2nd-opinion added; needs-privacy-review removed
  • Severity changed from major to normal
  • Version trunk deleted

Hi @mrinal013, thanks for opening this ticket!

I'm not seeing how these values could be invalid without intentionally modifying them to be. The values are hardcoded with a prefix of UTC and + or -. Anyone using DevTools or JavaScript to modify these values should look at how the existing values are formatted before setting their own.

If WordPress Core provided a filter for these timezone choices and lacked adequate validation, this would be a Core bug. However, this appears to be a warning created through intent, rather than a bug.


  • Removing needs-privacy-review as this doesn't relate to privacy.
  • Adding close and 2nd-opinion to gather additional thoughts.
  • Setting Severity to normal.
  • Removing Version as the initial report states that this can be reproduced in earlier versions.

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


9 months ago
#2

  • Keywords has-patch has-unit-tests added

Previously, timezone values such as an empty string, 0 or an invalid timezone identifier were saved.
This produced a warning in PHP < 7.4, and a Fatal Error in PHP 8+.

This adds validation which resets invalid values to the previous value before saving, and displays an error message to the user.

Includes E2E tests.

#3 @costdev
9 months ago

  • Component changed from Administration to Options, Meta APIs
  • Keywords close 2nd-opinion removed
  • Milestone changed from Awaiting Review to 6.4

After discussing this ticket with @peterwilsoncc, it may be worth improving server side validation here anyway.

PR 4853 resets invalid values to the previous value, and displays an error message to the user. E2E tests are included.


  • Milestoning for WordPress 6.4.
  • Updating Component to Options, Meta APIs.

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


8 months ago

#5 @oglekler
8 months ago

  • Keywords needs-testing added

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


8 months ago

#7 @huzaifaalmesbah
8 months ago

Test Report

Environment

OS: macOS m1
WordPress 6.4-alpha-56267-src
PHP 7.4.33
nginx/1.25.2
MySQL 5.7.43
Browser: Chrome 116.0.5845.140
Theme: Twenty Twenty-Tree
Active Plugins: No plugins activated.

Results

This patch works properly. Before applying the patch, it shows some errors and save successfully. After applying the patch, I am not seeing any errors but showing a warning message. It's good.

Before Applying Patch

https://i.ibb.co/TBgk9GS/before.png

After Applying Patch

https://i.ibb.co/WypzVTR/after.png

#8 @Ankit K Gupta
8 months ago

Test Report ❌

Patch tested: https://github.com/WordPress/wordpress-develop/pull/4853

Testing Environment:

  • WordPress - 6.4-alpha-20230908.205513
  • Web Server: Nginx
  • Chrome Version - 116
  • OS - macOS
  • Theme: Twenty Twenty-Three
  • PHP - 7.4.0

Test result:

The patch functions correctly when an invalid timezone is entered, such as a number (0), special characters ($#%@), or alphabetic characters (e.g., "Five:Thirty").
However, when a valid timezone is manually inputted, it results in errors. This needs attention and rectification.

Steps to Reproduce:

  1. Choose a timezone from the dropdown options, e.g., "UTC+0."
  2. Save the selected timezone setting.
  3. Inspect the element and select the value that was chosen, e.g.,
    <option selected="selected" value="UTC+0">UTC+0</option>.
    
  1. Modify the value to
    <option selected="selected" value="0">0</option>.
    
  1. Save the setting. Observe that an error message appears in the header as expected.
  2. Choose the selected value again and input a valid timezone format like 'UTC+8:30' e.g.,
    <option selected="selected" value="UTC+8:30">UTC+8:30</option>.
    
  1. Save the setting again.
  2. Notice the PHP warnings displayed.

Issue Demonstration Video: https://www.loom.com/share/9c4f5c7db3874bc08fa4629f64c99ca5

Screenshot:

When using Valid Timezone:
https://i.imgur.com/DR8mU7Z.jpg

When adding Invalid TimeZone

https://i.imgur.com/vFUmNUL.jpg

#9 @oglekler
7 months ago

I was unable to reproduce the issue from the last report 🤔 For me, the patch is working as expected.

#10 @Ankit K Gupta
7 months ago

Retested the patch again and still able to replicate the issue mentioned in the comment https://core.trac.wordpress.org/ticket/58814#comment:8

https://i.imgur.com/pdWnJ7K.jpg

Last edited 7 months ago by Ankit K Gupta (previous) (diff)

#11 @nicolefurlan
7 months ago

@costdev would you mind taking a look at the failing test report here?

#12 @costdev
7 months ago

@nicolefurlan Sure! The failing test is the E2E Test timeout failure we've been having trouble with in Core for a while now. The failure isn't caused by the code in the PR. 🙂

#13 @nicolefurlan
7 months ago

Wonderful! With the other two passing test reports, do we think this fix is ready for commit?

#14 @nicolefurlan
6 months ago

  • Keywords has-testing-info added

#15 @nicolefurlan
6 months ago

  • Keywords commit added

I've added commit but of course, if that doesn't seem right, please feel free to remove it :) (cc @costdev)

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


6 months ago

#17 @nicolefurlan
6 months ago

This ticket was discussed in today's bug scrub.

@costdev agrees that this is ready for commit.

I addressed Peter's feedback and was going to wait for him to approve it during his day as a courtesy.

cc @peterwilsoncc :)

#18 @peterwilsoncc
6 months ago

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

In 56949:

Options, Meta APIs: Prevent saving of invalid timezones.

Prevent the saving of invalid timezone string in to the database on the options pages. If an invalid timezone is submitted it is ignored and the setting remains unchanged.

This prevents a warning or fatal (depending on the PHP version) from being thrown by an invalid timezone setting on the Settings > General page.

Props ankit-k-gupta, costdev, huzaifaalmesbah, mrinal013, nicolefurlan, oglekler.
Fixes #58814.

Note: See TracTickets for help on using tickets.