Page MenuHomePhorge

Fix PHP 8.1 "strlen(null)" exception which blocks creating personal and global Herald rules

Authored by aklapper on May 4 2023, 20:25.
Referenced Files
Unknown Object (File)
Wed, May 31, 06:36
Unknown Object (File)
Tue, May 23, 21:40
Unknown Object (File)
Mon, May 22, 06:05
Unknown Object (File)
Mon, May 22, 02:04
Unknown Object (File)
Sun, May 21, 10:41
Unknown Object (File)
Wed, May 17, 20:32
Unknown Object (File)
May 5 2023, 12:21



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 T15348

Test Plan

Applied this change (on top of D25157) and creation form at /herald/edit/?content_type=HeraldPreCommitRefAdapter&rule_type=personal rendered in web browser.
Same applies for other types of personal and global Herald rules.

Diff Detail

rP Phorge
Lint Not Applicable
Tests Not Applicable

Event Timeline

aklapper requested review of this revision.May 4 2023, 20:25

I tested this locally intensively creating a very weird entropy of possible Herald conditions. I also hacked my database manually, with dangerous fuzzy tests in the phabricator_herald.herald_condition database table, kind of:

INSERT INTO phabricator_hera.dherald_condition (id, ruleID,fieldName,fieldCondition,value) VALUES (NULL, 6, 'contentsource', 'is', '<VERY WEIRD STUFF>');

And I was not able to find any possible side-effect.

I can annotate here that all the available expected values for getSource() are just the following strings:

  • nuance
  • bulk
  • conduit
  • console
  • daemon
  • email
  • fax
  • herald
  • lipsum
  • legacy
  • phortune
  • unittest
  • unknown
  • web
  • (plus null)

So, hoping to be useful I will also add a small PHPDoc just to clarify this a bit.

Thanks for this change, green light



✅ Yes, we are aware that the string "0" is falsy. The string "0" cannot be a valid source name.

This revision is now accepted and ready to land.May 5 2023, 10:07

hoping to be useful, add a small PHPDoc to help future reviews