Page MenuHomePhorge

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

Authored by aklapper on May 4 2023, 20:25.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 26, 13:43
Unknown Object (File)
Tue, Mar 26, 13:43
Unknown Object (File)
Tue, Mar 26, 13:43
Unknown Object (File)
Tue, Mar 26, 12:44
Unknown Object (File)
Tue, Mar 26, 11:34
Unknown Object (File)
Thu, Mar 14, 23:37
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 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

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
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

sgtm

src/infrastructure/contentsource/PhabricatorUnknownContentSource.php
10

✅ 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