Normalize validation/requried API

Summary: Makes Maniphest look more like standard fields to make the migration easier.

Test Plan: Edited tasks and users with required and invalid fields.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7035

src/__phutil_library_map__.php

@@ -1063,7 +1063,6 @@ phutil_register_library_map(array(
     'PhabricatorCustomFieldNumericIndexStorage' => 'infrastructure/customfield/storage/PhabricatorCustomFieldNumericIndexStorage.php',
     'PhabricatorCustomFieldStorage' => 'infrastructure/customfield/storage/PhabricatorCustomFieldStorage.php',
     'PhabricatorCustomFieldStringIndexStorage' => 'infrastructure/customfield/storage/PhabricatorCustomFieldStringIndexStorage.php',
-    'PhabricatorCustomFieldValidationException' => 'infrastructure/customfield/exception/PhabricatorCustomFieldValidationException.php',
     'PhabricatorDaemon' => 'infrastructure/daemon/PhabricatorDaemon.php',
     'PhabricatorDaemonCombinedLogController' => 'applications/daemon/controller/PhabricatorDaemonCombinedLogController.php',
     'PhabricatorDaemonConsoleController' => 'applications/daemon/controller/PhabricatorDaemonConsoleController.php',
@@ -3182,7 +3181,6 @@ phutil_register_library_map(array(
     'PhabricatorCustomFieldNumericIndexStorage' => 'PhabricatorCustomFieldIndexStorage',
     'PhabricatorCustomFieldStorage' => 'PhabricatorLiskDAO',
     'PhabricatorCustomFieldStringIndexStorage' => 'PhabricatorCustomFieldIndexStorage',
-    'PhabricatorCustomFieldValidationException' => 'Exception',
     'PhabricatorDaemon' => 'PhutilDaemon',
     'PhabricatorDaemonCombinedLogController' => 'PhabricatorDaemonController',
     'PhabricatorDaemonConsoleController' => 'PhabricatorDaemonController',

src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php

@@ -273,42 +273,62 @@ class ManiphestAuxiliaryFieldDefaultSpecification
     return $this->setValue($value);
   }
 
-  public function validate() {
+  public function validateApplicationTransactions(
+    PhabricatorApplicationTransactionEditor $editor,
+    $type,
+    array $xactions) {
+
+    $errors = parent::validateApplicationTransactions(
+      $editor,
+      $type,
+      $xactions);
+
+    $has_value = false;
+    $value = null;
+    foreach ($xactions as $xaction) {
+      $has_value = true;
+      $value = $xaction->getNewValue();
       switch ($this->getFieldType()) {
         case self::TYPE_INT:
-        if ($this->getValue() && !is_numeric($this->getValue())) {
+          if ($value && !is_numeric($value)) {
-          throw new PhabricatorCustomFieldValidationException(
+            $errors[] = new PhabricatorApplicationTransactionValidationError(
-            pht(
+              $type,
-              '%s must be an integer value.',
+              pht('Invalid'),
-              $this->getLabel()));
+              pht('%s must be an integer value.', $this->getLabel()),
+              $xaction);
+            $this->setError(pht('Invalid'));
           }
           break;
-      case self::TYPE_BOOL:
-        return true;
-      case self::TYPE_STRING:
-        return true;
-      case self::TYPE_SELECT:
-        return true;
         case self::TYPE_DATE:
-        if ((int)$this->getValue() <= 0 && $this->isRequired()) {
+          if ((int)$value <= 0 && $this->isRequired()) {
-          throw new PhabricatorCustomFieldValidationException(
+            $errors[] = new PhabricatorApplicationTransactionValidationError(
-            pht(
+              $type,
-              '%s must be a valid date.',
+              pht('Invalid'),
-              $this->getLabel()));
+              pht('%s must be a valid date.', $this->getLabel()),
-        }
+              $xaction);
-        break;
+            $this->setError(pht('Invalid'));
-      case self::TYPE_USER:
-      case self::TYPE_USERS:
-        if (!is_array($this->getValue())) {
-          throw new PhabricatorCustomFieldValidationException(
-            pht(
-              '%s is not a valid list of user PHIDs.',
-              $this->getLabel()));
           }
           break;
       }
     }
 
+    if ($this->isRequired()) {
+      if (!$has_value) {
+        $value = $this->getOldValueForApplicationTransactions();
+      }
+      if (!$value) {
+        $errors[] = new PhabricatorApplicationTransactionValidationError(
+          $type,
+          pht('Required'),
+          pht('%s is required.', $this->getLabel()),
+          null);
+        $this->setError(pht('Required'));
+      }
+    }
+
+    return $errors;
+  }
+
   public function setDefaultValue($value) {
     switch ($this->getFieldType()) {
       case self::TYPE_DATE:

src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php

@@ -63,10 +63,6 @@ abstract class ManiphestAuxiliaryFieldSpecification
     return $this->value;
   }
 
-  public function validate() {
-    return true;
-  }
-
   public function isRequired() {
     return false;
   }
@@ -173,7 +169,11 @@ abstract class ManiphestAuxiliaryFieldSpecification
   }
 
   public function renderPropertyViewValue() {
-    return $this->renderForDetailView();
+    $value = $this->renderForDetailView();
+    if (!strlen($value)) {
+      return null;
+    }
+    return $value;
   }
 
   public function renderPropertyViewLabel() {

src/applications/maniphest/controller/ManiphestTaskEditController.php

@@ -142,22 +142,24 @@ final class ManiphestTaskEditController extends ManiphestController {
         $aux_field->readValueFromRequest($request);
         $aux_new_value = $aux_field->getNewValueForApplicationTransactions();
 
-        // TODO: What's going on here?
+        // TODO: We're faking a call to the ApplicaitonTransaction validation
-        if ((int)$aux_old_value === $aux_new_value) {
+        // logic here. We need valid objects to pass, but they aren't used
-          unset($aux_fields[$aux_arr_key]);
+        // in a meaningful way. For now, build User objects. Once the Maniphest
-          continue;
+        // objects exist, this will switch over automatically. This is a big
-        }
+        // hack but shouldn't be long for this world.
+        $placeholder_editor = new PhabricatorUserProfileEditor();
 
-        if ($aux_field->isRequired() && !$aux_field->getValue()) {
+        $field_errors = $aux_field->validateApplicationTransactions(
-          $errors[] = pht('%s is required.', $aux_field->getLabel());
+          $placeholder_editor,
-          $aux_field->setError(pht('Required'));
+          PhabricatorTransactions::TYPE_CUSTOMFIELD,
-        }
+          array(
+            id(new PhabricatorUserTransaction())
+              ->setOldValue($aux_old_value)
+              ->setNewValue($aux_new_value),
+          ));
 
-        try {
+        foreach ($field_errors as $error) {
-          $aux_field->validate();
+          $errors[] = $error->getMessage();
-        } catch (Exception $e) {
-          $errors[] = $e->getMessage();
-          $aux_field->setError(pht('Invalid'));
         }
 
         $old_values[$aux_field->getFieldKey()] = $aux_old_value;
@@ -490,12 +492,6 @@ final class ManiphestTaskEditController extends ManiphestController {
           ->setDatasource('/typeahead/common/projects/'));
 
     foreach ($aux_fields as $aux_field) {
-      if ($aux_field->isRequired() &&
-          !$aux_field->getError() &&
-          !$aux_field->getValue()) {
-        $aux_field->setError(true);
-      }
-
       $aux_control = $aux_field->renderEditControl();
       $form->appendChild($aux_control);
     }

src/applications/maniphest/field/ManiphestCustomField.php

@@ -36,14 +36,6 @@ abstract class ManiphestCustomField
   public function setHandles(array $handles) {
   }
 
-  public function isRequired() {
-    return false;
-  }
-
-  public function validate() {
-    return true;
-  }
-
   /**
    * Render a verb to appear in email titles when a transaction involving this
    * field occurs. Specifically, Maniphest emails are formatted like this:

src/infrastructure/customfield/exception/PhabricatorCustomFieldValidationException.php

@@ -1,6 +0,0 @@
-<?php
-
-final class PhabricatorCustomFieldValidationException
-  extends Exception {
-
-}

src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php

@@ -271,7 +271,7 @@ abstract class PhabricatorStandardCustomField
       $xactions);
 
     if ($this->getRequired()) {
-      $value = null;
+      $value = $this->getOldValueForApplicationTransactions();
       $transaction = null;
       foreach ($xactions as $xaction) {
         $value = $xaction->getNewValue();

src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldDate.php

@@ -63,7 +63,7 @@ final class PhabricatorStandardCustomFieldDate
       ->setName($this->getFieldKey())
       ->setUser($this->getViewer())
       ->setCaption($this->getCaption())
-      ->setAllowNull(true);
+      ->setAllowNull(!$this->getRequired());
 
     $control->setValue($this->getFieldValue());