Fix PHP 8.1 "strlen(null)" exception which blocks creating a project with an empty Description field
ClosedPublic
Actions

Authored by aklapper on May 2 2023, 19:20.

Details

Reviewers

valerio.bozzolan
speck

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15331: PHP 8.1 "strlen(null)" exceptions block creating a project with an empty Description field

Commits

rARC97e163187418: Fix PHP 8.1 "strlen(null)" exception which blocks creating a project with an…

Summary

strlen() was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts phutil_nonempty_stringlike() as a replacement for string-alike variables.

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

Closes first half of T15331

Test Plan

Applied these two changes (one in Arcanist, one in Phorge). Project with empty Description field was created and /project/view/projectid/ rendered in web browser.

Diff Detail

Repository

rARC Arcanist

Branch

emptyProjDesc (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 339
Build 339: arc lint + arc unit

Event Timeline

aklapper created this revision.May 2 2023, 19:20

Owners added a reviewer: O1: Blessed Committers.May 2 2023, 19:20

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptMay 2 2023, 19:20

aklapper requested review of this revision.May 2 2023, 19:20

Harbormaster completed remote builds in B339: Diff 626.May 2 2023, 19:20

aklapper edited the summary of this revision. (Show Details)May 2 2023, 19:23

Thanks for this patch.

This needs the usual stringlike() trick to make Phorge happy

Also it probably needs the phlog() trick suggested by avivey to make O1 happy

I would say, let's wait for what happens in D25157, then let's turn back here

I'm sorry for this waiting

src/utils/utils.php
903	The already existing PHPDoc was wrong. The `$corpus` parameter is not necessarily a string but can be a `PhutilSafeHTML` as can be see at line 928
911	Sadly it seems using `phutil_nonempty_string()` in this case can crash since `$corpus` can be an object. We probably need the `phutil_nonempty_stringlike()` trick instead.
928	Interestingly this line highlights that `$corpus` is not always a string

This revision now requires changes to proceed.May 3 2023, 09:38

aklapper added a task: T15331: PHP 8.1 "strlen(null)" exceptions block creating a project with an empty Description field.May 3 2023, 17:03

Use phutil_nonempty_stringlike; update doc comment.

Not sure if some additional phlog code is wanted or needed here - what exactly do we want to find out, apart from string and PhutilSafeHTML being input types?

Harbormaster completed remote builds in B411: Diff 735.May 13 2023, 11:47

valerio.bozzolan mentioned this in Wikimedia Hackaton 2023 Athens Code Sprint.May 18 2023, 10:16

Accepting (but not as O1) since this seems totally legit to me now. Thanks for your update

I don't approve as O1 because I know probably somebody would like to share different opinions.