Fix PHP 8.1 "strlen(null)" and "explode()" exceptions which block rendering Administrator Account Creation page
ClosedPublic
Actions

Authored by aklapper on May 2 2023, 18:55.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15284: Installation: PHP 8.1 "strlen(null)" and "explode" exceptions block rendering Administrator Account Creation page

Commits

rP8a1cfff1e8ab: Fix PHP 8.1 "strlen(null)" and "explode()" exceptions which block rendering…

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.

Similarly, explode(string $separator, string $string, int $limit) does not accept
passing null instead of an actual string as input parameter either anymore.

Closes T15284

Test Plan

Applied these two changes. Afterwards, admin user account was created and Phorge homepage rendered in web browser on a fresh installation.

Diff Detail

Repository

rP Phorge

Branch

arcpatch-D25175

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 404
Build 404: arc lint + arc unit

Event Timeline

aklapper created this revision.May 2 2023, 18:55

Owners added a reviewer: O1: Blessed Committers.May 2 2023, 18:55

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

aklapper requested review of this revision.May 2 2023, 18:55

Harbormaster completed remote builds in B338: Diff 625.May 2 2023, 18:55

I am not sure about the assumption I made about the false return value of isRawCacheDataValid().

Thanks for this patch

To me it seems that your assumption is totally legit

I tested this locally and this does not introduce nuclear implosions

I accept as behalf of myself and not as O1 since I shared some strong positive facts but Phorge probably deserves a stronger opinion

src/applications/auth/constants/PhabricatorCookies.php
167	✅ I logged this variable with `phlog()` to verify that `$cookie` just assumes values like null or strings like `"1683107655,/"` etc. The function `phutil_nonempty_string()` will report any alien type, and that is OK.
src/applications/people/cache/PhabricatorUserProfileImageCacheType.php
94–96	Before PHP 8.1 this was happening, when `$data` was null: $parts = explode(',', null, 2); // array('') $version = reset($parts); // string(0) "" return ($version === $this->getCacheVersion($user)); // false Also note that `$this->getCacheVersion($user)` seems that it never returns an empty string so this is really always false when `$data` is null
99–107	Internal note: this method never returns an empty string or null

valerio.bozzolan retitled this revision from Fix PHP 8.1 "strlen(null)" and "explode" exceptions which block rendering Administrator Account Creation page to Fix PHP 8.1 "strlen(null)" and "explode()" exceptions which block rendering Administrator Account Creation page.May 4 2023, 12:34

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

valerio.bozzolan added inline comments.May 4 2023, 13:08

src/applications/people/cache/PhabricatorUserProfileImageCacheType.php
99–100	Thanks. Also here maybe better a minimal strict short-circuit check like if ($data === null) { return false; } To avoid type juggling with string `"0"` etc.