Page MenuHomePhorge

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

Authored by aklapper on Aug 19 2023, 08:07.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Sep 21, 06:31
Unknown Object (File)
Thu, Sep 21, 00:25
Unknown Object (File)
Wed, Aug 30, 08:07
Unknown Object (File)
Wed, Aug 30, 07:21
Unknown Object (File)
Sun, Aug 27, 02:22
Unknown Object (File)
Sat, Aug 26, 19:57
Unknown Object (File)
Sat, Aug 26, 09:05
Unknown Object (File)
Fri, Aug 25, 18:06

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.

Diff Detail

Repository
rP Phorge
Branch
PHUISegmentBarNoLabel (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 793
Build 793: 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