Page MenuHomePhorge

Fix PHP 8.1 null parameter exceptions which block rendering the "Browse Projects" overlay dialog
ClosedPublic

Authored by aklapper on May 3 2023, 14:00.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 26, 08:38
Unknown Object (File)
Tue, Mar 26, 08:38
Unknown Object (File)
Tue, Mar 26, 08:38
Unknown Object (File)
Tue, Mar 26, 07:57
Unknown Object (File)
Tue, Mar 26, 07:28
Unknown Object (File)
Sun, Mar 24, 06:28
Unknown Object (File)
Sat, Mar 23, 07:17
Unknown Object (File)
Fri, Mar 22, 08:05
Tokens
"Like" token, awarded by valerio.bozzolan.

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 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, passing null to the $haystack parameter of strpos() is deprecated in PHP 8.1.

Similarly, passing null to the $string parameter of ltrim() is deprecated in PHP 8.1.

Closes part of T15335

Test Plan

Applied these four changes in Phorge (plus the one change in D25180 in Arcanist) and the Browse Projects overlay dialog finally rendered in web browser and listed existing projects.

Diff Detail

Repository
rP Phorge
Branch
workbColAutoCompl (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 345
Build 345: arc lint + arc unit

Event Timeline

aklapper requested review of this revision.May 3 2023, 14:00

Thanks for this patch

I start approving this as behalf of myself, but

⏲ I wait before approving as O1 since I imagine at least one person in O1 would say the suggestion shared in PhabricatorProjectDatasource.php line 24

src/applications/project/typeahead/PhabricatorProjectDatasource.php
24

Don't hate me, but I think Evan would love to be able to search by string "0" (currently skipped since it's falsy)

146–147

✅ I verified the above line

Note that another known alternative could be:

$description = idx($descriptions, $phid, '');
if ($description !== '') {

But in my honest opinion your already-existing approach is better than the above one and avoids some possible facets.

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

src/applications/typeahead/datasource/PhabricatorTypeaheadCompositeDatasource.php
31

✅ Yes, the input string is always a string or null.

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

src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php
467

✅ I verified the above line

Yes, we are aware that the if($token) skips the string "0" and it has sense here.

This is valid since a function token must contain a bracket to be valid.

src/applications/project/typeahead/PhabricatorProjectDatasource.php
24

Hi @aklapper, if you change this small line I will immediately +1 since it would be totally OK also from O1 in my opinion

amend small stuff proposed in person to the kind @aklapper

Thaaaaanks

Tested locally following the test plan and something more, no nuclear implosions expected (as I usual say, but this time should be true - as I usual say, but really, this should not cause implosions)

lgtm

This revision is now accepted and ready to land.May 20 2023, 15:37