Page MenuHomePhorge
Differential D25875

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

Authored by aklapper on Feb 13 2025, 20:06.
Tags
None
Referenced Files
F4083739: D25875.1746892360.diff
Fri, May 9, 15:52
F4011877: D25875.1746626820.diff
Tue, May 6, 14:07
F3852716: D25875.1746130783.diff
Wed, Apr 30, 20:19
F3675932: D25875.1745532137.diff
Wed, Apr 23, 22:02
F3675818: D25875.1745524801.diff
Wed, Apr 23, 20:00
F3675298: D25875.1745506091.diff
Wed, Apr 23, 14:48
F3674268: D25875.1745493285.diff
Wed, Apr 23, 11:14
F3673751: D25875.1745479473.diff
Wed, Apr 23, 07:24
View All Files
Subscribers
Cigaryno
Matthew
slip
tobiaswiese
valerio.bozzolan

Details

Reviewers
slip
valerio.bozzolan
Group Reviewers
O1: Blessed Committers
Maniphest Tasks
T15994: PHP 8.1 ctype_digit/preg_split exceptions in AphrontFormDateControlValue for no date/time
Commits
rP6ea09aa82c8d: PHP 8.1: Do not pass null to ctype_digit() in AphrontFormDateControlValue…
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 1698
Build 1698: arc lint + arc unit

Event Timeline

aklapper created this revision.Feb 13 2025, 20:06
Owners added a reviewer: O1: Blessed Committers.Feb 13 2025, 20:06
Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptFeb 13 2025, 20:06
aklapper requested review of this revision.Feb 13 2025, 20:06
Harbormaster completed remote builds in B1698: Diff 2637.Feb 13 2025, 20:06
slip accepted this revision.Feb 13 2025, 23:45
slip subscribed.

LGTM

valerio.bozzolan added a comment.Feb 14 2025, 08:41

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
332

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)

aklapper marked an inline comment as done.Feb 14 2025, 13:54
aklapper updated this revision to Diff 2639.Feb 14 2025, 13:54

Short-circuit null check, as rightfully proposed by Valerio

Harbormaster completed remote builds in B1700: Diff 2639.Feb 14 2025, 13:54
aklapper updated this revision to Diff 2640.Feb 14 2025, 13:57

Errm yes, now for real

Harbormaster completed remote builds in B1701: Diff 2640.Feb 14 2025, 13:57
valerio.bozzolan added a comment.Feb 14 2025, 13:57

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

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

aklapper updated this revision to Diff 2642.Feb 14 2025, 15:41

null as fast as possible

Harbormaster completed remote builds in B1702: Diff 2642.Feb 14 2025, 15:41
valerio.bozzolan accepted this revision.Feb 14 2025, 15:43

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
aklapper edited the test plan for this revision. (Show Details)Feb 15 2025, 18:38
Closed by commit rP6ea09aa82c8d: PHP 8.1: Do not pass null to ctype_digit() in AphrontFormDateControlValue…. · Explain WhyFeb 15 2025, 18:39
This revision was automatically updated to reflect the committed changes.
aklapper added a commit: rP6ea09aa82c8d: PHP 8.1: Do not pass null to ctype_digit() in AphrontFormDateControlValue….

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)Packages
src/
view/
form/
control/
15 lines1%0%O1: Blessed Committers

Diff 2637

View Options

src/view/form/control/AphrontFormDateControlValue.php

Show First 20 Lines • Show All 323 Lines • ▼ Show 20 Lines private function getStandardDateFormat($date) {
if (isset($colloquial[$normalized])) { if (isset($colloquial[$normalized])) {
return $colloquial[$normalized]; return $colloquial[$normalized];
} }
// If this looks like an epoch timestamp, prefix it with "@" so that // If this looks like an epoch timestamp, prefix it with "@" so that
// DateTime() reads it as one. Assume small numbers are a "Ymd" digit // DateTime() reads it as one. Assume small numbers are a "Ymd" digit
// string instead of an epoch timestamp for a time in 1970. // string instead of an epoch timestamp for a time in 1970.
if ($date) {
valerio.bozzolanUnsubmitted
Done
Inline Actions

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)

valerio.bozzolan: Thanks! P.S. I see that Phorge also usually do a short-circuit ```lang=php if(!$date) {…
if (ctype_digit($date) && ($date > 30000000)) { if (ctype_digit($date) && ($date > 30000000)) {
$date = '@'.$date; $date = '@'.$date;
} }
$separator = $this->getFormatSeparator(); $separator = $this->getFormatSeparator();
$parts = preg_split('@[,./:-]@', $date); $parts = preg_split('@[,./:-]@', $date);
return implode($separator, $parts); return implode($separator, $parts);
} }
return null;
}
private function getStandardTimeFormat($time) { private function getStandardTimeFormat($time) {
$colloquial = array( $colloquial = array(
'crack of dawn' => '5:00 AM', 'crack of dawn' => '5:00 AM',
'dawn' => '6:00 AM', 'dawn' => '6:00 AM',
'early' => '7:00 AM', 'early' => '7:00 AM',
'morning' => '8:00 AM', 'morning' => '8:00 AM',
'elevenses' => '11:00 AM', 'elevenses' => '11:00 AM',
Show All 21 Lines
Content licensed under Creative Commons Attribution-ShareAlike 4.0 (CC-BY-SA) unless otherwise noted; code licensed under Apache 2.0 or other open source licenses. · CC BY-SA 4.0 · Apache 2.0