From 6d5f265a57957d9b7bbba54a0355bd004b8619aa Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 31 Jan 2018 11:11:29 -0800
Subject: [PATCH] Accept `null` via `conduit.edit` to unassign a task

Summary:
See <https://discourse.phabricator-community.org/t/maniphest-edit-to-unassign-owner-documentation-is-wrong/1053>. This unusual field doesn't actually accept `null`, although the documentation says it does and that was the intent.

Accept `null`, and show `phid|null` in the docs.

Test Plan: Viewed docs, saw `phid|null`. Unassigned with `null`.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18976
---
 .../ConduitPHIDParameterType.php              | 31 +++++++++++++++++--
 .../maniphest/editor/ManiphestEditEngine.php  |  1 +
 .../PhabricatorPHIDListEditField.php          | 16 ++++++++--
 .../edittype/PhabricatorPHIDListEditType.php  | 21 +++++++------
 4 files changed, 56 insertions(+), 13 deletions(-)

diff --git a/src/applications/conduit/parametertype/ConduitPHIDParameterType.php b/src/applications/conduit/parametertype/ConduitPHIDParameterType.php
index 3bb45697dc..eafee4d453 100644
--- a/src/applications/conduit/parametertype/ConduitPHIDParameterType.php
+++ b/src/applications/conduit/parametertype/ConduitPHIDParameterType.php
@@ -3,9 +3,26 @@
 final class ConduitPHIDParameterType
   extends ConduitParameterType {
 
+  private $isNullable;
+
+  public function setIsNullable($is_nullable) {
+    $this->isNullable = $is_nullable;
+    return $this;
+  }
+
+  public function getIsNullable() {
+    return $this->isNullable;
+  }
+
   protected function getParameterValue(array $request, $key, $strict) {
     $value = parent::getParameterValue($request, $key, $strict);
 
+    if ($this->getIsNullable()) {
+      if ($value === null) {
+        return $value;
+      }
+    }
+
     if (!is_string($value)) {
       $this->raiseValidationException(
         $request,
@@ -17,7 +34,11 @@ final class ConduitPHIDParameterType
   }
 
   protected function getParameterTypeName() {
-    return 'phid';
+    if ($this->getIsNullable()) {
+      return 'phid|null';
+    } else {
+      return 'phid';
+    }
   }
 
   protected function getParameterFormatDescriptions() {
@@ -27,9 +48,15 @@ final class ConduitPHIDParameterType
   }
 
   protected function getParameterExamples() {
-    return array(
+    $examples = array(
       '"PHID-WXYZ-1111222233334444"',
     );
+
+    if ($this->getIsNullable()) {
+      $examples[] = 'null';
+    }
+
+    return $examples;
   }
 
 }
diff --git a/src/applications/maniphest/editor/ManiphestEditEngine.php b/src/applications/maniphest/editor/ManiphestEditEngine.php
index 359ba493d4..c270104034 100644
--- a/src/applications/maniphest/editor/ManiphestEditEngine.php
+++ b/src/applications/maniphest/editor/ManiphestEditEngine.php
@@ -196,6 +196,7 @@ EODOCS
           pht('New task owner, or `null` to unassign.'))
         ->setTransactionType(ManiphestTaskOwnerTransaction::TRANSACTIONTYPE)
         ->setIsCopyable(true)
+        ->setIsNullable(true)
         ->setSingleValue($object->getOwnerPHID())
         ->setCommentActionLabel(pht('Assign / Claim'))
         ->setCommentActionValue($owner_value),
diff --git a/src/applications/transactions/editfield/PhabricatorPHIDListEditField.php b/src/applications/transactions/editfield/PhabricatorPHIDListEditField.php
index b084c142e5..d2b647b9aa 100644
--- a/src/applications/transactions/editfield/PhabricatorPHIDListEditField.php
+++ b/src/applications/transactions/editfield/PhabricatorPHIDListEditField.php
@@ -5,6 +5,7 @@ abstract class PhabricatorPHIDListEditField
 
   private $useEdgeTransactions;
   private $isSingleValue;
+  private $isNullable;
 
   public function setUseEdgeTransactions($use_edge_transactions) {
     $this->useEdgeTransactions = $use_edge_transactions;
@@ -30,13 +31,23 @@ abstract class PhabricatorPHIDListEditField
     return $this->isSingleValue;
   }
 
+  public function setIsNullable($is_nullable) {
+    $this->isNullable = $is_nullable;
+    return $this;
+  }
+
+  public function getIsNullable() {
+    return $this->isNullable;
+  }
+
   protected function newHTTPParameterType() {
     return new AphrontPHIDListHTTPParameterType();
   }
 
   protected function newConduitParameterType() {
     if ($this->getIsSingleValue()) {
-      return new ConduitPHIDParameterType();
+      return id(new ConduitPHIDParameterType())
+        ->setIsNullable($this->getIsNullable());
     } else {
       return new ConduitPHIDListParameterType();
     }
@@ -99,7 +110,8 @@ abstract class PhabricatorPHIDListEditField
     }
 
     return id(new PhabricatorDatasourceEditType())
-      ->setIsSingleValue($this->getIsSingleValue());
+      ->setIsSingleValue($this->getIsSingleValue())
+      ->setIsNullable($this->getIsNullable());
   }
 
   protected function newBulkEditTypes() {
diff --git a/src/applications/transactions/edittype/PhabricatorPHIDListEditType.php b/src/applications/transactions/edittype/PhabricatorPHIDListEditType.php
index 07489521b6..763f6ff001 100644
--- a/src/applications/transactions/edittype/PhabricatorPHIDListEditType.php
+++ b/src/applications/transactions/edittype/PhabricatorPHIDListEditType.php
@@ -6,6 +6,7 @@ abstract class PhabricatorPHIDListEditType
   private $datasource;
   private $isSingleValue;
   private $defaultValue;
+  private $isNullable;
 
   public function setDatasource(PhabricatorTypeaheadDatasource $datasource) {
     $this->datasource = $datasource;
@@ -30,16 +31,17 @@ abstract class PhabricatorPHIDListEditType
     return $this;
   }
 
-  public function getDefaultValue() {
-    return $this->defaultValue;
+  public function setIsNullable($is_nullable) {
+    $this->isNullable = $is_nullable;
+    return $this;
   }
 
-  public function getValueType() {
-    if ($this->getIsSingleValue()) {
-      return 'phid';
-    } else {
-      return 'list<phid>';
-    }
+  public function getIsNullable() {
+    return $this->isNullable;
+  }
+
+  public function getDefaultValue() {
+    return $this->defaultValue;
   }
 
   protected function newConduitParameterType() {
@@ -49,7 +51,8 @@ abstract class PhabricatorPHIDListEditType
     }
 
     if ($this->getIsSingleValue()) {
-      return new ConduitPHIDParameterType();
+      return id(new ConduitPHIDParameterType())
+        ->setIsNullable($this->getIsNullable());
     } else {
       return new ConduitPHIDListParameterType();
     }
-- 
2.51.2