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
F3330961: D25212.1743497938.diff
Mon, Mar 31, 08:58
F3327718: D25212.1743444558.diff
Sun, Mar 30, 18:09
F3319675: D25212.1743312549.diff
Sat, Mar 29, 05:29
F3318133: D25212.1743274986.diff
Fri, Mar 28, 19:03
F3317529: D25212.1743263543.diff
Fri, Mar 28, 15:52
F3314669: D25212.1743233109.diff
Fri, Mar 28, 07:25
F3309408: D25212.1743184266.diff
Thu, Mar 27, 17:51
F3307026: D25212.1743136909.diff
Thu, Mar 27, 04:41
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
arcpatch-D25212_1
Lint
Lint Passed
Unit
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)

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

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