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

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

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15622: PHP 8.1 "strlen(null)" exception rendering PHUISegmentBar without a label

Commits

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

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

aklapper created this revision.Aug 19 2023, 08:07

Owners added a reviewer: O1: Blessed Committers.Aug 19 2023, 08:07

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptAug 19 2023, 08:07

aklapper requested review of this revision.Aug 19 2023, 08:07

Harbormaster completed remote builds in B789: Diff 1311.Aug 19 2023, 08:07

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

Harbormaster completed remote builds in B793: Diff 1316.Aug 20 2023, 09:45

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 a subscriber: avivey.Aug 28 2023, 08:25

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