Page MenuHomePhorge

Fix PHP 8.1 "strlen(null)" exceptions which block rendering the People page
ClosedPublic

Authored by aklapper on Apr 29 2023, 20:59.
Tags
None
Referenced Files
F2067930: D25147.id603.diff
Fri, Mar 29, 00:43
Unknown Object (File)
Tue, Mar 26, 12:45
Unknown Object (File)
Tue, Mar 26, 11:33
Unknown Object (File)
Fri, Mar 8, 09:00
Unknown Object (File)
Feb 27 2024, 15:39
Unknown Object (File)
Feb 25 2024, 07:36
Unknown Object (File)
Feb 25 2024, 07:36
Unknown Object (File)
Feb 25 2024, 07:36

Details

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 general 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.

Use phutil_nonempty_scalar() instead in PhabricatorSearchDateField.php as
input could only be integer instead of string.

Closes T15297

Test Plan

Applied these four changes (on top of D25144, D25145, D25146) and /people/ finally rendered in web browser.

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

  • hoping to help future people, added small PHPDoc on a private method to clarify the $icon that is a string (name in Font Awesome)
valerio.bozzolan added inline comments.
src/applications/files/controller/PhabricatorFileDataController.php
32

✅ It seems $alt_uri->getDomain() is null as default and usages seems to just use setDomain(string).

The phutil_nonempty_string() will report alien values and this is OK.

src/applications/search/field/PhabricatorSearchDateField.php
20
WARNING: The check phutil_nonempty_string() is probably too much strict here, since this function also should accept integers. This is probably a problem for Conduit API consumers sending integers. Note that parseDateTime() accepts integers by design.

In short:

Valuephutil_nonempty_scalar($v)
"foo"true
""false
nullfalse
"0"true
"1"true
0true
1true
0.5true
obj with tostringtrue
obj withno tostr.Exception
array()Exception

For reference:

Valuephutil_nonempty_string($v)
"foo"true
""false
nullfalse
"0"true
"1"true
0Exception
1Exception
0.5Exception
obj with tostringException
obj withno tostr.Exception
array()Exception
35
WARNING: Same as above.
src/view/layout/AphrontSideNavFilterView.php
109

✅ I tested with phlog() and it seems $item is just the string name of Font Awesome icon, or null.

The phutil_nonempty_string() will report alien values and that is OK.

This revision now requires changes to proceed.May 1 2023, 12:49

Fix PHP 8.1 "strlen(null)" exceptions which block rendering the People page

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.

Use phutil_nonempty_scalar() instead in PhabricatorSearchDateField.php as
input could only be integer instead of string.

Closes T15297

Test Plan:
Applied these four changes (on top of D25144, D25145, D25146) and /people/ finally rendered in web browser.

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

Thanks for this patch and for the rapid update

Tested locally again on /people/ and no nuclear implosions.

sgtm

This revision is now accepted and ready to land.May 1 2023, 20:28