Fix PHP 8.1 "strlen(null)" exceptions which block rendering page of a File
ClosedPublic
Actions

Authored by aklapper on May 4 2023, 11:41.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15341: PHP 8.1 "strlen(null)" exceptions block rendering page of a File

Commits

rP6c692ddbbba8: Fix PHP 8.1 "strlen(null)" exceptions which block rendering page of a File

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_string() as a replacement.

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

Closes T15341

Test Plan

Applied these four changes and file page at /F91 finally rendered in web browser.

Diff Detail

Repository

rP Phorge

Branch

uploadFile (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 355
Build 355: arc lint + arc unit

Event Timeline

aklapper created this revision.May 4 2023, 11:41

Owners added a reviewer: O1: Blessed Committers.May 4 2023, 11:41

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptMay 4 2023, 11:41

Harbormaster completed remote builds in B355: Diff 652.May 4 2023, 11:41

aklapper requested review of this revision.May 4 2023, 11:41

aklapper mentioned this in T15342: PHP 8.1 "strlen(null)" exception blocks rendering a File page when Alt Text was altered.May 4 2023, 12:00

aklapper mentioned this in D25187: Fix PHP 8.1 "strlen(null)" exception which blocks rendering a File page when Alt Text was altered.

Thanks for this patch, again (!!!)

I tested this patch, locally, trying to do lot of fuzzy tests. With/withno alternative texts, changing encoding types, etc. I was not able to reproduce any nuclear crash.

Green light

sgtm

src/applications/files/controller/PhabricatorFileViewController.php
314	✅ I verified the above line The `$custom_alt` variable is the alternative image text that assumes null as default, or a string. The function `phutil_nonempty_string()` will report alien types, and this is OK here.
src/applications/files/document/render/PhabricatorDocumentRenderingEngine.php
63	✅ I verified the above line The `Aphront#getStr()` only returns strings, or null (default). The function `phutil_nonempty_string()` will report other alien types, and that is OK.
68	✅ I verified the above line The `Aphront#getStr()` only returns strings, or null (default). The function `phutil_nonempty_string()` will report other alien types, and that is OK.
src/applications/files/storage/PhabricatorFile.php
1281	✅ I verified the above line The `$alt` variable is the alternative image text that assumes null as default, or a string. The function `phutil_nonempty_string()` will report alien types, and this is good.