Fix PHP 8.1 "strlen(null)" exception which blocks rendering Notifications' Advanced Search page
ClosedPublic
Actions

Authored by aklapper on May 1 2023, 17:20.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15320: PHP 8.1 "strlen(null)" exception blocks rendering the Notifications' Advanced Search page

Commits

rPb44665aefe52: Fix PHP 8.1 "strlen(null)" exception which blocks rendering Notifications'…

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.

Closes T15320

Test Plan

Applied this change and /notification/query/advanced/ rendered in web browser.

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

aklapper created this revision.May 1 2023, 17:20

Owners added a reviewer: O1: Blessed Committers.May 1 2023, 17:20

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptMay 1 2023, 17:20

aklapper requested review of this revision.May 1 2023, 17:20

Harbormaster completed remote builds in B323: Diff 601.May 1 2023, 17:20

Thanks

Tested locally with / without the Notification checkbox. No implosions.

I also tested this on the advanced search on my Maniphest Tasks with custom boolean fields but that seems unrelated since the filters are "(Any)" and "(Require)" in that case. This confusing filter is not specific to this change, but it can be discussed here: T15177: Allow to query Tasks with a custom boolean field set to false

Ready to land.

yesyes

src/applications/search/engine/PhabricatorApplicationSearchEngine.php
875	✅ I tested `$request->getStr($key)` locally with `phlog()` and it seems it only assumes null or the string `"1"`. It should never assume the integer `1` to me, since the method `getStr()` is supposed to cast to string. The `phutil_nonempty_string()` will report any alien type (like integers), and that is OK.