Page MenuHomePhorge

Fix PHP 8.1 "strlen(null)" exception rendering PHUISegmentBar without a label
ClosedPublic

Authored by aklapper on Aug 19 2023, 08:07.

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.

EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=df6c315ace5f), phorge(head=master, ref.master=3cc5ee6a33df)
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/view/phui/PHUISegmentBarView.php:33]

Closes T15622

Test Plan

After applying this change, page with the PHUISegmentBar without label renders as expected without an exception.

For example, visit this page:

/uiexample/view/PhabricatorAphrontBarUIExample/

Diff Detail

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

Event Timeline

I will kindly mark this as request changes since unfortunately we have a crash if the $label is an object like PhutilSafeHTML.

Tested with:

$tag = phutil_tag('span', array(), "asd");
$asd = id(new PHUISegmentBarView());
$asd->setLabel($tag);
src/view/phui/PHUISegmentBarView.php
8–11

Maybe a good moment to write this down

This revision now requires changes to proceed.Aug 20 2023, 07:48

Accept whatever stuff is passed as $label as long as it's not null, per last comment

I don't love this, but this is a general fix:

if ($label != null && strlen($label)) {

Also this is somehow appreciated:

$not_empty = phutil_string_cast($label) !== '';
if ($not_empty) {

Unfortunately this is not good because it generates the HTML tag even if the label is an empty string, or if it's an object with a __toString() method returning an empty string:

if ($label !== null) {

I'm personal in favor of this, but some people don't like it - so I cannot "hard +1" this alone:

if (phutil_nonempty_stringlike($label)) {

I would say, roll a die for the first two and I will immediately +1.

Hoping to be useful, this is now somehow covered by https://we.phorge.it/book/flavor/article/php_pitfalls/

valerio.bozzolan added inline comments.
src/view/phui/PHUISegmentBarView.php
33

Hi @avivey, are you OK with adopting this "like" thing here, but without logging anything?

So we can keep support to "whatever can be casted to string, and with non-empty string", keeping support to string|PhutilSafeHTML|null.

https://we.phorge.it/source/arcanist/browse/master/src/utils/utils.php$2134-2177

Thaanks! Over-tested intensively with NULL, 'asd', objects, aliens, lizards etc.

sgtm

This revision is now accepted and ready to land.Nov 11 2023, 21:01