Fix PHP 8.1 "ltrim(null)" exception which blocks rendering five applications' Configure pages
ClosedPublic
Actions

Authored by aklapper on May 6 2023, 13:22.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15359: PHP 8.1 "ltrim(null)" exception blocks rendering five applications' Configure pages

Commits

rP3819f7ff196e: Fix PHP 8.1 "ltrim(null)" exception which blocks rendering five applications'…

Summary

Since PHP 8.1, passing a null string to ltrim(string $string, string $characters) is deprecated.

Thus do not check for $path = null but check for $path = '' before passing $path as the $string parameter to ltrim(), like src/applications/settings/panel/PhabricatorSettingsPanel.php already does.

Closes T15359

Test Plan

Applied this change (on top of D25197) and five applications' Configure pages (Differential, Maniphest, Files, Paste, and Ponder) finally rendered in web browser.

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

aklapper created this revision.May 6 2023, 13:22

Owners added a reviewer: O1: Blessed Committers.May 6 2023, 13:22

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptMay 6 2023, 13:22

aklapper requested review of this revision.May 6 2023, 13:22

Harbormaster completed remote builds in B370: Diff 674.May 6 2023, 13:22

Thanks for this patch (again)

The approach seems 100% legit to me.

Just to be sure I tested this against each damn application:

Screenshot_2023_05_08_133448.png (98×1 px, 20 KB)

Aaaand, no implosions. For all the cases I monitored with phlog() the $path and the general assumption is correct.

Green light

sgtm

src/applications/meta/panel/PhabricatorApplicationConfigurationPanel.php
28	✅ It seems that most PHPDoc stuff in Phorge ends with the dot, also in the first line.
30	✅ It seems "string?" is an usual way in Phorge to mark an optional parameter like in `./files/PhabricatorImageTransformer.php` - I was honestly not aware of that, since PHPDoc does not have that formally.
33	I verified that all the code that extends this class does not rely on the old `null` default. This is 100% legit since the argument goes directly into an `ltrim()` and so a NULL really should not arrive.