Remove HeraldConditionConfig

Summary: Ref T2769. Moves all traces of HeraldConditionConfig into Adapters.

Test Plan: Edited rules and used Test Console to exercise both affected code paths. Tried to save invalid rules to hit error pat.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2769

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

src/__phutil_library_map__.php

@@ -600,7 +600,6 @@ phutil_register_library_map(array(
     'HeraldApplyTranscript' => 'applications/herald/storage/transcript/HeraldApplyTranscript.php',
     'HeraldCommitAdapter' => 'applications/herald/adapter/HeraldCommitAdapter.php',
     'HeraldCondition' => 'applications/herald/storage/HeraldCondition.php',
-    'HeraldConditionConfig' => 'applications/herald/config/HeraldConditionConfig.php',
     'HeraldConditionTranscript' => 'applications/herald/storage/transcript/HeraldConditionTranscript.php',
     'HeraldController' => 'applications/herald/controller/HeraldController.php',
     'HeraldDAO' => 'applications/herald/storage/HeraldDAO.php',

src/applications/herald/adapter/HeraldAdapter.php

@@ -54,7 +54,17 @@ abstract class HeraldAdapter {
 
   abstract public function getPHID();
   abstract public function getHeraldName();
-  abstract public function getHeraldField($field_name);
+
+  public function getHeraldField($field_name) {
+    switch ($field_name) {
+      case self::FIELD_RULE:
+        return null;
+      default:
+        throw new Exception(
+          "Unknown field '{$field_name}'!");
+    }
+  }
+
   abstract public function applyHeraldEffects(array $effects);
 
   public function isEnabled() {
@@ -176,6 +186,225 @@ abstract class HeraldAdapter {
     }
   }
 
+  public function doesConditionMatch(
+    HeraldEngine $engine,
+    HeraldRule $rule,
+    HeraldCondition $condition,
+    $field_value) {
+
+    $condition_type = $condition->getFieldCondition();
+    $condition_value = $condition->getValue();
+
+    switch ($condition_type) {
+      case self::CONDITION_CONTAINS:
+        // "Contains" can take an array of strings, as in "Any changed
+        // filename" for diffs.
+        foreach ((array)$field_value as $value) {
+          if (stripos($value, $condition_value) !== false) {
+            return true;
+          }
+        }
+        return false;
+      case self::CONDITION_NOT_CONTAINS:
+        return (stripos($field_value, $condition_value) === false);
+      case self::CONDITION_IS:
+        return ($field_value == $condition_value);
+      case self::CONDITION_IS_NOT:
+        return ($field_value != $condition_value);
+      case self::CONDITION_IS_ME:
+        return ($field_value == $rule->getAuthorPHID());
+      case self::CONDITION_IS_NOT_ME:
+        return ($field_value != $rule->getAuthorPHID());
+      case self::CONDITION_IS_ANY:
+        if (!is_array($condition_value)) {
+          throw new HeraldInvalidConditionException(
+            "Expected condition value to be an array.");
+        }
+        $condition_value = array_fuse($condition_value);
+        return isset($condition_value[$field_value]);
+      case self::CONDITION_IS_NOT_ANY:
+        if (!is_array($condition_value)) {
+          throw new HeraldInvalidConditionException(
+            "Expected condition value to be an array.");
+        }
+        $condition_value = array_fuse($condition_value);
+        return !isset($condition_value[$field_value]);
+      case self::CONDITION_INCLUDE_ALL:
+        if (!is_array($field_value)) {
+          throw new HeraldInvalidConditionException(
+            "Object produced non-array value!");
+        }
+        if (!is_array($condition_value)) {
+          throw new HeraldInvalidConditionException(
+            "Expected conditionv value to be an array.");
+        }
+
+        $have = array_select_keys(array_fuse($field_value), $condition_value);
+        return (count($have) == count($condition_value));
+      case self::CONDITION_INCLUDE_ANY:
+        return (bool)array_select_keys(
+          array_fuse($field_value),
+          $condition_value);
+      case self::CONDITION_INCLUDE_NONE:
+        return !array_select_keys(
+          array_fuse($field_value),
+          $condition_value);
+      case self::CONDITION_EXISTS:
+        return (bool)$field_value;
+      case self::CONDITION_NOT_EXISTS:
+        return !$field_value;
+      case self::CONDITION_REGEXP:
+        foreach ((array)$field_value as $value) {
+          // We add the 'S' flag because we use the regexp multiple times.
+          // It shouldn't cause any troubles if the flag is already there
+          // - /.*/S is evaluated same as /.*/SS.
+          $result = @preg_match($condition_value . 'S', $value);
+          if ($result === false) {
+            throw new HeraldInvalidConditionException(
+              "Regular expression is not valid!");
+          }
+          if ($result) {
+            return true;
+          }
+        }
+        return false;
+      case self::CONDITION_REGEXP_PAIR:
+        // Match a JSON-encoded pair of regular expressions against a
+        // dictionary. The first regexp must match the dictionary key, and the
+        // second regexp must match the dictionary value. If any key/value pair
+        // in the dictionary matches both regexps, the condition is satisfied.
+        $regexp_pair = json_decode($condition_value, true);
+        if (!is_array($regexp_pair)) {
+          throw new HeraldInvalidConditionException(
+            "Regular expression pair is not valid JSON!");
+        }
+        if (count($regexp_pair) != 2) {
+          throw new HeraldInvalidConditionException(
+            "Regular expression pair is not a pair!");
+        }
+
+        $key_regexp   = array_shift($regexp_pair);
+        $value_regexp = array_shift($regexp_pair);
+
+        foreach ((array)$field_value as $key => $value) {
+          $key_matches = @preg_match($key_regexp, $key);
+          if ($key_matches === false) {
+            throw new HeraldInvalidConditionException(
+              "First regular expression is invalid!");
+          }
+          if ($key_matches) {
+            $value_matches = @preg_match($value_regexp, $value);
+            if ($value_matches === false) {
+              throw new HeraldInvalidConditionException(
+                "Second regular expression is invalid!");
+            }
+            if ($value_matches) {
+              return true;
+            }
+          }
+        }
+        return false;
+      case self::CONDITION_RULE:
+      case self::CONDITION_NOT_RULE:
+        $rule = $engine->getRule($condition_value);
+        if (!$rule) {
+          throw new HeraldInvalidConditionException(
+            "Condition references a rule which does not exist!");
+        }
+
+        $is_not = ($condition_type == self::CONDITION_NOT_RULE);
+        $result = $engine->doesRuleMatch($rule, $this);
+        if ($is_not) {
+          $result = !$result;
+        }
+        return $result;
+      default:
+        throw new HeraldInvalidConditionException(
+          "Unknown condition '{$condition_type}'.");
+    }
+  }
+
+  public function willSaveCondition(HeraldCondition $condition) {
+    $condition_type = $condition->getFieldCondition();
+    $condition_value = $condition->getValue();
+
+    switch ($condition_type) {
+      case self::CONDITION_REGEXP:
+        $ok = @preg_match($condition_value, '');
+        if ($ok === false) {
+          throw new HeraldInvalidConditionException(
+            pht(
+              'The regular expression "%s" is not valid. Regular expressions '.
+              'must have enclosing characters (e.g. "@/path/to/file@", not '.
+              '"/path/to/file") and be syntactically correct.',
+              $condition_value));
+        }
+        break;
+      case self::CONDITION_REGEXP_PAIR:
+        $json = json_decode($condition_value, true);
+        if (!is_array($json)) {
+          throw new HeraldInvalidConditionException(
+            pht(
+              'The regular expression pair "%s" is not valid JSON. Enter a '.
+              'valid JSON array with two elements.',
+              $condition_value));
+        }
+
+        if (count($json) != 2) {
+          throw new HeraldInvalidConditionException(
+            pht(
+              'The regular expression pair "%s" must have exactly two '.
+              'elements.',
+              $condition_value));
+        }
+
+        $key_regexp = array_shift($json);
+        $val_regexp = array_shift($json);
+
+        $key_ok = @preg_match($key_regexp, '');
+        if ($key_ok === false) {
+          throw new HeraldInvalidConditionException(
+            pht(
+              'The first regexp in the regexp pair, "%s", is not a valid '.
+              'regexp.',
+              $key_regexp));
+        }
+
+        $val_ok = @preg_match($val_regexp, '');
+        if ($val_ok === false) {
+          throw new HeraldInvalidConditionException(
+            pht(
+              'The second regexp in the regexp pair, "%s", is not a valid '.
+              'regexp.',
+              $val_regexp));
+        }
+        break;
+      case self::CONDITION_CONTAINS:
+      case self::CONDITION_NOT_CONTAINS:
+      case self::CONDITION_IS:
+      case self::CONDITION_IS_NOT:
+      case self::CONDITION_IS_ANY:
+      case self::CONDITION_IS_NOT_ANY:
+      case self::CONDITION_INCLUDE_ALL:
+      case self::CONDITION_INCLUDE_ANY:
+      case self::CONDITION_INCLUDE_NONE:
+      case self::CONDITION_IS_ME:
+      case self::CONDITION_IS_NOT_ME:
+      case self::CONDITION_RULE:
+      case self::CONDITION_NOT_RULE:
+      case self::CONDITION_EXISTS:
+      case self::CONDITION_NOT_EXISTS:
+        // No explicit validation for these types, although there probably
+        // should be in some cases.
+        break;
+      default:
+        throw new HeraldInvalidConditionException(
+          pht(
+            'Unknown condition "%s"!',
+            $condition_type));
+    }
+  }
+
 
 /* -(  Actions  )------------------------------------------------------------ */
 

src/applications/herald/adapter/HeraldCommitAdapter.php

@@ -287,9 +287,9 @@ final class HeraldCommitAdapter extends HeraldAdapter {
           return array();
         }
         return $revision->getCCPHIDs();
-      default:
-        throw new Exception("Invalid field '{$field}'.");
     }
+
+    return parent::getHeraldField($field);
   }
 
   public function applyHeraldEffects(array $effects) {

src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php

@@ -239,9 +239,9 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
         $packages = $this->loadAffectedPackages();
         return PhabricatorOwnersOwner::loadAffiliatedUserPHIDs(
           mpull($packages, 'getID'));
-      default:
-        throw new Exception("Invalid field '{$field}'.");
     }
+
+    return parent::getHeraldField($field);
   }
 
   public function getActions($rule_type) {

src/applications/herald/config/HeraldConditionConfig.php

@@ -1,24 +0,0 @@
-<?php
-
-final class HeraldConditionConfig {
-
-  // TODO: Remove; still used by Engine/etc.
-  const CONDITION_CONTAINS        = 'contains';
-  const CONDITION_NOT_CONTAINS    = '!contains';
-  const CONDITION_IS              = 'is';
-  const CONDITION_IS_NOT          = '!is';
-  const CONDITION_IS_ANY          = 'isany';
-  const CONDITION_IS_NOT_ANY      = '!isany';
-  const CONDITION_INCLUDE_ALL     = 'all';
-  const CONDITION_INCLUDE_ANY     = 'any';
-  const CONDITION_INCLUDE_NONE    = 'none';
-  const CONDITION_IS_ME           = 'me';
-  const CONDITION_IS_NOT_ME       = '!me';
-  const CONDITION_REGEXP          = 'regexp';
-  const CONDITION_RULE            = 'conditions';
-  const CONDITION_NOT_RULE        = '!conditions';
-  const CONDITION_EXISTS          = 'exists';
-  const CONDITION_NOT_EXISTS      = '!exists';
-  const CONDITION_REGEXP_PAIR     = 'regexp-pair';
-
-}

src/applications/herald/controller/HeraldRuleController.php

@@ -68,7 +68,7 @@ final class HeraldRuleController extends HeraldController {
     $e_name = true;
     $errors = array();
     if ($request->isFormPost() && $request->getStr('save')) {
-      list($e_name, $errors) = $this->saveRule($rule, $request);
+      list($e_name, $errors) = $this->saveRule($adapter, $rule, $request);
       if (!$errors) {
         $uri = '/herald/view/'.
           $rule->getContentType().'/'.
@@ -198,7 +198,7 @@ final class HeraldRuleController extends HeraldController {
       ));
   }
 
-  private function saveRule($rule, $request) {
+  private function saveRule(HeraldAdapter $adapter, $rule, $request) {
     $rule->setName($request->getStr('name'));
     $rule->setMustMatchAll(($request->getStr('must_match') == 'all'));
 
@@ -239,46 +239,10 @@ final class HeraldRuleController extends HeraldController {
         $obj->setValue($condition[2]);
       }
 
-      $cond_type = $obj->getFieldCondition();
+      try {
-
+        $adapter->willSaveCondition($obj);
-      if ($cond_type == HeraldConditionConfig::CONDITION_REGEXP) {
+      } catch (HeraldInvalidConditionException $ex) {
-        if (@preg_match($obj->getValue(), '') === false) {
+        $errors[] = $ex->getMessage();
-          $errors[] =
-            pht('The regular expression "%s" is not valid. '.
-            'Regular expressions must have enclosing characters (e.g. '.
-            '"@/path/to/file@", not "/path/to/file") and be syntactically '.
-            'correct.', $obj->getValue());
-        }
-      }
-
-      if ($cond_type == HeraldConditionConfig::CONDITION_REGEXP_PAIR) {
-        $json = json_decode($obj->getValue(), true);
-        if (!is_array($json)) {
-          $errors[] =
-            pht('The regular expression pair "%s" is not '.
-            'valid JSON. Enter a valid JSON array with two elements.',
-            $obj->getValue());
-        } else {
-          if (count($json) != 2) {
-            $errors[] =
-              pht('The regular expression pair "%s" must have '.
-              'exactly two elements.', $obj->getValue());
-          } else {
-            $key_regexp = array_shift($json);
-            $val_regexp = array_shift($json);
-
-            if (@preg_match($key_regexp, '') === false) {
-              $errors[] =
-                pht('The first regexp, "%s" in the regexp pair '.
-                'is not a valid regexp.', $key_regexp);
-            }
-            if (@preg_match($val_regexp, '') === false) {
-              $errors[] =
-                pht('The second regexp, "%s" in the regexp pair '.
-                'is not a valid regexp.', $val_regexp);
-            }
-          }
-        }
       }
 
       $conditions[] = $obj;

src/applications/herald/engine/HeraldEngine.php

@@ -10,6 +10,10 @@ final class HeraldEngine {
   protected $fieldCache = array();
   protected $object = null;
 
+  public function getRule($id) {
+    return idx($this->rules, $id);
+  }
+
   public static function loadAndApplyRules(HeraldAdapter $object) {
     $content_type = $object->getAdapterContentType();
     $rules = HeraldRule::loadAllByContentTypeWithFullData(
@@ -173,7 +177,7 @@ final class HeraldEngine {
     return $this->transcript;
   }
 
-  protected function doesRuleMatch(
+  public function doesRuleMatch(
     HeraldRule $rule,
     HeraldAdapter $object) {
 
@@ -272,143 +276,15 @@ final class HeraldEngine {
     $transcript->setCondition($cond);
     $transcript->setTestValue($test_value);
 
-    $result = null;
+    try {
-    switch ($cond) {
+      $result = $object->doesConditionMatch(
-      case HeraldConditionConfig::CONDITION_CONTAINS:
+        $this,
-        // "Contains" can take an array of strings, as in "Any changed
+        $rule,
-        // filename" for diffs.
+        $condition,
-        foreach ((array)$object_value as $value) {
+        $object_value);
-          $result = (stripos($value, $test_value) !== false);
+    } catch (HeraldInvalidConditionException $ex) {
-          if ($result) {
+      $result = false;
-            break;
+      $transcript->setNote($ex->getMessage());
-          }
-        }
-        break;
-      case HeraldConditionConfig::CONDITION_NOT_CONTAINS:
-        $result = (stripos($object_value, $test_value) === false);
-        break;
-      case HeraldConditionConfig::CONDITION_IS:
-        $result = ($object_value == $test_value);
-        break;
-      case HeraldConditionConfig::CONDITION_IS_NOT:
-        $result = ($object_value != $test_value);
-        break;
-      case HeraldConditionConfig::CONDITION_IS_ME:
-        $result = ($object_value == $rule->getAuthorPHID());
-        break;
-      case HeraldConditionConfig::CONDITION_IS_NOT_ME:
-        $result = ($object_value != $rule->getAuthorPHID());
-        break;
-      case HeraldConditionConfig::CONDITION_IS_ANY:
-        $test_value = array_flip($test_value);
-        $result = isset($test_value[$object_value]);
-        break;
-      case HeraldConditionConfig::CONDITION_IS_NOT_ANY:
-        $test_value = array_flip($test_value);
-        $result = !isset($test_value[$object_value]);
-        break;
-      case HeraldConditionConfig::CONDITION_INCLUDE_ALL:
-        if (!is_array($object_value)) {
-          $transcript->setNote('Object produced bad value!');
-          $result = false;
-        } else {
-          $have = array_select_keys(array_flip($object_value),
-                                    $test_value);
-          $result = (count($have) == count($test_value));
-        }
-        break;
-      case HeraldConditionConfig::CONDITION_INCLUDE_ANY:
-        $result = (bool)array_select_keys(array_flip($object_value),
-                                          $test_value);
-        break;
-      case HeraldConditionConfig::CONDITION_INCLUDE_NONE:
-        $result = !array_select_keys(array_flip($object_value),
-                                     $test_value);
-        break;
-      case HeraldConditionConfig::CONDITION_EXISTS:
-        $result = (bool)$object_value;
-        break;
-      case HeraldConditionConfig::CONDITION_NOT_EXISTS:
-        $result = !$object_value;
-        break;
-      case HeraldConditionConfig::CONDITION_REGEXP:
-        foreach ((array)$object_value as $value) {
-          // We add the 'S' flag because we use the regexp multiple times.
-          // It shouldn't cause any troubles if the flag is already there
-          // - /.*/S is evaluated same as /.*/SS.
-          $result = @preg_match($test_value . 'S', $value);
-          if ($result === false) {
-            $transcript->setNote(
-              "Regular expression is not valid!");
-            break;
-          }
-          if ($result) {
-            break;
-          }
-        }
-        $result = (bool)$result;
-        break;
-      case HeraldConditionConfig::CONDITION_REGEXP_PAIR:
-        // Match a JSON-encoded pair of regular expressions against a
-        // dictionary. The first regexp must match the dictionary key, and the
-        // second regexp must match the dictionary value. If any key/value pair
-        // in the dictionary matches both regexps, the condition is satisfied.
-        $regexp_pair = json_decode($test_value, true);
-        if (!is_array($regexp_pair)) {
-          $result = false;
-          $transcript->setNote("Regular expression pair is not valid JSON!");
-          break;
-        }
-        if (count($regexp_pair) != 2) {
-          $result = false;
-          $transcript->setNote("Regular expression pair is not a pair!");
-          break;
-        }
-
-        $key_regexp   = array_shift($regexp_pair);
-        $value_regexp = array_shift($regexp_pair);
-
-        foreach ((array)$object_value as $key => $value) {
-          $key_matches = @preg_match($key_regexp, $key);
-          if ($key_matches === false) {
-            $result = false;
-            $transcript->setNote("First regular expression is invalid!");
-            break 2;
-          }
-          if ($key_matches) {
-            $value_matches = @preg_match($value_regexp, $value);
-            if ($value_matches === false) {
-              $result = false;
-              $transcript->setNote("Second regular expression is invalid!");
-              break 2;
-            }
-            if ($value_matches) {
-              $result = true;
-              break 2;
-            }
-          }
-        }
-        $result = false;
-        break;
-      case HeraldConditionConfig::CONDITION_RULE:
-      case HeraldConditionConfig::CONDITION_NOT_RULE:
-
-        $rule = idx($this->rules, $test_value);
-        if (!$rule) {
-          $transcript->setNote(
-            "Condition references a rule which does not exist!");
-          $result = false;
-        } else {
-          $is_not = ($cond == HeraldConditionConfig::CONDITION_NOT_RULE);
-          $result = $this->doesRuleMatch($rule, $object);
-          if ($is_not) {
-            $result = !$result;
-          }
-        }
-        break;
-      default:
-        throw new HeraldInvalidConditionException(
-          "Unknown condition '{$cond}'.");
     }
 
     $transcript->setResult($result);
@@ -432,49 +308,7 @@ final class HeraldEngine {
       return $this->fieldCache[$field];
     }
 
-    $result = null;
+    $result = $this->object->getHeraldField($field);
-    switch ($field) {
-      case HeraldFieldConfig::FIELD_RULE:
-        $result = null;
-        break;
-      case HeraldFieldConfig::FIELD_TITLE:
-      case HeraldFieldConfig::FIELD_BODY:
-      case HeraldFieldConfig::FIELD_DIFF_FILE:
-      case HeraldFieldConfig::FIELD_DIFF_CONTENT:
-        // TODO: Type should be string.
-        $result = $this->object->getHeraldField($field);
-        break;
-      case HeraldFieldConfig::FIELD_AUTHOR:
-      case HeraldFieldConfig::FIELD_REPOSITORY:
-        // TODO: Type should be PHID.
-        $result = $this->object->getHeraldField($field);
-        break;
-      case HeraldFieldConfig::FIELD_TAGS:
-      case HeraldFieldConfig::FIELD_REVIEWER:
-      case HeraldFieldConfig::FIELD_REVIEWERS:
-      case HeraldFieldConfig::FIELD_CC:
-      case HeraldFieldConfig::FIELD_DIFFERENTIAL_REVIEWERS:
-      case HeraldFieldConfig::FIELD_DIFFERENTIAL_CCS:
-        // TODO: Type should be list.
-        $result = $this->object->getHeraldField($field);
-        break;
-      case HeraldFieldConfig::FIELD_AFFECTED_PACKAGE:
-      case HeraldFieldConfig::FIELD_AFFECTED_PACKAGE_OWNER:
-      case HeraldFieldConfig::FIELD_NEED_AUDIT_FOR_PACKAGE:
-        $result = $this->object->getHeraldField($field);
-        if (!is_array($result)) {
-          throw new HeraldInvalidFieldException(
-            "Value of field type {$field} is not an array!");
-        }
-        break;
-      case HeraldFieldConfig::FIELD_DIFFERENTIAL_REVISION:
-        // TODO: Type should be boolean I guess.
-        $result = $this->object->getHeraldField($field);
-        break;
-      default:
-        throw new HeraldInvalidConditionException(
-          "Unknown field type '{$field}'!");
-    }
 
     $this->fieldCache[$field] = $result;
     return $result;