From a26cf20dd128723a2d7016c8ccef24d6a00d0b09 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 19 Jan 2018 06:15:52 -0800
Subject: [PATCH] Fix a bug with setting custom PHID list field values via
 Conduit and prepare for bulk edits

Summary:
Ref T13025. Custom field transactions work somewhat unusually: the values sometimes need to be encoded. We currently do not apply this encoding correctly via Conduit.

For example, setting some custom PHID field to `["PHID-X-Y"]` fails with a bunch of JSON errors.

Add an extra hook callback so that EditTypes can apply processing to transaction values, then apply the correct CustomField processing.

This only affects Conduit. In a future diff, this also allows bulk edit of custom fields to work correctly.

Test Plan: Added a custom field to Maniphest with a list of projects. Used Conduit to bulk edit it (which now works, but did not before). Used the web UI to bulk edit it.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13025

Differential Revision: https://secure.phabricator.com/D18876
---
 .../editengine/PhabricatorEditEngine.php           | 14 ++++++++++----
 .../transactions/edittype/PhabricatorEditType.php  | 12 ++++++++++++
 .../editor/PhabricatorCustomFieldEditType.php      | 10 ++++++++++
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php
index 2abc419895..1d03896cb5 100644
--- a/src/applications/transactions/editengine/PhabricatorEditEngine.php
+++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php
@@ -2169,6 +2169,8 @@ abstract class PhabricatorEditEngine
         ->setTransactionType(PhabricatorTransactions::TYPE_CREATE);
     }
 
+    $is_strict = $request->getIsStrictlyTyped();
+
     foreach ($xactions as $xaction) {
       $type = $types[$xaction['type']];
 
@@ -2179,10 +2181,10 @@ abstract class PhabricatorEditEngine
       $parameter_type->setViewer($viewer);
 
       try {
-        $xaction['value'] = $parameter_type->getValue(
-          $xaction,
-          'value',
-          $request->getIsStrictlyTyped());
+        $value = $xaction['value'];
+        $value = $parameter_type->getValue($xaction, 'value', $is_strict);
+        $value = $type->getTransactionValueFromConduit($value);
+        $xaction['value'] = $value;
       } catch (Exception $ex) {
         throw new PhutilProxyException(
           pht(
@@ -2498,6 +2500,10 @@ abstract class PhabricatorEditEngine
       // but it's possible that this isn't the case.
       $xaction['type'] = $edit_type->getTransactionType();
 
+      $value = $xaction['value'];
+      $value = $edit_type->getTransactionValueFromBulkEdit($value);
+      $xaction['value'] = $value;
+
       $xaction_objects = $edit_type->generateTransactions(
         clone $template,
         $xaction);
diff --git a/src/applications/transactions/edittype/PhabricatorEditType.php b/src/applications/transactions/edittype/PhabricatorEditType.php
index de9404866d..51eed77f89 100644
--- a/src/applications/transactions/edittype/PhabricatorEditType.php
+++ b/src/applications/transactions/edittype/PhabricatorEditType.php
@@ -91,6 +91,10 @@ abstract class PhabricatorEditType extends Phobject {
     return $this->editField;
   }
 
+  protected function getTransactionValueFromValue($value) {
+    return $value;
+  }
+
 
 /* -(  Bulk  )--------------------------------------------------------------- */
 
@@ -114,6 +118,10 @@ abstract class PhabricatorEditType extends Phobject {
     return $this->newBulkParameterType();
   }
 
+  public function getTransactionValueFromBulkEdit($value) {
+    return $this->getTransactionValueFromValue($value);
+  }
+
 
 /* -(  Conduit  )------------------------------------------------------------ */
 
@@ -192,4 +200,8 @@ abstract class PhabricatorEditType extends Phobject {
     return $this->conduitDocumentation;
   }
 
+  public function getTransactionValueFromConduit($value) {
+    return $this->getTransactionValueFromValue($value);
+  }
+
 }
diff --git a/src/infrastructure/customfield/editor/PhabricatorCustomFieldEditType.php b/src/infrastructure/customfield/editor/PhabricatorCustomFieldEditType.php
index 2f8008c699..88c8fe07a8 100644
--- a/src/infrastructure/customfield/editor/PhabricatorCustomFieldEditType.php
+++ b/src/infrastructure/customfield/editor/PhabricatorCustomFieldEditType.php
@@ -40,4 +40,14 @@ final class PhabricatorCustomFieldEditType
     return array($xaction);
   }
 
+  protected function getTransactionValueFromValue($value) {
+    $field = $this->getCustomField();
+
+    // Avoid changing the value of the field itself, since later calls would
+    // incorrectly reflect the new value.
+    $clone = clone $field;
+    $clone->setValueFromApplicationTransactions($value);
+    return $clone->getNewValueForApplicationTransactions();
+  }
+
 }