Paths

Table of Contentst

Differential D25414

Fix PHP 8.1 "strlen(null)" exception rendering PHUISegmentBar without a label
Needs ReviewPublic
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

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

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.Mon, Aug 28, 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

Revision Contents
Changeset List

Path

Size

Packages

src/

view/

phui/

PHUISegmentBarView.php

2 lines

O1: Blessed Committers

Diff 1316

View Options

src/view/phui/PHUISegmentBarView.php

Fix PHP 8.1 "strlen(null)" exception rendering PHUISegmentBar without a labelNeeds ReviewPublicActions