Page MenuHomePhabricator
Create Task
Maniphest T320296

Improve handling of invalid offsets in UserTimeCorrection
Closed, ResolvedPublic
Actions

Description

As noted in T318455#8301251, UserTimeCorrection::parse silently clamps offsets between -12 and +14, without saying anything to its callers (and thus, to users). This should be changed so that invalid values can be handled as appropriate, e.g. by informing the user that they entered an invalid value.

Details

SubjectRepoBranchLines +/-
Move validation of timezone to HTMLTimezoneFieldmediawiki/coremaster+16 -7
preferences: Show errors about invalid timezone instead of silently correctingmediawiki/coremaster+9 -0
Customize query in gerrit

Related Objects

Event Timeline

Daimona created this task.Oct 7 2022, 10:43 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 7 2022, 10:43 PM
Daimona mentioned this in T318455: "time offset from UTC" timezone option in Special:Preferences doesn't work with negative offsets with minutes.Oct 7 2022, 10:44 PM
matmarex claimed this task.Oct 8 2022, 3:58 PM
matmarex subscribed.

This annoys me, I hope you don't mind reviewing a patch.

Daimona added a comment.Oct 8 2022, 3:59 PM

Sure, it annoys me as well, so I'll definitely review it. We would also benefit from proper error handling in CampaignEvents.

gerritbot added a comment.Oct 8 2022, 4:01 PM

Change 840309 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] preferences: Show errors about invalid timezone instead of silently correcting

https://gerrit.wikimedia.org/r/840309

gerritbot added a project: Patch-For-Review.Oct 8 2022, 4:01 PM
gerritbot added a comment.Oct 9 2022, 12:59 PM

Change 840309 merged by jenkins-bot:

[mediawiki/core@master] preferences: Show errors about invalid timezone instead of silently correcting

https://gerrit.wikimedia.org/r/840309

ReleaseTaggerBot added a project: MW-1.40-notes (1.40.0-wmf.5; 2022-10-10).Oct 9 2022, 1:00 PM
Maintenance_bot removed a project: Patch-For-Review.Oct 9 2022, 1:29 PM
matmarex closed this task as Resolved.Oct 9 2022, 6:13 PM
Daimona mentioned this in T320359: Reject registration creation/edit if the timezone offset is < -12 or > 14.Oct 9 2022, 9:16 PM
Func reopened this task as Open.Jan 18 2023, 11:08 AM
Func subscribed.
In T320296#8302256, @Daimona wrote:

We would also benefit from proper error handling in CampaignEvents.

Then I think the validation should happen in HTMLTimezoneField::validate() instead of validation-callback for user preferences only.

Commit d716fee for CampaignEvents extension added more validations, do we need that in core?

Daimona added a comment.Jan 19 2023, 1:39 PM
In T320296#8534223, @Func wrote:
In T320296#8302256, @Daimona wrote:

We would also benefit from proper error handling in CampaignEvents.

Then I think the validation should happen in HTMLTimezoneField::validate() instead of validation-callback for user preferences only.

Sounds reasonable.

Commit d716fee for CampaignEvents extension added more validations, do we need that in core?

I would say yes

gerritbot added a comment.Jan 19 2023, 2:26 PM

Change 881381 had a related patch set uploaded (by Func; author: Func):

[mediawiki/core@master] Move validation of timezone to HTMLTimezoneField

https://gerrit.wikimedia.org/r/881381

gerritbot added a project: Patch-For-Review.Jan 19 2023, 2:26 PM
Func added a comment.EditedJan 19 2023, 2:29 PM
In T320296#8540271, @Daimona wrote:

Commit d716fee for CampaignEvents extension added more validations, do we need that in core?

I would say yes

I tested the given cases manually, and all of them can be identified as invalid correctly by ( new UserTimeCorrection( $value ) )->isValid(), so I didn't include extra validations.

Daimona added a comment.Jan 19 2023, 6:21 PM
In T320296#8540407, @Func wrote:
In T320296#8540271, @Daimona wrote:

Commit d716fee for CampaignEvents extension added more validations, do we need that in core?

I would say yes

I tested the given cases manually, and all of them can be identified as invalid correctly by ( new UserTimeCorrection( $value ) )->isValid(), so I didn't include extra validations.

Right, because the code in CampaignEvents uses PHP's DateTimeZone directly. I think we *may* have to reconsider this as part of T322352, but that's for another day.

gerritbot added a comment.Jan 20 2023, 6:28 AM

Change 881381 merged by jenkins-bot:

[mediawiki/core@master] Move validation of timezone to HTMLTimezoneField

https://gerrit.wikimedia.org/r/881381

Maintenance_bot removed a project: Patch-For-Review.Jan 20 2023, 6:30 AM
matmarex closed this task as Resolved.Jan 20 2023, 6:37 AM
ReleaseTaggerBot edited projects, added MW-1.40-notes (1.40.0-wmf.20; 2023-01-23); removed MW-1.40-notes (1.40.0-wmf.5; 2022-10-10).Jan 20 2023, 9:01 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL