Page MenuHomePhabricator
Files F15388020

D21563.diff
No OneTemporary
Actions

D21563.diff
View Options

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 @@
+<?php
+
+final class HeraldConditionResult
+ extends Phobject {
+
+ const RESULT_MATCHED = 'matched';
+ const RESULT_FAILED = 'failed';
+ const RESULT_OBJECT_STATE = 'object-state';
+ const RESULT_INVALID = 'invalid';
+ const RESULT_EXCEPTION = 'exception';
+ const RESULT_UNKNOWN = 'unknown';
+
+ private $resultCode;
+ private $resultData = array();
+
+ public function toMap() {
+ return array(
+ 'code' => $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 {

File Metadata

Mime Type
text/plain
Expires
Sun, Mar 16, 3:12 AM (1 w, 3 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7704086
Default Alt Text
D21563.diff (23 KB)

Event Timeline

Phabricator · Built by Phacility