Page MenuHomePhorge

PHP 8.1: Do not pass null to ctype_digit() in AphrontFormDateControlValue::getStandardDateFormat()
ClosedPublic

Authored by aklapper on Thu, Feb 13, 20:06.
Tags
None
Referenced Files
F2992527: D25875.1740227108.diff
Fri, Feb 21, 12:25
F2991640: D25875.1740196444.diff
Fri, Feb 21, 03:54
F2988186: D25875.1740128329.diff
Thu, Feb 20, 08:58
F2987330: D25875.1740113824.diff
Thu, Feb 20, 04:57
F2987088: D25875.1740106537.diff
Thu, Feb 20, 02:55
F2986202: D25875.1740097045.diff
Thu, Feb 20, 00:17
F2985500: D25875.1740058779.diff
Wed, Feb 19, 13:39
F2985385: D25875.1740042523.diff
Wed, Feb 19, 09:08

Details

Summary

PHP 8.1 warns that ctype_digit(): Argument of type null will be interpreted as string in the future. Thus do not pass a null value to ctype_digit() in AphrontFormDateControlValue but check first that the value is not null.

ERROR 8192: ctype_digit(): Argument of type null will be interpreted as string in the future at [/var/www/html/phorge/phorge/src/view/form/control/AphrontFormDateControlValue.php:332]

(It does not matter if we return null or return '' as the new fallback of getStandardDateFormat() - Phorge seems to handle both in this case.)

Closes T15994

Test Plan
  1. Go to http://phorge.localhost/T1, select "Start Tracking Time", remove both values, click the "Start Timer" button
  2. Go to http://phorge.localhost/phrequent/ and see that the task is listed
  3. Go to http://phorge.localhost/T1, select "Stop Tracking Time", remove both values, click the "Stop Timer" button
  4. Go to http://phorge.localhost/phrequent/ and see that the task is not listed
  5. Use a non-standard task creation form via http://phorge.localhost/maniphest/task/edit/form/5/ which has custom date fields visible and editable defined via http://phorge.localhost/config/edit/maniphest.custom-field-definitions/, enable the date fields, and remove their values, and file the task. Same behavior as on git master without the patch: These custom date values get set on the newly created task (which is not what I'd expect but it's not a regression created by this patch).

Diff Detail

Repository
rP Phorge
Branch
T15994 (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1700
Build 1700: arc lint + arc unit

Event Timeline

Thanks. I'm curious if there's anything up that should not call this with null. Also I'm curious to test with a couple of date filters and custom date fields.

src/view/form/control/AphrontFormDateControlValue.php
336

Thanks! P.S. I see that Phorge also usually do a short-circuit

if(!$date) {
  return null;
}

Maybe useful also for less indentations, easier git blame

(not a blocking comment - just tip)

Short-circuit null check, as rightfully proposed by Valerio

(Maybe also moving in line 316, before phutil_utf8_strtolower() and the regexes)

(LOOOOL sorry my brain generates tips at very slow rate)

You're fast. lol I approve because life is too short.

This revision is now accepted and ready to land.Fri, Feb 14, 15:43