Page MenuHomePhorge

Fix Pholio RuntimeException: Undefined variable $dictionary (when adding an empty Inline Comment)
ClosedPublic

Authored by aklapper on Jun 9 2023, 18:55.

Details

Summary

Trying to create an empty inline comment in a Pholio mock, $dictionary does not get set as both strlen($v_content) and $inline->getID() are not true.

Thus show a more explanatory error message ('Comment cannot be empty.') to users instead of exposing internal variable names.

EXCEPTION: (RuntimeException) Undefined variable $dictionary at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=b325304b6e52), phorge(head=master, ref.master=980293b707a0)
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/pholio/controller/PholioInlineController.php:117]

Closes T15456

Test Plan

After applying this change, try to add an empty Inline Comment in a Pholio mock. See that the error message is now "Comment cannot be empty." instead of "Undefined variable $dictionary".

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

aklapper requested review of this revision.Jun 9 2023, 18:55

Thanks!

Soft +1 since generating an unhandled exception seems reasonable:

Screenshot_2023_06_12_230545.png (230×682 px, 12 KB)

But I discovered that we can also call setError() somewhere to show a nice user message. I tried in this way that maybe can be used as inspiration:

--- a/src/applications/pholio/controller/PholioInlineController.php
+++ b/src/applications/pholio/controller/PholioInlineController.php
@@ -102,6 +102,7 @@ final class PholioInlineController extends PholioController {
         ->addCancelButton($mock_uri, pht('Close'));
     }

+    $error = null;
     if ($request->isFormPost()) {
       $v_content = $request->getStr('content');

@@ -112,9 +113,13 @@ final class PholioInlineController extends PholioController {
       } else if ($inline->getID()) {
         $inline->delete();
         $dictionary = array();
+      } else {
+        $error = pht('Comment cannot be empty.');
       }

-      return id(new AphrontAjaxResponse())->setContent($dictionary);
+      if(!$error) {
+        return id(new AphrontAjaxResponse())->setContent($dictionary);
+      }
     }

     switch ($mode) {
@@ -151,7 +156,8 @@ final class PholioInlineController extends PholioController {
           ->setUser($viewer)
           ->setName('content')
           ->setLabel(pht('Comment'))
-          ->setValue($v_content));
+          ->setValue($v_content)
+          ->setError($error));

     return $this->newDialog()
       ->setTitle($title)

In that case, this would be the result - still very ugly to me - but somehow less raw, and also the popup is not closed and you can just continue typing:

Mockup empty comment handled error.png (328×822 px, 21 KB)

Would you love an amend also here, to try the setError approach?

What do you think about this? In this way there are no exceptions in the error log

Mockup empty comment handled error.png (328×822 px, 21 KB)

If you like, feel free to land

If you don't, I will immediately restore the previous version :)

This revision is now accepted and ready to land.Jun 16 2023, 14:23

I've tested the new revision with the soft error and this is a way better UX - thanks!