Fix numerous PHP 8.1 "strlen(null)" exceptions preventing homepage to display
ClosedPublic
Actions

Authored by aklapper on Apr 22 2023, 13:17.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15264: Numerous PHP 8.1 "strlen(null)" exceptions preventing homepage to display after installation

Commits

rPb56d86e48d0f: Fix numerous PHP 8.1 "strlen(null)" exceptions preventing homepage to display

Summary

Fix numerous PHP 8.1 RuntimeExceptions caused by the deprecation of strlen(null).

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.

Closes T15264

Test Plan

Phorge homepage is displayed on PHP 8.1 after applying these changes

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

aklapper created this revision.Apr 22 2023, 13:17

Owners added a reviewer: O1: Blessed Committers.Apr 22 2023, 13:17

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

aklapper requested review of this revision.Apr 22 2023, 13:17

Harbormaster completed remote builds in B262: Diff 509.Apr 22 2023, 13:17

Fix numerous PHP 8.1 "strlen(null)" exceptions preventing homepage to display

Harbormaster completed remote builds in B263: Diff 510.Apr 22 2023, 13:30

valerio.bozzolan added inline comments.Apr 25 2023, 10:51

src/applications/config/check/PhabricatorDaemonsSetupCheck.php
56	✅ OK since the expected input domain is NULL (default) or a string. The phutil will throw for alien types, and it's OK.
src/applications/files/view/PhabricatorGlobalUploadTargetView.php
70	✅ OK since the expected input domain is NULL (default) or a string. The phutil will throw for alien types, and it's OK. Known usages: ./src/applications/xhprof/query/PhabricatorXHProfSampleSearchEngine.php: ->setHintText("\xE2\x87\xAA ".pht('Drop .xhprof Files to Import')) ./src/applications/calendar/query/PhabricatorCalendarEventSearchEngine.php: ->setHintText("\xE2\x87\xAA ".pht('Drop .ics Files to Import'))
src/applications/search/engine/PhabricatorProfileMenuEngine.php
1311	👀 This should be double-checked Sometime Phabricator handles IDs as integers and - if this happens here - phutil will throw.
src/applications/search/engine/PhabricatorProfileMenuItemView.php
143	✅ OK since the expected input domain is NULL (default) or a string. The phutil will throw for alien types, and it's OK.
179	✅ OK since the expected input domain is NULL (default) or a string. The phutil will throw for alien types, and it's OK. Example usage: ->setTooltip(pht('Silent Edit'))
src/view/page/menu/PhabricatorMainMenuView.php
336	✅ OK since the expected input domain is NULL (default) or a string. The phutil will throw for alien types, and it's OK. Context: ./src/applications/config/custom/PhabricatorCustomLogoConfigType.php public static function getLogoWordmark() { $logo = PhabricatorEnv::getEnvConfig('ui.logo'); return idx($logo, 'wordmarkText'); }
src/view/phui/PHUIObjectItemView.php
662–664	✅ OK since the expected input domain is NULL (default) or a string. The phutil will throw for alien types, and it's OK.

aklapper mentioned this in D25137: Fix PHP 8.1 "strlen(null)" exceptions to render the Account Creation page.Apr 27 2023, 08:11

valerio.bozzolan added inline comments.Apr 27 2023, 09:07

src/applications/home/menuitem/PhabricatorHomeLauncherProfileMenuItem.php
34	✅ OK since the expected input domain is NULL (default) or a string. The phutil will throw for alien types, and it's OK. Context input: src/applications/search/storage/PhabricatorProfileMenuItemConfiguration.php public function getMenuItemProperty($key, $default = null) { return idx($this->menuItemProperties, $key, $default); }
src/applications/search/menuitem/PhabricatorDashboardProfileMenuItem.php
120	✅ OK since the expected input domain is NULL (default) or a string. The phutil will throw for alien types, and it's OK. Context input: src/applications/search/menuitem/PhabricatorDashboardProfileMenuItem.php private function getName( PhabricatorProfileMenuItemConfiguration $config) { return $config->getMenuItemProperty('name'); } src/applications/search/storage/PhabricatorProfileMenuItemConfiguration.php public function getMenuItemProperty($key, $default = null) { return idx($this->menuItemProperties, $key, $default); }
src/applications/search/menuitem/PhabricatorEditEngineProfileMenuItem.php
74	✅ OK since the expected input domain is NULL (default) or a string. The phutil will throw for alien types, and it's OK. Context input: src/applications/search/menuitem/PhabricatorDashboardProfileMenuItem.php private function getName( PhabricatorProfileMenuItemConfiguration $config) { return $config->getMenuItemProperty('name'); } src/applications/search/storage/PhabricatorProfileMenuItemConfiguration.php public function getMenuItemProperty($key, $default = null) { return idx($this->menuItemProperties, $key, $default); }
src/applications/search/menuitem/PhabricatorManageProfileMenuItem.php
34	✅ OK since the expected input domain is NULL (default) or a string. The phutil will throw for alien types, and it's OK. Context input: src/applications/search/storage/PhabricatorProfileMenuItemConfiguration.php public function getMenuItemProperty($key, $default = null) { return idx($this->menuItemProperties, $key, $default); }
src/view/page/PhabricatorStandardPageView.php
191	✅ OK since the expected input domain is NULL (default) or a string. The phutil will throw for alien types, and it's OK.

valerio.bozzolan edited the summary of this revision. (Show Details)Apr 27 2023, 10:05

valerio.bozzolan accepted this revision.Apr 27 2023, 10:13

valerio.bozzolan added inline comments.

src/applications/search/engine/PhabricatorProfileMenuEngine.php
1311	It seems to me that `$item_id` comes from `buildResponse()` from the same class, from this line: $item_id = $request->getURIData('itemID'); So: public function getURIData($key, $default = null) { return idx($this->uriData, $key, $default); } And the `uriData` comes for example from: $request->setURIMap( array( 'id' => head($must_sign_docs)->getID(), )); At this point I can 99.9999% say that `ID` is just a string. If a related problem will be raised, we can surly adopt instead `phutil_nonempty_scalar` in the future.
src/view/phui/PHUIObjectItemListView.php
123	This is the most risky to me. ✅ Anyway I've done a series of spot checks and it seems everything that call `PHUIObjectItemListView#setHeader()` is always assuming a `string` or `NULL` (like `PhabricatorEditEngine#getSummaryHeader()` etc.) If we will encounter issues we can adopt `phutil_nonempty_scalar()` instead.

This revision is now accepted and ready to land.Apr 27 2023, 10:13

I can confirm that I successfully opened the homepage of Phorge after merging D25132 and D25137 locally 🎉 Thanks

Closed by commit rPb56d86e48d0f: Fix numerous PHP 8.1 "strlen(null)" exceptions preventing homepage to display. · Explain WhyApr 27 2023, 12:08

This revision was automatically updated to reflect the committed changes.

aklapper added a commit: rPb56d86e48d0f: Fix numerous PHP 8.1 "strlen(null)" exceptions preventing homepage to display.

valerio.bozzolan added inline comments.Apr 30 2023, 12:04

src/view/phui/PHUIObjectItemView.php
662–664	IMPORTANT: This can also be a `PhutilURI` object. So they should be `phutil_nonempty_stringlike()`. Steps to reproduce: Create Dashboard, add Panel: `Call to phutil_nonempty_string() expected null or a string, got: PhutilURI.`.