Separate commit message parsing and validation from Conduit

Summary:
Ref T11114. I want to move this step away from custom fields. To start with, isolate all the parsing in one class with a clearer API boundary.

Next, I'll make this class use new field objects to perform parsing, without CustomField interactions.

Test Plan: Created and edited revisions from the CLI, using valid and invalid commit messages.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

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

src/applications/differential/conduit/DifferentialParseCommitMessageConduitAPIMethod.php

@@ -3,8 +3,6 @@
 final class DifferentialParseCommitMessageConduitAPIMethod
   extends DifferentialConduitAPIMethod {
 
-  private $errors;
-
   public function getAPIMethodName() {
     return 'differential.parsecommitmessage';
   }
@@ -25,63 +23,30 @@ final class DifferentialParseCommitMessageConduitAPIMethod
   }
 
   protected function execute(ConduitAPIRequest $request) {
-    $viewer = $request->getUser();
-    $corpus = $request->getValue('corpus');
+    $viewer = $this->getViewer();
+
+    $parser = DifferentialCommitMessageParser::newStandardParser($viewer);
+
     $is_partial = $request->getValue('partial');
-
-    $field_list = PhabricatorCustomField::getObjectFields(
-      new DifferentialRevision(),
-      DifferentialCustomField::ROLE_COMMITMESSAGE);
-    $field_list->setViewer($viewer);
-    $field_map = mpull($field_list->getFields(), null, 'getFieldKeyForConduit');
-
-    $corpus_map = $this->parseCommitMessage($corpus);
-
-    $values = array();
-    foreach ($corpus_map as $field_key => $text_value) {
-      $field = idx($field_map, $field_key);
-
-      if (!$field) {
-        throw new Exception(
-          pht(
-            'Parser emitted text value for field key "%s", but no such '.
-            'field exists.',
-            $field_key));
-      }
-
-      try {
-        $values[$field_key] = $field->parseValueFromCommitMessage($text_value);
-      } catch (DifferentialFieldParseException $ex) {
-        $this->errors[] = pht(
-          'Error parsing field "%s": %s',
-          $field->renderCommitMessageLabel(),
-          $ex->getMessage());
-      }
+    if ($is_partial) {
+      $parser->setRaiseMissingFieldErrors(false);
     }
 
-    if (!$is_partial) {
-      foreach ($field_map as $key => $field) {
-        try {
-          $field->validateCommitMessageValue(idx($values, $key));
-        } catch (DifferentialFieldValidationException $ex) {
-          $this->errors[] = pht(
-            'Invalid or missing field "%s": %s',
-            $field->renderCommitMessageLabel(),
-            $ex->getMessage());
-        }
-      }
-    }
+    $corpus = $request->getValue('corpus');
+    $field_map = $parser->parseFields($corpus);
+
+    $errors = $parser->getErrors();
 
     // grab some extra information about the Differential Revision: field...
     $revision_id_field = new DifferentialRevisionIDField();
     $revision_id_value = idx(
-      $corpus_map,
+      $field_map,
       $revision_id_field->getFieldKeyForConduit());
     $revision_id_valid_domain = PhabricatorEnv::getProductionURI('');
 
     return array(
-      'errors' => $this->errors,
-      'fields' => $values,
+      'errors' => $errors,
+      'fields' => $field_map,
       'revisionIDFieldInfo' => array(
         'value' => $revision_id_value,
         'validDomain' => $revision_id_valid_domain,
@@ -89,17 +54,4 @@ final class DifferentialParseCommitMessageConduitAPIMethod
     );
   }
 
-  private function parseCommitMessage($corpus) {
-    $viewer = $this->getViewer();
-    $parser = DifferentialCommitMessageParser::newStandardParser($viewer);
-    $result = $parser->parseCorpus($corpus);
-
-    $this->errors = array();
-    foreach ($parser->getErrors() as $error) {
-      $this->errors[] = $error;
-    }
-
-    return $result;
-  }
-
 }

src/applications/differential/parser/DifferentialCommitMessageParser.php

@@ -21,11 +21,12 @@
  */
 final class DifferentialCommitMessageParser extends Phobject {
 
+  private $viewer;
   private $labelMap;
   private $titleKey;
   private $summaryKey;
   private $errors;
-
+  private $raiseMissingFieldErrors = true;
 
   public static function newStandardParser(PhabricatorUser $viewer) {
 
@@ -60,6 +61,7 @@ final class DifferentialCommitMessageParser extends Phobject {
     }
 
     return id(new self())
+      ->setViewer($viewer)
       ->setLabelMap($label_map)
       ->setTitleKey($key_title)
       ->setSummaryKey($key_summary);
@@ -69,6 +71,40 @@ final class DifferentialCommitMessageParser extends Phobject {
 /* -(  Configuring the Parser  )--------------------------------------------- */
 
 
+  /**
+   * @task config
+   */
+  public function setViewer(PhabricatorUser $viewer) {
+    $this->viewer = $viewer;
+    return $this;
+  }
+
+
+  /**
+   * @task config
+   */
+  public function getViewer() {
+    return $this->viewer;
+  }
+
+
+  /**
+   * @task config
+   */
+  public function setRaiseMissingFieldErrors($raise) {
+    $this->raiseMissingFieldErrors = $raise;
+    return $this;
+  }
+
+
+  /**
+   * @task config
+   */
+  public function getRaiseMissingFieldErrors() {
+    return $this->raiseMissingFieldErrors;
+  }
+
+
   /**
    * @task config
    */
@@ -215,6 +251,63 @@ final class DifferentialCommitMessageParser extends Phobject {
   }
 
 
+  /**
+   * @task parse
+   */
+  public function parseFields($corpus) {
+    $viewer = $this->getViewer();
+    $text_map = $this->parseCorpus($corpus);
+
+    $field_list = PhabricatorCustomField::getObjectFields(
+      new DifferentialRevision(),
+      DifferentialCustomField::ROLE_COMMITMESSAGE);
+    $field_list->setViewer($viewer);
+
+    $field_map = $field_list->getFields();
+    $field_map = mpull($field_map, null, 'getFieldKeyForConduit');
+
+    $result_map = array();
+    foreach ($text_map as $field_key => $text_value) {
+      $field = idx($field_map, $field_key);
+      if (!$field) {
+        // This is a strict error, since we only parse fields which we have
+        // been told are valid. The caller probably handed us an invalid label
+        // map.
+        throw new Exception(
+          pht(
+            'Parser emitted a field with key "%s", but no corresponding '.
+            'field definition exists.',
+            $field_key));
+      }
+
+      try {
+        $result = $field->parseValueFromCommitMessage($text_value);
+        $result_map[$field_key] = $result;
+      } catch (DifferentialFieldParseException $ex) {
+        $this->errors[] = pht(
+          'Error parsing field "%s": %s',
+          $field->renderCommitMessageLabel(),
+          $ex->getMessage());
+      }
+    }
+
+    if ($this->getRaiseMissingFieldErrors()) {
+      foreach ($field_map as $key => $field) {
+        try {
+          $field->validateCommitMessageValue(idx($result_map, $key));
+        } catch (DifferentialFieldValidationException $ex) {
+          $this->errors[] = pht(
+            'Invalid or missing field "%s": %s',
+            $field->renderCommitMessageLabel(),
+            $ex->getMessage());
+        }
+      }
+    }
+
+    return $result_map;
+  }
+
+
   /**
    * @task parse
    */