Page MenuHomePhorge

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

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

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=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

src/applications/diffusion/controller/DiffusionCommitController.php
30–31

callsign probably also needs a similar safety check

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

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