Validate some user provided calendar query range dates
ClosedPublic
Actions

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

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15943: Calendar: Call to a member function format() on null at $this->getQueryDateFrom($query)->getDateTime()

Commits

rP89be7a51d8c8: Validate some user provided calendar query range dates

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

On http://phorge.localhost/calendar/query/month/, click "Edit Query", check "Occurs After", replace default date value with "abcde", click "Search" button
On http://phorge.localhost/calendar/query/month/, click "Edit Query", check "Occurs After", replace default time value with "abcde", click "Search" button
On http://phorge.localhost/calendar/query/month/, click "Edit Query", check "Occurs Before", replace default date value with "abcde", click "Search" button
On http://phorge.localhost/calendar/query/month/, click "Edit Query", check "Occurs Before", replace default time value with "abcde", click "Search" button
On http://phorge.localhost/calendar/query/day/, click "Edit Query", check "Occurs After", replace default date value with "abcde", click "Search" button
On http://phorge.localhost/calendar/query/day/, click "Edit Query", check "Occurs After", replace default time value with "abcde", click "Search" button
On http://phorge.localhost/calendar/query/day/, click "Edit Query", check "Occurs Before", replace default date value with "abcde", click "Search" button
On http://phorge.localhost/calendar/query/day/, click "Edit Query", check "Occurs Before", replace default time value with "abcde", click "Search" button

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

aklapper created this revision.Sep 18 2024, 13:01

Owners added a reviewer: O1: Blessed Committers.Sep 18 2024, 13:01

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptSep 18 2024, 13:01

aklapper requested review of this revision.Sep 18 2024, 13:01

Harbormaster completed remote builds in B1598: Diff 2482.Sep 18 2024, 13:01

aklapper edited the summary of this revision. (Show Details)Sep 18 2024, 13:20

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.

aklapper added inline comments.Oct 1 2024, 15:19

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