Fix PHP 8.1 "strlen(null)" exception which blocks rendering commit page in Diffusion
ClosedPublic
Actions

Authored by aklapper on Jun 12 2023, 01:34.

Details

Reviewers

speck

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15466: PHP 8.1 "strlen(null)" exception blocks rendering commit page in Diffusion

Commits

rPefbb809d420f: Fix PHP 8.1 "strlen(null)" exception which blocks rendering commit page in…

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=diffusionCallsignHistory, ref.master=9b99123ff931, ref.diffusionCallsignHistory=d5fb6702a49a)
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/diffusion/controller/DiffusionCommitController.php:31]

Closes T15466

Test Plan

After applying this change, going to an existing commit URL like /diffusion/XYZ/commit/999a2c7d1ff8 shows the exception covered by T15464 instead, as expected.

Diff Detail

Repository

rP Phorge

Branch

diffusionSourceCommit (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 542
Build 542: arc lint + arc unit

Event Timeline

aklapper created this revision.Jun 12 2023, 01:34

Owners added a reviewer: O1: Blessed Committers.Jun 12 2023, 01:34

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptJun 12 2023, 01:34

Harbormaster completed remote builds in B542: Diff 938.Jun 12 2023, 01:34

aklapper requested review of this revision.Jun 12 2023, 01:34

speck added inline comments.Jun 12 2023, 01:45

src/applications/diffusion/controller/DiffusionCommitController.php
30–31	callsign probably also needs a similar safety check

aklapper added inline comments.Jun 12 2023, 01:52

src/applications/diffusion/controller/DiffusionCommitController.php
30–31	I thought the same, however I'm never sure if we're still in the "only fix code that has triggered an exception" phase, or slowly enter the "also fix code found by reading code" phase... Will do!

Also add check for $has_callsign

Harbormaster completed remote builds in B546: Diff 945.Jun 12 2023, 01:56

speck accepted this revision.Jun 12 2023, 02:07

speck added inline comments.

src/applications/diffusion/controller/DiffusionCommitController.php
30–31	Ah, nice! I think it’s reasonable to address things in the vicinity of where you hit issues as it’s often difficult to hit every code path through testing. Particularly with these types of changes where the primary functionality being changed is checking for nulls.

This revision is now accepted and ready to land.Jun 12 2023, 02:07

Closed by commit rPefbb809d420f: Fix PHP 8.1 "strlen(null)" exception which blocks rendering commit page in…. · Explain WhyJun 12 2023, 09:21

This revision was automatically updated to reflect the committed changes.

aklapper added a commit: rPefbb809d420f: Fix PHP 8.1 "strlen(null)" exception which blocks rendering commit page in….