diff --git a/resources/celerity/map.php b/resources/celerity/map.php --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -78,7 +78,7 @@ 'rsrc/css/application/files/global-drag-and-drop.css' => '1d2713a4', 'rsrc/css/application/flag/flag.css' => '2b77be8d', 'rsrc/css/application/harbormaster/harbormaster.css' => '8dfe16b2', - 'rsrc/css/application/herald/herald-test.css' => 'e004176f', + 'rsrc/css/application/herald/herald-test.css' => '7e7bbdae', 'rsrc/css/application/herald/herald.css' => '648d39e2', 'rsrc/css/application/maniphest/report.css' => '3d53188b', 'rsrc/css/application/maniphest/task-edit.css' => '272daa84', @@ -587,7 +587,7 @@ 'harbormaster-css' => '8dfe16b2', 'herald-css' => '648d39e2', 'herald-rule-editor' => '2633bef7', - 'herald-test-css' => 'e004176f', + 'herald-test-css' => '7e7bbdae', 'inline-comment-summary-css' => '81eb368d', 'javelin-aphlict' => '022516b4', 'javelin-behavior' => '1b6acc2a', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1566,6 +1566,7 @@ 'HeraldCommentContentField' => 'applications/herald/field/HeraldCommentContentField.php', 'HeraldCommitAdapter' => 'applications/diffusion/herald/HeraldCommitAdapter.php', 'HeraldCondition' => 'applications/herald/storage/HeraldCondition.php', + 'HeraldConditionResult' => 'applications/herald/storage/transcript/HeraldConditionResult.php', 'HeraldConditionTranscript' => 'applications/herald/storage/transcript/HeraldConditionTranscript.php', 'HeraldContentSourceField' => 'applications/herald/field/HeraldContentSourceField.php', 'HeraldController' => 'applications/herald/controller/HeraldController.php', @@ -7793,6 +7794,7 @@ 'HarbormasterBuildableAdapterInterface', ), 'HeraldCondition' => 'HeraldDAO', + 'HeraldConditionResult' => 'Phobject', 'HeraldConditionTranscript' => 'Phobject', 'HeraldContentSourceField' => 'HeraldField', 'HeraldController' => 'PhabricatorController', diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -520,21 +520,29 @@ case self::CONDITION_NOT_REGEXP: $result_if_match = ($condition_type == self::CONDITION_REGEXP); + // 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. + $condition_pattern = $condition_value.'S'; + 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( - pht( - 'Regular expression "%s" in Herald rule "%s" is not valid, '. - 'or exceeded backtracking or recursion limits while '. - 'executing. Verify the expression and correct it or rewrite '. - 'it with less backtracking.', - $condition_value, - $rule->getMonogram())); + try { + $result = phutil_preg_match($condition_pattern, $value); + } catch (PhutilRegexException $ex) { + $message = array(); + $message[] = pht( + 'Regular expression "%s" in Herald rule "%s" is not valid, '. + 'or exceeded backtracking or recursion limits while '. + 'executing. Verify the expression and correct it or rewrite '. + 'it with less backtracking.', + $condition_value, + $rule->getMonogram()); + $message[] = $ex->getMessage(); + $message = implode("\n\n", $message); + + throw new HeraldInvalidConditionException($message); } + if ($result) { return $result_if_match; } diff --git a/src/applications/herald/controller/HeraldTranscriptController.php b/src/applications/herald/controller/HeraldTranscriptController.php --- a/src/applications/herald/controller/HeraldTranscriptController.php +++ b/src/applications/herald/controller/HeraldTranscriptController.php @@ -269,34 +269,20 @@ ->setTarget(phutil_tag('strong', array(), pht('Conditions')))); foreach ($cond_xscripts as $cond_xscript) { - if ($cond_xscript->isForbidden()) { - $icon = 'fa-ban'; - $color = 'indigo'; - $result = pht('Forbidden'); - } else if ($cond_xscript->getResult()) { - $icon = 'fa-check'; - $color = 'green'; - $result = pht('Passed'); - } else { - $icon = 'fa-times'; - $color = 'red'; - $result = pht('Failed'); - } + $result = $cond_xscript->getResult(); - if ($cond_xscript->getNote()) { - $note_text = $cond_xscript->getNote(); - if ($cond_xscript->isForbidden()) { - $note_text = HeraldStateReasons::getExplanation($note_text); - } + $icon = $result->getIconIcon(); + $color = $result->getIconColor(); + $name = $result->getName(); - $note = phutil_tag( + $result_details = $result->newDetailsView(); + if ($result_details !== null) { + $result_details = phutil_tag( 'div', array( 'class' => 'herald-condition-note', ), - $note_text); - } else { - $note = null; + $result_details); } // TODO: This is not really translatable and should be driven through @@ -309,8 +295,8 @@ $cond_item = id(new PHUIStatusItemView()) ->setIcon($icon, $color) - ->setTarget($result) - ->setNote(array($explanation, $note)); + ->setTarget($name) + ->setNote(array($explanation, $result_details)); $cond_list->addItem($cond_item); } diff --git a/src/applications/herald/engine/HeraldEngine.php b/src/applications/herald/engine/HeraldEngine.php --- a/src/applications/herald/engine/HeraldEngine.php +++ b/src/applications/herald/engine/HeraldEngine.php @@ -8,7 +8,8 @@ protected $activeRule; protected $transcript; - protected $fieldCache = array(); + private $fieldCache = array(); + private $fieldExceptions = array(); protected $object; private $dryRun; @@ -64,6 +65,7 @@ $this->transcript = new HeraldTranscript(); $this->transcript->setObjectPHID((string)$object->getPHID()); $this->fieldCache = array(); + $this->fieldExceptions = array(); $this->results = array(); $this->rules = $rules; $this->object = $object; @@ -74,6 +76,7 @@ $is_first_only = $rule->isRepeatFirst(); + $caught = null; try { if (!$this->getDryRun() && $is_first_only && @@ -119,7 +122,21 @@ $names)); } $rule_matches = false; + } catch (Exception $ex) { + $caught = $ex; + } catch (Throwable $ex) { + $caught = $ex; + } + + if ($caught) { + $this->newRuleTranscript($rules[$phid]) + ->setResult(false) + ->setReason( + pht( + 'Rule encountered an exception while evaluting.')); + $rule_matches = false; } + $this->results[$phid] = $rule_matches; if ($rule_matches) { @@ -323,42 +340,18 @@ $result = false; } else { foreach ($conditions as $condition) { - try { - $this->getConditionObjectValue($condition, $object); - } catch (Exception $ex) { - $reason = pht( - 'Field "%s" does not exist!', - $condition->getFieldName()); - $result = false; - break; - } - - // Here, we're profiling the cost to match the condition value against - // whatever test is configured. Normally, this cost should be very - // small (<<1ms) since it amounts to a single comparison: - // - // [ Task author ][ is any of ][ alice ] - // - // However, it may be expensive in some cases, particularly if you - // write a rule with a very creative regular expression that backtracks - // explosively. - // - // At time of writing, the "Another Herald Rule" field is also - // evaluated inside the matching function. This may be arbitrarily - // expensive (it can prompt us to execute any finite number of other - // Herald rules), although we'll push the profiler stack appropriately - // so we don't count the evaluation time against this rule in the final - // profile. - $caught = null; - $this->pushProfilerRule($rule); try { - $match = $this->doesConditionMatch($rule, $condition, $object); + $match = $this->doesConditionMatch( + $rule, + $condition, + $object); } catch (Exception $ex) { $caught = $ex; + } catch (Throwable $ex) { + $caught = $ex; } - $this->popProfilerRule($rule); if ($caught) { throw $ex; @@ -419,63 +412,176 @@ return $result; } - protected function doesConditionMatch( + private function doesConditionMatch( HeraldRule $rule, HeraldCondition $condition, - HeraldAdapter $object) { + HeraldAdapter $adapter) { - $object_value = $this->getConditionObjectValue($condition, $object); $transcript = $this->newConditionTranscript($rule, $condition); + $caught = null; + $result_data = array(); + try { - $result = $object->doesConditionMatch( - $this, + $field_key = $condition->getFieldName(); + + $field_value = $this->getProfiledObjectFieldValue( + $adapter, + $field_key); + + $is_match = $this->getProfiledConditionMatch( + $adapter, $rule, $condition, - $object_value); + $field_value); + if ($is_match) { + $result_code = HeraldConditionResult::RESULT_MATCHED; + } else { + $result_code = HeraldConditionResult::RESULT_FAILED; + } } catch (HeraldInvalidConditionException $ex) { - $result = false; - $transcript->setNote($ex->getMessage()); + $result_code = HeraldConditionResult::RESULT_INVALID; + $caught = $ex; + } catch (Exception $ex) { + $result_code = HeraldConditionResult::RESULT_EXCEPTION; + $caught = $ex; + } catch (Throwable $ex) { + $result_code = HeraldConditionResult::RESULT_EXCEPTION; + $caught = $ex; + } + + if ($caught) { + $result_data = array( + 'exception.class' => get_class($caught), + 'exception.message' => $ex->getMessage(), + ); } + $result = HeraldConditionResult::newFromResultCode($result_code) + ->setResultData($result_data); + $transcript->setResult($result); - return $result; + if ($caught) { + throw $caught; + } + + return $result->getIsMatch(); } - protected function getConditionObjectValue( + private function getProfiledConditionMatch( + HeraldAdapter $adapter, + HeraldRule $rule, HeraldCondition $condition, - HeraldAdapter $object) { + $field_value) { + + // Here, we're profiling the cost to match the condition value against + // whatever test is configured. Normally, this cost should be very + // small (<<1ms) since it amounts to a single comparison: + // + // [ Task author ][ is any of ][ alice ] + // + // However, it may be expensive in some cases, particularly if you + // write a rule with a very creative regular expression that backtracks + // explosively. + // + // At time of writing, the "Another Herald Rule" field is also + // evaluated inside the matching function. This may be arbitrarily + // expensive (it can prompt us to execute any finite number of other + // Herald rules), although we'll push the profiler stack appropriately + // so we don't count the evaluation time against this rule in the final + // profile. + + $this->pushProfilerRule($rule); + + $caught = null; + try { + $is_match = $adapter->doesConditionMatch( + $this, + $rule, + $condition, + $field_value); + } catch (Exception $ex) { + $caught = $ex; + } catch (Throwable $ex) { + $caught = $ex; + } + + $this->popProfilerRule($rule); - $field = $condition->getFieldName(); + if ($caught) { + throw $caught; + } - return $this->getObjectFieldValue($field); + return $is_match; } - public function getObjectFieldValue($field) { - if (!array_key_exists($field, $this->fieldCache)) { - $adapter = $this->object; + private function getProfiledObjectFieldValue( + HeraldAdapter $adapter, + $field_key) { - $adapter->willGetHeraldField($field); + // Before engaging the profiler, make sure the field class is loaded. - $caught = null; + $adapter->willGetHeraldField($field_key); - $this->pushProfilerField($field); - try { - $value = $adapter->getHeraldField($field); - } catch (Exception $ex) { - $caught = $ex; - } - $this->popProfilerField($field); + // The first time we read a field value, we'll actually generate it, which + // may be slow. - if ($caught) { - throw $caught; - } + // After it is generated for the first time, this will just read it from a + // cache, which should be very fast. + + // We still want to profile the request even if it goes to cache so we can + // get an accurate count of how many times we access the field value: when + // trying to improve the performance of Herald rules, it's helpful to know + // how many rules rely on the value of a field which is slow to generate. - $this->fieldCache[$field] = $value; + $caught = null; + + $this->pushProfilerField($field_key); + try { + $value = $this->getObjectFieldValue($field_key); + } catch (Exception $ex) { + $caught = $ex; + } catch (Throwable $ex) { + $caught = $ex; } + $this->popProfilerField($field_key); - return $this->fieldCache[$field]; + if ($caught) { + throw $caught; + } + + return $value; + } + + private function getObjectFieldValue($field_key) { + if (array_key_exists($field_key, $this->fieldExceptions)) { + throw $this->fieldExceptions[$field_key]; + } + + if (array_key_exists($field_key, $this->fieldCache)) { + return $this->fieldCache[$field_key]; + } + + $adapter = $this->object; + + $caught = null; + try { + $value = $adapter->getHeraldField($field_key); + } catch (Exception $ex) { + $caught = $ex; + } catch (Throwable $ex) { + $caught = $ex; + } + + if ($caught) { + $this->fieldExceptions[$field_key] = $caught; + throw $caught; + } + + $this->fieldCache[$field_key] = $value; + + return $value; } protected function getRuleEffects( @@ -639,9 +745,16 @@ $forbidden_reason = $this->forbiddenFields[$field_key]; if ($forbidden_reason !== null) { + $result_code = HeraldConditionResult::RESULT_OBJECT_STATE; + $result_data = array( + 'reason' => $forbidden_reason, + ); + + $result = HeraldConditionResult::newFromResultCode($result_code) + ->setResultData($result_data); + $this->newConditionTranscript($rule, $condition) - ->setResult(HeraldConditionTranscript::RESULT_FORBIDDEN) - ->setNote($forbidden_reason); + ->setResult($result); $is_forbidden = true; } diff --git a/src/applications/herald/storage/transcript/HeraldConditionResult.php b/src/applications/herald/storage/transcript/HeraldConditionResult.php new file mode 100644 --- /dev/null +++ b/src/applications/herald/storage/transcript/HeraldConditionResult.php @@ -0,0 +1,177 @@ + $this->getResultCode(), + 'data' => $this->getResultData(), + ); + } + + public static function newFromMap(array $map) { + $result_code = idx($map, 'code'); + $result = self::newFromResultCode($result_code); + + $result_data = idx($map, 'data', array()); + $result->setResultData($result_data); + + return $result; + } + + public static function newFromResultCode($result_code) { + $map = self::getResultSpecification($result_code); + + $result = new self(); + $result->resultCode = $result_code; + + return $result; + } + + public function getResultCode() { + return $this->resultCode; + } + + private function getResultData() { + return $this->resultData; + } + + public function getIconIcon() { + return $this->getSpecificationProperty('icon'); + } + + public function getIconColor() { + return $this->getSpecificationProperty('color.icon'); + } + + public function getIsMatch() { + return ($this->getSpecificationProperty('match') === true); + } + + public function getName() { + return $this->getSpecificationProperty('name'); + } + + public function newDetailsView() { + switch ($this->resultCode) { + case self::RESULT_OBJECT_STATE: + $reason = $this->getDataProperty('reason'); + $details = HeraldStateReasons::getExplanation($reason); + break; + case self::RESULT_INVALID: + case self::RESULT_EXCEPTION: + $error_class = $this->getDataProperty('exception.class'); + $error_message = $this->getDataProperty('exception.message'); + + if (!strlen($error_class)) { + $error_class = pht('Unknown Error'); + } + + switch ($error_class) { + case 'HeraldInvalidConditionException': + $error_class = pht('Invalid Condition'); + break; + } + + if (!strlen($error_message)) { + $error_message = pht( + 'An unknown error occurred while evaluating this condition. No '. + 'additional information is available.'); + } + + $details = pht( + '%s: %s', + phutil_tag('strong', array(), $error_class), + phutil_escape_html_newlines($error_message)); + break; + $details = 'exception'; + break; + default: + $details = null; + break; + } + + return $details; + } + + public function setResultData(array $result_data) { + $this->resultData = $result_data; + return $this; + } + + private function getDataProperty($key) { + $data = $this->getResultData(); + return idx($data, $key); + } + + private function getSpecificationProperty($key) { + $map = self::getResultSpecification($this->resultCode); + return $map[$key]; + } + + private static function getResultSpecification($result_code) { + $map = self::getResultSpecificationMap(); + + if (!isset($map[$result_code])) { + throw new Exception( + pht( + 'Condition result "%s" is unknown.', + $result_code)); + } + + return $map[$result_code]; + } + + private static function getResultSpecificationMap() { + return array( + self::RESULT_MATCHED => array( + 'match' => true, + 'icon' => 'fa-check', + 'color.icon' => 'green', + 'name' => pht('Passed'), + ), + self::RESULT_FAILED => array( + 'match' => false, + 'icon' => 'fa-times', + 'color.icon' => 'red', + 'name' => pht('Failed'), + ), + self::RESULT_OBJECT_STATE => array( + 'match' => null, + 'icon' => 'fa-ban', + 'color.icon' => 'indigo', + 'name' => pht('Forbidden'), + ), + self::RESULT_INVALID => array( + 'match' => null, + 'icon' => 'fa-exclamation-triangle', + 'color.icon' => 'yellow', + 'name' => pht('Invalid'), + ), + self::RESULT_EXCEPTION => array( + 'match' => null, + 'icon' => 'fa-exclamation-triangle', + 'color.icon' => 'red', + 'name' => pht('Exception'), + ), + self::RESULT_UNKNOWN => array( + 'match' => null, + 'icon' => 'fa-question', + 'color.icon' => 'grey', + 'name' => pht('Unknown'), + ), + ); + } + +} diff --git a/src/applications/herald/storage/transcript/HeraldConditionTranscript.php b/src/applications/herald/storage/transcript/HeraldConditionTranscript.php --- a/src/applications/herald/storage/transcript/HeraldConditionTranscript.php +++ b/src/applications/herald/storage/transcript/HeraldConditionTranscript.php @@ -7,10 +7,17 @@ protected $fieldName; protected $condition; protected $testValue; - protected $note; - protected $result; + protected $resultMap; - const RESULT_FORBIDDEN = 'forbidden'; + // See T13586. Older versions of this record stored a boolean true, boolean + // false, or the string "forbidden" in the "$result" field. They stored a + // human-readable English-language message or a state code in the "$note" + // field. + + // The modern record does not use either field. + + protected $result; + protected $note; public function setRuleID($rule_id) { $this->ruleID = $rule_id; @@ -57,26 +64,39 @@ return $this->testValue; } - public function setNote($note) { - $this->note = $note; - return $this; - } - - public function getNote() { - return $this->note; - } - - public function setResult($result) { - $this->result = $result; + public function setResult(HeraldConditionResult $result) { + $this->resultMap = $result->toMap(); return $this; } public function getResult() { - return $this->result; - } - - public function isForbidden() { - return ($this->getResult() === self::RESULT_FORBIDDEN); + $map = $this->resultMap; + + if (is_array($map)) { + $result = HeraldConditionResult::newFromMap($map); + } else { + $legacy_result = $this->result; + + $result_data = array(); + + if ($legacy_result === 'forbidden') { + $result_code = HeraldConditionResult::RESULT_OBJECT_STATE; + $result_data = array( + 'reason' => $this->note, + ); + } else if ($legacy_result === true) { + $result_code = HeraldConditionResult::RESULT_MATCHED; + } else if ($legacy_result === false) { + $result_code = HeraldConditionResult::RESULT_FAILED; + } else { + $result_code = HeraldConditionResult::RESULT_UNKNOWN; + } + + $result = HeraldConditionResult::newFromResultCode($result_code) + ->setResultData($result_data); + } + + return $result; } } diff --git a/webroot/rsrc/css/application/herald/herald-test.css b/webroot/rsrc/css/application/herald/herald-test.css --- a/webroot/rsrc/css/application/herald/herald-test.css +++ b/webroot/rsrc/css/application/herald/herald-test.css @@ -4,6 +4,8 @@ .herald-condition-note { color: {$red}; + padding: 4px 0; + margin: 4px 0 8px; } textarea.herald-field-value-transcript {