Fix PHP 8.1 "strlen(null)" exception on History page of Diffusion repo after renaming default branch
ClosedPublic
Actions

Authored by aklapper on May 30 2023, 18:16.

Details

Reviewers

speck

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15434: PHP 8.1 "strlen(null)" exception on History page of Diffusion repo after renaming default branch

Commits

rP9aa88ae0d7d4: 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=arcpatch-D25250, ref.master=18554ea76ceb, ref.arcpatch-D25250=fade4603a799), phorge(head=master, ref.master=e11c5486c92b)
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/repository/xaction/PhabricatorRepositoryDefaultBranchTransaction.php:25]

Closes T15434

Test Plan

After applying this change, after changing the name of the (previously empty) default branch of a Diffusion repository, and then going to the repository History at /diffusion/<someID>/manage/history/, the page renders correctly in the web browser instead of showing an exception.

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

aklapper created this revision.May 30 2023, 18:16

Owners added a reviewer: O1: Blessed Committers.May 30 2023, 18:16

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptMay 30 2023, 18:16

aklapper requested review of this revision.May 30 2023, 18:16

Harbormaster completed remote builds in B491: Diff 856.May 30 2023, 18:16

speck added inline comments.Jun 8 2023, 00:33

src/applications/repository/xaction/PhabricatorRepositoryDefaultBranchTransaction.php
20–25	Please update both conditions here as I think `$new` can also be null

speck requested changes to this revision.Jun 8 2023, 00:33

This revision now requires changes to proceed.Jun 8 2023, 00:33

Also pass $new to phutil_nonempty_string() and not only $old.
I'm not convinced that we should check $new and $old for === null here instead of using phutil_nonempty_string(). But of course you make the final decision.

Harbormaster completed remote builds in B517: Diff 898.Jun 8 2023, 09:57

These are likely fine. The reason for preferring null check instead is if there’s uncertainty that the values could ever be anything besides null or string. The nonemepty string check might reject something that was previously accepted, where an object that overrides __toString would have strlen and friends operate on the toString function but the object itself is still used/passed. In this case I think it’s likely fine.

This revision is now accepted and ready to land.Jun 8 2023, 11:08

Closed by commit rP9aa88ae0d7d4: Fix PHP 8.1 "strlen(null)" exception on History page of Diffusion repo after…. · Explain WhyJun 8 2023, 12:50

This revision was automatically updated to reflect the committed changes.

aklapper added a commit: rP9aa88ae0d7d4: Fix PHP 8.1 "strlen(null)" exception on History page of Diffusion repo after….