Fix PHP 8.1 "ltrim(null)" exception which blocks rendering first Meme comment
ClosedPublic
Actions

Authored by aklapper on May 10 2023, 13:59.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15379: PHP 8.1 "ltrim(null)" exception blocks rendering first Meme comment

Commits

rPb4cfe56f03b4: Fix PHP 8.1 "ltrim(null)" exception which blocks rendering first Meme comment

Summary

Since PHP 8.1, passing a null string to ltrim(string $string) is deprecated.

Thus first check if After and Below text are not null before trimming.

Closes T15379

Test Plan

Applied this change; afterwards several times created a new Pholio mock and added a Meme comment. I could not reproduce the problem anymore (first meme comment in a mock always rendered correctly instead of showing an exception as comment). However, I could not reliably reproduce the problem anyway.

Create a meme without any above/below text
Create a meme with just spaces as above/below text
Create a meme with just above text as "asd", or "0" or a lizard
Create a meme with just below text as "asd", or "0" or a lizard
Create a meme with both above and below texts with "asd", "0" and a lizard and more stuff (doing all 64 combinations)

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

aklapper created this revision.May 10 2023, 13:59

Owners added a reviewer: O1: Blessed Committers.May 10 2023, 13:59

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptMay 10 2023, 13:59

aklapper requested review of this revision.May 10 2023, 13:59

Harbormaster completed remote builds in B389: Diff 708.May 10 2023, 13:59

Thanks for addressing this issue

We still need to work on this since, before this change this was allowed:

{meme, src=somememe, above=0, below=0}

But after this change, the resulting meme has no above/below texts.

(Note: it's not so easy to test what I'm saying, since memes are cached from the above/below fingerprint, so, to test this locally, we have to upload another meme for each test to be conducted)