Fix PHP 8.1 "strlen(null)" exception on History page of Diffusion repo after setting maintenance mode
ClosedPublic
Actions

Authored by aklapper on Jun 14 2023, 19:21.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15475: PHP 8.1 "strlen(null)" exception on History page of Diffusion repo after setting maintenance mode

Commits

rP71f38a90a358: Fix PHP 8.1 "strlen(null)" exception on History page of Diffusion repo after…

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=97e163187418), phorge(head=master, ref.master=1c098c273d06)
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/repository/xaction/PhabricatorRepositoryMaintenanceTransaction.php:28]

Closes T15475

Test Plan

Play a bit with maintenance messages:

./bin/repository maintenance --start "Start"      R1
./bin/repository maintenance --start "Start Yeah" R1
./bin/repository maintenance --stop               R1

Then visit /diffusion/1/manage/history/ and look at Diffusion put this repository into maintenance mode. as expected, instead of an exception.

Diff Detail

Repository

rP Phorge

Branch

repoMaintStartHistory (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 559
Build 559: arc lint + arc unit

Event Timeline

aklapper created this revision.Jun 14 2023, 19:21

Owners added a reviewer: O1: Blessed Committers.Jun 14 2023, 19:21

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptJun 14 2023, 19:21

aklapper requested review of this revision.Jun 14 2023, 19:21

Harbormaster completed remote builds in B559: Diff 964.Jun 14 2023, 19:21

Indeed the maintenance message should always be null or a string. Soft +1 for a week. Then hard +1.

valerio.bozzolan added inline comments.Jun 29 2023, 08:21

src/applications/repository/xaction/PhabricatorRepositoryMaintenanceTransaction.php
27	Thanks again. To make this a bit more readable (and a bit more performant) than the original legacy source code, maybe we can define these in line 27, and use these instead: $old_nonempty = phutil_nonempty_string($old); $new_nonempty = phutil_nonempty_string($new);