diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index d60b71d912..4a0e093071 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 412af8016a..17e1e424bd 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/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 )------------------------------------------------------------ */ diff --git a/src/applications/herald/adapter/HeraldCommitAdapter.php b/src/applications/herald/adapter/HeraldCommitAdapter.php index eb6a866f24..608a1f3cb6 100644 --- a/src/applications/herald/adapter/HeraldCommitAdapter.php +++ b/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) { diff --git a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php index cb78a09b35..63f14ab727 100644 --- a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php +++ b/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) { diff --git a/src/applications/herald/config/HeraldConditionConfig.php b/src/applications/herald/config/HeraldConditionConfig.php deleted file mode 100644 index e5ae6e4c94..0000000000 --- a/src/applications/herald/config/HeraldConditionConfig.php +++ /dev/null @@ -1,24 +0,0 @@ -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(); - - if ($cond_type == HeraldConditionConfig::CONDITION_REGEXP) { - if (@preg_match($obj->getValue(), '') === false) { - $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); - } - } - } + try { + $adapter->willSaveCondition($obj); + } catch (HeraldInvalidConditionException $ex) { + $errors[] = $ex->getMessage(); } $conditions[] = $obj; diff --git a/src/applications/herald/engine/HeraldEngine.php b/src/applications/herald/engine/HeraldEngine.php index c585690cd3..b92371ccff 100644 --- a/src/applications/herald/engine/HeraldEngine.php +++ b/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; - switch ($cond) { - case HeraldConditionConfig::CONDITION_CONTAINS: - // "Contains" can take an array of strings, as in "Any changed - // filename" for diffs. - foreach ((array)$object_value as $value) { - $result = (stripos($value, $test_value) !== false); - if ($result) { - break; - } - } - 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}'."); + try { + $result = $object->doesConditionMatch( + $this, + $rule, + $condition, + $object_value); + } catch (HeraldInvalidConditionException $ex) { + $result = false; + $transcript->setNote($ex->getMessage()); } $transcript->setResult($result); @@ -432,49 +308,7 @@ final class HeraldEngine { return $this->fieldCache[$field]; } - $result = null; - 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}'!"); - } + $result = $this->object->getHeraldField($field); $this->fieldCache[$field] = $result; return $result;