Page MenuHomePhorge

Fix "Undefined index" exception setting Meme text
ClosedPublic

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

Details

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

TimeTest
488 msPhabricatorLibraryTestCase::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 msPhabricatorCelerityTestCase::testCelerityMaps
1 assertion passed.
11 msPhabricatorConduitTestCase::testConduitMethods
1 assertion passed.
0 msPhabricatorInfrastructureTestCase::testApplicationsInstalled
1 assertion passed.
0 msPhabricatorInfrastructureTestCase::testRejectMySQLNonUTF8Queries
1 assertion passed.
View Full Test Results (1 Failed · 6 Passed)

Event Timeline

Fixed my dirty tree, sorry

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'];

@valerio.bozzolan: I've lost track a bit given the latest comments. Shall I commit the accepted version, or are changes required?

You have green light, feel free to land and thanks

Mine was just a proposal