Page MenuHomePhorge

Validate some user provided calendar query range dates
ClosedPublic

Authored by aklapper on Sep 18 2024, 13:01.

Details

Summary

Calendar search form allows users to define date ranges. Entering gibberish data leads to a cryptic exception due to calling format() on null, as AphrontFormDateControlValue::getDateTime() can return null instead of a DateTime object.

Also add some additional PhpDoc as a result of playing with this code.

Note that other calendar query forms are more lenient and still accepts gibberish after applying this patch. The intention behind this patch is replacing a cryptic exception with a more appropriate and descriptive error; this patch does not attempt to introduce validation everywhere.

EXCEPTION: (Error) Call to a member function format() on null at [<phorge>/src/applications/calendar/query/PhabricatorCalendarEventSearchEngine.php:469]

Closes T15943

Test Plan

Diff Detail

Repository
rP Phorge
Branch
calVerifyUserDates
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1630
Build 1630: arc lint + arc unit

Event Timeline

aklapper retitled this revision from Validate user provided calendar query range dates to Validate some user provided calendar query range dates.

Nice test plan thanks

src/applications/calendar/query/PhabricatorCalendarEventSearchEngine.php
579–585

Maybe better to throw just 1 exception (Exception - or anything already available in Phorge but I don't know what - or a new Exception PhabricatorMalformedDateStringException or anything similar; but I mean just 1 exception) so to do not change behavior based on the current version of PHP. So future callers can avoid esoteric try-catch, to catch this specific exception in a reliable way. But also to simplify the code.

src/applications/calendar/query/PhabricatorCalendarEventSearchEngine.php
579–585

Hmm but is there any realistic "future caller" who actually wants to catch this?
I'm not keen on adding another TODO here about "Replace this generic Exception with DateMalformedStringException once we require PHP 8.3.0 or later but I can change this to a generic Exception if that gets this closer to getting merged :P

src/applications/calendar/query/PhabricatorCalendarEventSearchEngine.php
579–585

Yeah just Exception is fine in my opinion 👍 It's everywhere in Phorge. Thanks - sorry for late comment

Ignore PHP 8.3.0's new DateMalformedStringException and stick to generic Exception in all cases instead

Also update the PhpDoc accordingly

This revision is now accepted and ready to land.Tue, Nov 26, 14:03