Page MenuHomePhorge

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

Authored by aklapper on May 10 2023, 13:59.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 26, 06:08
Unknown Object (File)
Thu, Mar 21, 06:43
Unknown Object (File)
Mon, Mar 18, 23:44
Unknown Object (File)
Tue, Mar 12, 14:33
Unknown Object (File)
Fri, Mar 8, 14:19
Unknown Object (File)
Feb 25 2024, 07:40
Unknown Object (File)
Feb 25 2024, 07:40
Unknown Object (File)
Feb 25 2024, 07:40
Tokens
"Yellow Medal" token, awarded by valerio.bozzolan.

Details

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
Branch
master
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 389
Build 389: 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)

src/applications/macro/engine/PhabricatorMemeEngine.php
185–186

Probably this would allow more memes

This revision now requires changes to proceed.May 12 2023, 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

Yahiii

sgtm

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