Page MenuHomePhorge

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

Authored by aklapper on Feb 13 2025, 20:06.
Tags
None
Referenced Files
F3290204: D25875.1742861839.diff
Mon, Mar 24, 00:17
F3289696: D25875.1742852209.diff
Sun, Mar 23, 21:36
F3288590: D25875.1742832445.diff
Sun, Mar 23, 16:07
F3287632: D25875.1742821388.diff
Sun, Mar 23, 13:03
F3285623: D25875.1742805292.diff
Sun, Mar 23, 08:34
F3285180: D25875.1742794830.diff
Sun, Mar 23, 05:40
F3285058: D25875.1742792403.diff
Sun, Mar 23, 05:00
F3283167: D25875.1742741057.diff
Sat, Mar 22, 14:44

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 1701
Build 1701: 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.Feb 14 2025, 15:43