Allow custom fields to have validation logic

Summary:
Ref T418. This is fairly messy, but basically:

  - Add a validation phase to TransactionEditor.
  - Add a validation phase to CustomField.
  - Bring it to StandardField.
  - Add validation logic for the int field.
  - Provide support in related classes.

Test Plan: See screenshot.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T418

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

src/__phutil_library_map__.php

@@ -868,6 +868,8 @@ phutil_register_library_map(array(
     'PhabricatorApplicationTransactionQuery' => 'applications/transactions/query/PhabricatorApplicationTransactionQuery.php',
     'PhabricatorApplicationTransactionResponse' => 'applications/transactions/response/PhabricatorApplicationTransactionResponse.php',
     'PhabricatorApplicationTransactionTextDiffDetailView' => 'applications/transactions/view/PhabricatorApplicationTransactionTextDiffDetailView.php',
+    'PhabricatorApplicationTransactionValidationError' => 'applications/transactions/error/PhabricatorApplicationTransactionValidationError.php',
+    'PhabricatorApplicationTransactionValidationException' => 'applications/transactions/exception/PhabricatorApplicationTransactionValidationException.php',
     'PhabricatorApplicationTransactionView' => 'applications/transactions/view/PhabricatorApplicationTransactionView.php',
     'PhabricatorApplicationTransactions' => 'applications/transactions/application/PhabricatorApplicationTransactions.php',
     'PhabricatorApplicationUIExamples' => 'applications/uiexample/application/PhabricatorApplicationUIExamples.php',
@@ -2960,6 +2962,8 @@ phutil_register_library_map(array(
     'PhabricatorApplicationTransactionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorApplicationTransactionResponse' => 'AphrontProxyResponse',
     'PhabricatorApplicationTransactionTextDiffDetailView' => 'AphrontView',
+    'PhabricatorApplicationTransactionValidationError' => 'Phobject',
+    'PhabricatorApplicationTransactionValidationException' => 'Exception',
     'PhabricatorApplicationTransactionView' => 'AphrontView',
     'PhabricatorApplicationTransactions' => 'PhabricatorApplication',
     'PhabricatorApplicationUIExamples' => 'PhabricatorApplication',

src/applications/people/controller/PhabricatorPeopleProfileEditController.php

@@ -39,6 +39,7 @@ final class PhabricatorPeopleProfileEditController
       ->setViewer($user)
       ->readFieldsFromStorage($user);
 
+    $validation_exception = null;
     if ($request->isFormPost()) {
       $xactions = $field_list->buildFieldTransactionsFromRequest(
         new PhabricatorUserTransaction(),
@@ -50,9 +51,12 @@ final class PhabricatorPeopleProfileEditController
           PhabricatorContentSource::newFromRequest($request))
         ->setContinueOnNoEffect(true);
 
+      try {
         $editor->applyTransactions($user, $xactions);
-
         return id(new AphrontRedirectResponse())->setURI($profile_uri);
+      } catch (PhabricatorApplicationTransactionValidationException $ex) {
+        $validation_exception = $ex;
+      }
     }
 
     $title = pht('Edit Profile');
@@ -78,6 +82,7 @@ final class PhabricatorPeopleProfileEditController
 
     $form_box = id(new PHUIFormBoxView())
       ->setHeaderText(pht('Edit Your Profile'))
+      ->setValidationException($validation_exception)
       ->setForm($form);
 
     return $this->buildApplicationPage(

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -185,7 +185,6 @@ abstract class PhabricatorApplicationTransactionEditor
     PhabricatorLiskDAO $object,
     array $xactions) {
     return false;
-
   }
 
   protected function applyInitialEffects(
@@ -356,6 +355,18 @@ abstract class PhabricatorApplicationTransactionEditor
     $transaction_open = false;
 
     if (!$is_preview) {
+      $errors = array();
+      $type_map = mgroup($xactions, 'getTransactionType');
+      foreach ($this->getTransactionTypes() as $type) {
+        $type_xactions = idx($type_map, $type, array());
+        $errors[] = $this->validateTransaction($object, $type, $type_xactions);
+      }
+
+      $errors = array_mergev($errors);
+      if ($errors) {
+        throw new PhabricatorApplicationTransactionValidationException($errors);
+      }
+
       if ($object->getID()) {
         foreach ($xactions as $xaction) {
 
@@ -1003,6 +1014,55 @@ abstract class PhabricatorApplicationTransactionEditor
   }
 
 
+  /**
+   * Hook for validating transactions. This callback will be invoked for each
+   * available transaction type, even if an edit does not apply any transactions
+   * of that type. This allows you to raise exceptions when required fields are
+   * missing, by detecting that the object has no field value and there is no
+   * transaction which sets one.
+   *
+   * @param PhabricatorLiskDAO Object being edited.
+   * @param string Transaction type to validate.
+   * @param list<PhabricatorApplicationTransaction> Transactions of given type,
+   *   which may be empty if the edit does not apply any transactions of the
+   *   given type.
+   * @return list<PhabricatorApplicationTransactionValidationError> List of
+   *   validation errors.
+   */
+  protected function validateTransaction(
+    PhabricatorLiskDAO $object,
+    $type,
+    array $xactions) {
+
+    $errors = array();
+    switch ($type) {
+      case PhabricatorTransactions::TYPE_CUSTOMFIELD:
+        $groups = array();
+        foreach ($xactions as $xaction) {
+          $groups[$xaction->getMetadataValue('customfield:key')][] = $xaction;
+        }
+
+        $field_list = PhabricatorCustomField::getObjectFields(
+          $object,
+          PhabricatorCustomField::ROLE_EDIT);
+
+        $role_xactions = PhabricatorCustomField::ROLE_APPLICATIONTRANSACTIONS;
+        foreach ($field_list->getFields() as $field) {
+          if (!$field->shouldEnableForRole($role_xactions)) {
+            continue;
+          }
+          $errors[] = $field->validateApplicationTransactions(
+            $this,
+            $type,
+            idx($groups, $field->getFieldKey(), array()));
+        }
+        break;
+    }
+
+    return array_mergev($errors);
+  }
+
+
 /* -(  Implicit CCs  )------------------------------------------------------- */
 
 

src/applications/transactions/error/PhabricatorApplicationTransactionValidationError.php

@@ -0,0 +1,39 @@
+<?php
+
+final class PhabricatorApplicationTransactionValidationError
+  extends Phobject {
+
+  private $type;
+  private $transaction;
+  private $shortMessage;
+  private $message;
+
+  public function __construct(
+    $type,
+    $short_message,
+    $message,
+    PhabricatorApplicationTransaction $xaction = null) {
+
+    $this->type = $type;
+    $this->shortMessage = $short_message;
+    $this->message = $message;
+    $this->transaction = $xaction;
+  }
+
+  public function getType() {
+    return $this->type;
+  }
+
+  public function getTransaction() {
+    return $this->tranaction;
+  }
+
+  public function getShortMessage() {
+    return $this->shortMessage;
+  }
+
+  public function getMessage() {
+    return $this->message;
+  }
+
+}

src/applications/transactions/exception/PhabricatorApplicationTransactionValidationException.php

@@ -0,0 +1,43 @@
+<?php
+
+final class PhabricatorApplicationTransactionValidationException
+  extends Exception {
+
+  private $errors;
+
+  public function __construct(array $errors) {
+    assert_instances_of(
+      $errors,
+      'PhabricatorApplicationTransactionValidationError');
+
+    $this->errors = $errors;
+
+    $message = array();
+    $message[] = 'Validation errors:';
+    foreach ($this->errors as $error) {
+      $message[] = '  - '.$error->getMessage();
+    }
+
+    parent::__construct(implode("\n", $message));
+  }
+
+  public function getErrors() {
+    return $this->errors;
+  }
+
+  public function getErrorMessages() {
+    return mpull($this->errors, 'getMessage');
+  }
+
+  public function getShortMessage($type) {
+    foreach ($this->errors as $error) {
+      if ($error->getType() === $type) {
+        if ($error->getShortMessage() !== null) {
+          return $error->getShortMessage();
+        }
+      }
+    }
+    return null;
+  }
+
+}

src/infrastructure/customfield/field/PhabricatorCustomField.php

@@ -844,6 +844,36 @@ abstract class PhabricatorCustomField {
   }
 
 
+  /**
+   * Validate transactions for an object. This allows you to raise an error
+   * when a transaction would set a field to an invalid value, or when a field
+   * is required but no transactions provide value.
+   *
+   * @param PhabricatorLiskDAO Editor applying the transactions.
+   * @param string Transaction type. This type is always
+   *   `PhabricatorTransactions::TYPE_CUSTOMFIELD`, it is provided for
+   *   convenience when constructing exceptions.
+   * @param list<PhabricatorApplicationTransaction> Transactions being applied,
+   *   which may be empty if this field is not being edited.
+   * @return list<PhabricatorApplicationTransactionValidationError> Validation
+   *   errors.
+   *
+   * @task appxaction
+   */
+  public function validateApplicationTransactions(
+    PhabricatorApplicationTransactionEditor $editor,
+    $type,
+    array $xactions) {
+    if ($this->proxy) {
+      return $this->proxy->validateApplicationTransactions(
+        $editor,
+        $type,
+        $xactions);
+    }
+    return array();
+  }
+
+
 /* -(  Edit View  )---------------------------------------------------------- */
 
 

src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php

@@ -11,6 +11,7 @@ abstract class PhabricatorStandardCustomField
   private $applicationField;
   private $strings;
   private $caption;
+  private $fieldError;
 
   abstract public function getFieldType();
 
@@ -114,6 +115,14 @@ abstract class PhabricatorStandardCustomField
     return idx($this->fieldConfig, $key, $default);
   }
 
+  public function setFieldError($field_error) {
+    $this->fieldError = $field_error;
+    return $this;
+  }
+
+  public function getFieldError() {
+    return $this->fieldError;
+  }
 
 
 /* -(  PhabricatorCustomField  )--------------------------------------------- */
@@ -178,6 +187,7 @@ abstract class PhabricatorStandardCustomField
       ->setName($this->getFieldKey())
       ->setCaption($this->getCaption())
       ->setValue($this->getFieldValue())
+      ->setError($this->getFieldError())
       ->setLabel($this->getFieldName());
   }
 
@@ -230,4 +240,19 @@ abstract class PhabricatorStandardCustomField
     return;
   }
 
+  public function validateApplicationTransactions(
+    PhabricatorApplicationTransactionEditor $editor,
+    $type,
+    array $xactions) {
+
+    $this->setFieldError(null);
+
+    $errors = parent::validateApplicationTransactions(
+      $editor,
+      $type,
+      $xactions);
+
+    return $errors;
+  }
+
 }

src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php

@@ -21,7 +21,7 @@ final class PhabricatorStandardCustomFieldInt
   public function getValueForStorage() {
     $value = $this->getFieldValue();
     if (strlen($value)) {
-      return (int)$value;
+      return $value;
     } else {
       return null;
     }
@@ -68,4 +68,31 @@ final class PhabricatorStandardCustomFieldInt
         ->setValue($value));
   }
 
+  public function validateApplicationTransactions(
+    PhabricatorApplicationTransactionEditor $editor,
+    $type,
+    array $xactions) {
+
+    $errors = parent::validateApplicationTransactions(
+      $editor,
+      $type,
+      $xactions);
+
+    foreach ($xactions as $xaction) {
+      $value = $xaction->getNewValue();
+      if (strlen($value)) {
+        if (!preg_match('/^-?\d+/', $value)) {
+          $errors[] = new PhabricatorApplicationTransactionValidationError(
+            $type,
+            pht('Invalid'),
+            pht('%s must be an integer.', $this->getFieldName()),
+            $xaction);
+          $this->setFieldError(pht('Invalid'));
+        }
+      }
+    }
+
+    return $errors;
+  }
+
 }

src/view/form/PHUIFormBoxView.php

@@ -5,6 +5,7 @@ final class PHUIFormBoxView extends AphrontView {
   private $headerText;
   private $formError = null;
   private $form;
+  private $validationException;
 
   public function setHeaderText($text) {
     $this->headerText = $text;
@@ -21,16 +22,39 @@ final class PHUIFormBoxView extends AphrontView {
     return $this;
   }
 
-  public function render() {
+  public function setValidationException(
+    PhabricatorApplicationTransactionValidationException $ex = null) {
+    $this->validationException = $ex;
+    return $this;
+  }
 
-    $error = $this->formError ? $this->formError : null;
+  public function render() {
 
     $header = id(new PhabricatorActionHeaderView())
       ->setHeaderTitle($this->headerText)
       ->setHeaderColor(PhabricatorActionHeaderView::HEADER_LIGHTBLUE);
 
+    $ex = $this->validationException;
+    $exception_errors = null;
+    if ($ex) {
+      $messages = array();
+      foreach ($ex->getErrors() as $error) {
+        $messages[] = $error->getMessage();
+      }
+      if ($messages) {
+        $exception_errors = id(new AphrontErrorView())
+          ->setErrors($messages);
+      }
+    }
+
     $content = id(new PHUIBoxView())
-      ->appendChild(array($header, $error, $this->form))
+      ->appendChild(
+        array(
+          $header,
+          $this->formError,
+          $exception_errors,
+          $this->form,
+        ))
       ->setBorder(true)
       ->addMargin(PHUI::MARGIN_LARGE_TOP)
       ->addMargin(PHUI::MARGIN_LARGE_LEFT)