Fix "Undefined index" exception setting Meme text
ClosedPublic
Actions

Authored by aklapper on Sep 16 2023, 12:14.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15637: Undefined index exception setting Meme text

Commits

rP7b0021a03cb2: Fix "Undefined index" exception setting Meme text

Summary

strlen() was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts phutil_nonempty_string() as a replacement.

Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.

EXCEPTION: (RuntimeException) Undefined index: above at [<arcanist>/src/error/PhutilErrorHandler.php:251]
 arcanist(), phorge()
   #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<phorge>/src/applications/macro/engine/PhabricatorMemeEngine.php:276]

Closes T15637

Test Plan

Create a meme called "angrycat" from the /macro/ page, and try a comment like this, expecting no nuclear implosion:

{meme, src=angrycat, below=}
{meme, src=angrycat, above=}
{meme, src=angrycat, below=, above=}
{meme, src=angrycat, below=  , above=  }
{meme, src=angrycat, below=asd}
{meme, src=angrycat, above=asd}
{meme, src=angrycat, above=asd, below=dsa}
{meme, src=angrycat, above=   asd   , below=   dsa  }

Also carefully read the code with your big eyes, keeping in mind that strlen does not accept passing null in PHP 8, and looking at what we did in rPb4cfe56f03b44615ac9251aed8d74bf13b085051.

Diff Detail

Repository

rP Phorge

Branch

memeTextNull (branched from master)

Lint

Lint Passed

Unit

Test Failures

Build Status

Buildable 842
Build 842: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	488 ms	PhabricatorLibraryTestCase::testLibraryMap Assertion failed, expected 'true' (at PhutilLibraryTestCase.php:45): The library map is out of date. Rebuild it with `arc liberate`. These entries differ: class.PhabricatorProjectDescriptionTransaction, xmap.PhabricatorProjectDescriptionTransaction.
	218 ms	PhabricatorCelerityTestCase::testCelerityMaps 1 assertion passed.
	11 ms	PhabricatorConduitTestCase::testConduitMethods 1 assertion passed.
	0 ms	PhabricatorInfrastructureTestCase::testApplicationsInstalled 1 assertion passed.
	0 ms	PhabricatorInfrastructureTestCase::testRejectMySQLNonUTF8Queries 1 assertion passed.
		View Full Test Results (1 Failed · 6 Passed)

Event Timeline

aklapper created this revision.Sep 16 2023, 12:14

Owners added a reviewer: O1: Blessed Committers.Sep 16 2023, 12:14

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptSep 16 2023, 12:14

aklapper requested review of this revision.Sep 16 2023, 12:14

Harbormaster completed remote builds in B842: Diff 1386.Sep 16 2023, 12:14

Fixed my dirty tree, sorry

Harbormaster completed remote builds in B843: Diff 1387.Sep 16 2023, 12:16

Thanks

Maybe other people would like to propose a coalesce() and always trim() that and just have the if(phutil_nonempty...($)), but to be honest this is just correct and somehow minimal.

I tested this with:

{meme, src=angrycat, below=}

{meme, src=angrycat, above=}

{meme, src=angrycat, below=, above=}

{meme, src=angrycat, below=  , above=  }

{meme, src=angrycat, below=asd}

{meme, src=angrycat, above=asd}

{meme, src=angrycat, above=asd, below=dsa}

{meme, src=angrycat, above=   asd   , below=   dsa  }

This revision is now accepted and ready to land.Sep 29 2023, 07:54

There is a thing, to be honest. We are trimming the $above just in the test, but then we are not using the trimmed string in the rendering. So, the $v = coalesce($v, '') thing, and always trim() later, may still be better.

What about something like this?

So we add a bit of documentation, plus some commodity methods to avoid NULL at all costs, and only manage strings. So we have simpler business logic.

diff --git a/src/applications/macro/engine/PhabricatorMemeEngine.php b/src/applications/macro/engine/PhabricatorMemeEngine.php
index e1befc20a2..f97fb002ae 100644
--- a/src/applications/macro/engine/PhabricatorMemeEngine.php
+++ b/src/applications/macro/engine/PhabricatorMemeEngine.php
@@ -33,19 +33,39 @@ final class PhabricatorMemeEngine extends Phobject {
     return $this;
   }
 
+  /**
+   * @return wild
+   */
   public function getAboveText() {
     return $this->aboveText;
   }
 
+  /**
+   * @return string
+   */
+  public function getAboveTextStr() {
+    return phutil_string_cast($this->getAboveText());
+  }
+
   public function setBelowText($below_text) {
     $this->belowText = $below_text;
     return $this;
   }
 
+  /**
+   * @return wild
+   */
   public function getBelowText() {
     return $this->belowText;
   }
 
+  /**
+   * @return string
+   */
+  public function getBelowTextStr() {
+    return phutil_string_cast($this->getBelowText());
+  }
+
   public function getGenerateURI() {
     $params = array(
       'macro' => $this->getTemplate(),
@@ -272,16 +292,16 @@ final class PhabricatorMemeEngine extends Phobject {
     $font = $this->getFont();
     $size = $metrics['size'];
 
-    $above = $this->getAboveText();
-    if (strlen($above)) {
+    $above = trim($this->getAboveTextStr());
+    if ($above !== '') {
       $x = (int)floor(($dx - $metrics['text']['above']['width']) / 2);
       $y = $metrics['text']['above']['height'] + 12;
 
       $this->drawText($img, $font, $metrics['size'], $x, $y, $above);
     }
 
-    $below = $this->getBelowText();
-    if (strlen($below)) {
+    $below = trim($this->getBelowTextStr());
+    if ($below !== '') {
       $x = (int)floor(($dx - $metrics['text']['below']['width']) / 2);
       $y = $dy - 12 - $metrics['text']['below']['descend'];