From 19662e33bc4565f69766dc2a16bb79d1c29e51fe Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 23 Jan 2020 11:47:31 -0800
Subject: [PATCH] In Herald transcript rendering, don't store display labels in
 keys

Summary:
Ref T13480. Currently, when Herald renders a transcript, it puts display labels into array keys. This is a bad pattern for several reasons, notably that the values must be scalar (so you can't add icons or other markup later) and the values must be unique (which is easily violated because many values are translated).

Instead, keep values as list items.

Test Plan: Viewed Herald transcripts, saw no (meaningful) change in rendering output.

Maniphest Tasks: T13480

Differential Revision: https://secure.phabricator.com/D20949
---
 .../controller/HeraldTranscriptController.php | 51 +++++++++++++------
 1 file changed, 35 insertions(+), 16 deletions(-)

diff --git a/src/applications/herald/controller/HeraldTranscriptController.php b/src/applications/herald/controller/HeraldTranscriptController.php
index 09a23408d3..bf4e6f635f 100644
--- a/src/applications/herald/controller/HeraldTranscriptController.php
+++ b/src/applications/herald/controller/HeraldTranscriptController.php
@@ -453,31 +453,47 @@ final class HeraldTranscriptController extends HeraldController {
 
     $object_xscript = $xscript->getObjectTranscript();
 
-    $data = array();
+    $rows = array();
     if ($object_xscript) {
       $phid = $object_xscript->getPHID();
       $handles = $this->handles;
 
-      $data += array(
-        pht('Object Name') => $object_xscript->getName(),
-        pht('Object Type') => $object_xscript->getType(),
-        pht('Object PHID') => $phid,
-        pht('Object Link') => $handles[$phid]->renderLink(),
+      $rows[] = array(
+        pht('Object Name'),
+        $object_xscript->getName(),
+      );
+
+      $rows[] = array(
+        pht('Object Type'),
+        $object_xscript->getType(),
+      );
+
+      $rows[] = array(
+        pht('Object PHID'),
+        $phid,
+      );
+
+      $rows[] = array(
+        pht('Object Link'),
+        $handles[$phid]->renderLink(),
       );
     }
 
-    $data += $xscript->getMetadataMap();
+    foreach ($xscript->getMetadataMap() as $key => $value) {
+      $rows[] = array(
+        $key,
+        $value,
+      );
+    }
 
     if ($object_xscript) {
       foreach ($object_xscript->getFields() as $field => $value) {
-        $field = idx($field_names, $field, '['.$field.'?]');
-        $data['Field: '.$field] = $value;
-      }
-    }
+        if (isset($field_names[$field])) {
+          $field_name = pht('Field: %s', $field_names[$field]);
+        } else {
+          $field_name = pht('Unknown Field ("%s")', $field_name);
+        }
 
-    $rows = array();
-    foreach ($data as $name => $value) {
-      if (!($value instanceof PhutilSafeHTML)) {
         if (!is_scalar($value) && !is_null($value)) {
           $value = implode("\n", $value);
         }
@@ -490,9 +506,12 @@ final class HeraldTranscriptController extends HeraldController {
             ),
             $value);
         }
-      }
 
-      $rows[] = array($name, $value);
+        $rows[] = array(
+          $field_name,
+          $value,
+        );
+      }
     }
 
     $property_list = new PHUIPropertyListView();