Paths

Table of Contentst

Differential D25137

Fix PHP 8.1 "strlen(null)" exceptions to render the Account Creation page
ClosedPublic
Actions

Authored by aklapper on Apr 27 2023, 07:51.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15279: PHP 8.1 "strlen(null)" exceptions block rendering the Account Creation page after installation

Commits

rP562d36ef5f9e: Fix PHP 8.1 "strlen(null)" exceptions to render the Account Creation page

Summary

Fix numerous PHP 8.1 "strlen(null)" exceptions which block rendering the initial Account
Creation page in a fresh Phorge installation.

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 T15279

Test Plan

After these code changes the account creation page got displayed (though without CSS and JS).

Diff Detail

Repository

rP Phorge

Branch

arcpatch-D25137_2

Lint

Lint Passed

Unit

Test Failures

Build Status

Buildable 278
Build 278: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	214 ms	PhabricatorAuthPasswordTestCase::testPasswordEngine EXCEPTION (RuntimeException): Implicit conversion from float 76.50000000000001 to int loses precision #0 /var/www/phorge/src/applications/files/builtin/PhabricatorFilesComposeAvatarBuiltinFile.php(192): PhutilErrorHandler::handleError(8192, '...', '...', 192) #1 /var/www/phorge/src/applications/files/builtin/PhabricatorFilesComposeAvatarBuiltinFile.php(148): PhabricatorFilesComposeAvatarBuiltinFile::rgba2gd(Array)
	166 ms	PhabricatorAuthPasswordTestCase::testPasswordUpgrade EXCEPTION (RuntimeException): Implicit conversion from float 178.5 to int loses precision #0 /var/www/phorge/src/applications/files/builtin/PhabricatorFilesComposeAvatarBuiltinFile.php(192): PhutilErrorHandler::handleError(8192, '...', '...', 192) #1 /var/www/phorge/src/applications/files/builtin/PhabricatorFilesComposeAvatarBuiltinFile.php(148): PhabricatorFilesComposeAvatarBuiltinFile::rgba2gd(Array)
	423 ms	PhabricatorAccessControlTestCase::testControllerAccessControls 59 assertions passed.
	257 ms	PhabricatorAphrontViewTestCase::testHasChildren 6 assertions passed.
	256 ms	PhabricatorApplicationTestCase::testGetAllApplications 1 assertion passed.
		View Full Test Results (2 Failed · 17 Passed)

Event Timeline

aklapper created this revision.Apr 27 2023, 07:51

Owners added a reviewer: O1: Blessed Committers.Apr 27 2023, 07:51

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptApr 27 2023, 07:51

aklapper requested review of this revision.Apr 27 2023, 07:51

Harbormaster completed remote builds in B274: Diff 534.Apr 27 2023, 07:51

aklapper mentioned this in T15283: Fix PHP 8.1 "strlen(null)" exception inCelerityResourceController to make CSS/JS load.Apr 27 2023, 08:03

aklapper mentioned this in D25138: Fix PHP 8.1 "strlen(null)" exception inCelerityResourceController to make CSS/JS load.Apr 27 2023, 08:10

Note that this has some inevitable overlap with D25132.

aklapper mentioned this in rPf3b37413167e: Fix PHP 8.1 "strlen(null)" exception inCelerityResourceController to make….Apr 27 2023, 08:51

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

valerio.bozzolan mentioned this in D25132: Fix numerous PHP 8.1 "strlen(null)" exceptions preventing homepage to display.Apr 27 2023, 10:49

valerio.bozzolan added inline comments.Apr 27 2023, 10:56

src/applications/auth/controller/PhabricatorAuthRegisterController.php
21	✅ The input value is probably safe to be assumed as `string` or NULL since it comes from `$request->getURIData('akey')`
src/applications/base/controller/PhabricatorController.php
77	✅ The input value is probably safe to be assumed as `string` or NULL
src/applications/config/check/PhabricatorDaemonsSetupCheck.php
56 ↗	(On Diff #534)	✅ The input value is safe to be assumed as `string` or NULL
src/view/page/PhabricatorStandardPageView.php
183 ↗	(On Diff #534)	This returns a string
191 ↗	(On Diff #534)	✅ The input value is probably safe to be assumed as `string` or NULL
src/view/page/menu/PhabricatorMainMenuView.php
336 ↗	(On Diff #534)	✅ The input value is safe to be assumed as `string` or NULL ./src/applications/config/custom/PhabricatorCustomLogoConfigType.php public static function getLogoWordmark() { $logo = PhabricatorEnv::getEnvConfig('ui.logo'); // this can return NULL or an array return idx($logo, 'wordmarkText'); // this can return a string or NULL }

aklapper mentioned this in T15284: Installation: PHP 8.1 "strlen(null)" and "explode" exceptions block rendering Administrator Account Creation page.Apr 27 2023, 13:22

git rebase master

Harbormaster completed remote builds in B278: Diff 541.Apr 28 2023, 11:33

git rebase master (again?)

Harbormaster completed remote builds in B279: Diff 542.Apr 28 2023, 11:36

valerio.bozzolan added a subscriber: avivey.Apr 28 2023, 11:43

valerio.bozzolan added inline comments.

src/view/form/control/AphrontFormControl.php
175	The problem with `if($error)` is that it discards errors message like `"0"` and Evan is sad. Probably this is a good example for `phutil_nonempty_scalar()` since it accepts booleans and other stuff. But we need to merge this before: D25117: phutil_nonempty_scalar(): don't throw when receiving a boolean scalar @avivey what do you think about?

Thanks. I have a very small concern on AphrontFormControl.php:50 but really anything important. A simple if is probably just elegant in that case since the string "0" or the number 0 are not useful error messages that should be shown to a frontend.

I start accepting this as O1 since such a severe crash in PHP8.1 doesn't justify not trying to manage it as best we can, and you did it.

sgtm

src/applications/auth/controller/PhabricatorAuthRegisterController.php
247	✅ It's OK to assume a username as string or as NULL (default). Alien values will be reported and this is OK.
249	✅ It's really OK to assume a username as string or as NULL (default). Alien values will be reported and this is OK.
src/view/form/control/AphrontFormControl.php
190	✅ The input value is probably safe to be assumed as `string` or NULL. Alien values will be reported and that is OK. I checked 30+ usages and the label is probably always a raw string containing HTML for example and I have not noticed objects with toString() methods. Average usage example: ->setLabel(pht('Subscribers'))
206	✅ The input value is probably safe to be assumed as `string` or NULL. Alien values will be reported and that is OK. I checked 30+ usages and the caption is always a raw string containing HTML for example and I have not noticed objects with toString() methods. Average usage example: ->setCaption( pht('Example: %s', phutil_tag('tt', array(), 'JIS-3, JIS-9'))))