Page MenuHomePhorge

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

Authored by aklapper on Wed, May 10, 13:59.
Referenced Files
Unknown Object (File)
Fri, Jun 2, 22:14
Unknown Object (File)
Wed, May 31, 06:42
Unknown Object (File)
Tue, May 30, 16:25
Unknown Object (File)
Sat, May 27, 14:09
Unknown Object (File)
Thu, May 25, 10:35
Unknown Object (File)
Mon, May 22, 04:00
Unknown Object (File)
Sun, May 14, 23:09
F288945: image.png
Fri, May 12, 08:12
"Yellow Medal" token, awarded by valerio.bozzolan.



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

rP Phorge
Lint Passed
Tests Passed
Build Status
Buildable 440
Build 440: arc lint + arc unit

Event Timeline

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}

image.png (170×170 px, 55 KB)

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)


Probably this would allow more memes

This revision now requires changes to proceed.Fri, May 12, 08:12

hoping to be useful, applied small fix to allow "0" as above/below text

Tested, now "0" is approved again as above/below text



This revision is now accepted and ready to land.Mon, May 22, 20:23