Fix PHP 8.1 "trim(null)" exception which blocks rendering Conduit's harbormaster.sendmessage page
ClosedPublic
Actions

Authored by aklapper on May 29 2023, 14:54.

Details

Reviewers

speck
valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15427: PHP 8.1 "trim(null)" exception blocks rendering Conduit's harbormaster.sendmessage page

Commits

rPbab997074055: Fix PHP 8.1 "trim(null)" exception which blocks rendering Conduit's…

Summary

Since PHP 8.1, passing a null string to trim() is deprecated.
Thus first check that $content is not null before trimming it.

Also since trim() returns a string and never null, we can also simplify the non-empty check,
in a more readable and efficient way, avoiding strlen() that was usually used for other "more wild" cases.

EXCEPTION: (RuntimeException) trim(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=18554ea76ceb), phorge(head=master, ref.master=0d81da590923)
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<arcanist>/src/error/PhutilErrorHandler.php:261]
  #1 <#2> trim(NULL) called at [<phorge>/src/infrastructure/markup/blockrule/PhutilRemarkupTableBlockRule.php:117]

Closes T15427

Test Plan

Applied this change, afterwards /conduit/method/harbormaster.sendmessage/ correctly rendered in web browser.

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

aklapper created this revision.May 29 2023, 14:54

Owners added a reviewer: O1: Blessed Committers.May 29 2023, 14:54

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptMay 29 2023, 14:54

aklapper requested review of this revision.May 29 2023, 14:54

Harbormaster completed remote builds in B478: Diff 837.May 29 2023, 14:54

aklapper edited the summary of this revision. (Show Details)May 30 2023, 14:57

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

speck added inline comments.

src/infrastructure/markup/blockrule/PhutilRemarkupTableBlockRule.php
117–118	Please simplify this

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

valerio.bozzolan added inline comments.Jun 10 2023, 10:19

src/infrastructure/markup/blockrule/PhutilRemarkupTableBlockRule.php
117–118	Another possibility: if ($content === null \|\| trim($content) === '') {

Simplify logic; use null check instead

Harbormaster completed remote builds in B525: Diff 912.Jun 10 2023, 11:09

valerio.bozzolan edited the summary of this revision. (Show Details)Jun 10 2023, 11:17

Thanks for the edit :) I would wait for another "OK" also from speck, premising that sgtm

I think !strlen(trim($var)) is more semantically meaningful than comparing to the empty string but this is fine. Locating calls to strlen which do not compare its return value to another value is more indicative of a non-empty check m, and easier to identify later on

This revision is now accepted and ready to land.Jun 10 2023, 14:22

Closed by commit rPbab997074055: Fix PHP 8.1 "trim(null)" exception which blocks rendering Conduit's…. · Explain WhyJun 10 2023, 14:26

This revision was automatically updated to reflect the committed changes.

aklapper added a commit: rPbab997074055: Fix PHP 8.1 "trim(null)" exception which blocks rendering Conduit's….