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
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
F2985347: D25875.1740033440.diff
Wed, Feb 19, 06:37
F2984281: D25875.1740005904.diff
Tue, Feb 18, 22:58
F2982827: D25875.1739971945.diff
Tue, Feb 18, 13:32
F2982749: D25875.1739971764.diff
Tue, Feb 18, 13:29
F2979364: D25875.1739844511.diff
Mon, Feb 17, 02: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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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
337

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