PHP 8.1: fixes for strlen() not accepting NULL anymore, part 3
AbandonedPublic
Actions

Authored by valerio.bozzolan on Apr 1 2023, 09:55.

Details

Reviewers

Cigaryno

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15064: Make Phorge compatible with PHP 8.1/8.2/8.3
T15190: PHP 8.1: strlen() and other scalar-only functions do not accept NULL anymore - understand fix strategies

Summary

This is a fix for PHP 8.1 deprecation of strlen(NULL), for these Phorge components:

src/view
src/infrastructure (partial, needs more patches)

The strlen() was used in Phabricator to check if a generic value was a non-empty string.
For this reason, Phorge adopts phutil_nonempty_string() that checks that.

Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If your phutil_nonempty_string() throws an exception, just
report it to Phorge to evaluate and fix together that specific corner case.

Ref T15190
Ref T15064

Test Plan

check with big eyes that there are no obvious typos

Diff Detail

Repository

rP Phorge

Branch

arcpatch-D25106

Lint

Lint Passed

Severity	Location	Code	Message
Advice	src/infrastructure/export/field/PhabricatorOptionExportField.php:19	XHP16	TODO Comment

Unit

Tests Passed

Build Status

Buildable 221
Build 221: arc lint + arc unit

Event Timeline

valerio.bozzolan created this revision.Apr 1 2023, 09:55

Owners added a reviewer: O1: Blessed Committers.Apr 1 2023, 09:55

Herald added subscribers: Cigaryno, Matthew, tobiaswiese, speck. · View Herald TranscriptApr 1 2023, 09:55

valerio.bozzolan requested review of this revision.Apr 1 2023, 09:55

Harbormaster completed remote builds in B214: Diff 441.Apr 1 2023, 09:55

As usual, the risk of this change is that exceptions may occur due to
really weird stuff coming in as input. In any case, if it happens,
it must be evaluated on a case-by-case basis, because in this case,
the previously present code (strlen) didn't make sense anyway either.

Harbormaster completed remote builds in B215: Diff 442.Apr 1 2023, 10:10

valerio.bozzolan edited the summary of this revision. (Show Details)Apr 1 2023, 12:21

Cigaryno added a project: PHP 8.2 support.Apr 1 2023, 12:23

valerio.bozzolan retitled this revision from PHP 8.2: fixes for strlen() not accepting NULL anymore, part 3 to PHP 8.1: fixes for strlen() not accepting NULL anymore, part 3.Apr 1 2023, 13:08

valerio.bozzolan edited the summary of this revision. (Show Details)

valerio.bozzolan edited projects, added PHP 8.1 support (deprecated); removed PHP 8.2 support.

valerio.bozzolan edited the summary of this revision. (Show Details)Apr 1 2023, 13:22

Parts 1 and 2 are approved by Blessed Committers.

valerio.bozzolan mentioned this in rP96ae4ba13acb: PHP 8.1: fixes for strlen() not accepting NULL anymore, part 2.Apr 4 2023, 05:48

fix AphrontFileResponse#setDownload() that had weird corner-cases

Harbormaster completed remote builds in B221: Diff 449.Apr 4 2023, 05:53

valerio.bozzolan added inline comments.Apr 4 2023, 05:56

src/aphront/response/AphrontFileResponse.php
22	That above check was introduced by this commit: rP96ae4ba13acb: PHP 8.1: fixes for strlen() not accepting NULL anymore, part 2 But it's too much strict. I have double-checked and this deserves more inline documentation. See on the right. I just added some comments, and a strong default.
47	Also this method ↓ deserved an inline comment to show that it can return mixed stuff.
140	IMPORTANT: this cannot be used as-is, since getDownload() returns mixed stuff by design it seems.

valerio.bozzolan planned changes to this revision.Apr 4 2023, 06:01

valerio.bozzolan abandoned this revision.Apr 7 2023, 10:17

valerio.bozzolan added inline comments.

src/infrastructure/log/PhabricatorSSHLog.php
38	This replacement has sense since everything from `getenv()` should be a string and we should report otherwise.
src/infrastructure/management/PhabricatorManagementWorkflow.php
17	This check has sense since then we use `strtotime($time)` so we really want a string, and we should report alien stuff.
src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php
2452	This check has sense since then we want to count length in bytes, so a string is necessary.
src/infrastructure/ssh/PhabricatorSSHWorkflow.php
104	This check has sense since we want a string and we want to report alien values from `getenv()`.