From cd3a3bf759b18ee6ac1c94a3f9072f28e63a88c0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Aug 2011 13:05:12 -0700 Subject: [PATCH] Make Herald Rules sticky in X-Herald-Rules Summary: See T354. List every rule which has ever been applied in X-Herald-Rules, not just the ones which most recently triggered. Also some random fixes while I was debugging this: - When conduit methods throw non-conduit exceptions, make sure they get logged. - Trigger the Facebook "tasks" backcompat block only if we were going to fail (this should reduce the shakniess of the transition). - Fix some log spew from the new field stuff. Test Plan: - Created a rule (ID #3) "No Zebras" which triggers for revisions without "zebra" in the title. - Created a revision without "zebra" in the title, got X-Herald-Rules: <2>, <3> - Updated revision to have "zebra" in the title, verified rule did not trigger in Herald transcript. - Verified X-Herald-Rules is still: <2>, <3> Reviewed By: aran Reviewers: aran, jungejason, tuomaspelkonen CC: aran, epriestley Differential Revision: 817 --- .../api/PhabricatorConduitAPIController.php | 1 + .../conduit/controller/api/__init__.php | 1 + .../revision/DifferentialRevisionEditor.php | 16 +++++++--------- .../ccs/DifferentialCCsFieldSpecification.php | 2 +- .../DifferentialSummaryFieldSpecification.php | 4 ++-- .../transcript/base/HeraldTranscript.php | 17 +++++++++++++++++ 6 files changed, 29 insertions(+), 12 deletions(-) diff --git a/src/applications/conduit/controller/api/PhabricatorConduitAPIController.php b/src/applications/conduit/controller/api/PhabricatorConduitAPIController.php index 731c5f6a2e..bb5ddffed2 100644 --- a/src/applications/conduit/controller/api/PhabricatorConduitAPIController.php +++ b/src/applications/conduit/controller/api/PhabricatorConduitAPIController.php @@ -124,6 +124,7 @@ class PhabricatorConduitAPIController list($error_code, $error_info) = $auth_error; } } catch (Exception $ex) { + phlog($ex); $result = null; $error_code = 'ERR-CONDUIT-CORE'; $error_info = $ex->getMessage(); diff --git a/src/applications/conduit/controller/api/__init__.php b/src/applications/conduit/controller/api/__init__.php index 706e380ddf..e6c5f66402 100644 --- a/src/applications/conduit/controller/api/__init__.php +++ b/src/applications/conduit/controller/api/__init__.php @@ -17,6 +17,7 @@ phutil_require_module('phabricator', 'storage/queryfx'); phutil_require_module('phabricator', 'view/control/table'); phutil_require_module('phabricator', 'view/layout/panel'); +phutil_require_module('phutil', 'error'); phutil_require_module('phutil', 'markup'); phutil_require_module('phutil', 'parser/json'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/differential/editor/revision/DifferentialRevisionEditor.php b/src/applications/differential/editor/revision/DifferentialRevisionEditor.php index c71f66d55c..eaa3107cf2 100644 --- a/src/applications/differential/editor/revision/DifferentialRevisionEditor.php +++ b/src/applications/differential/editor/revision/DifferentialRevisionEditor.php @@ -83,15 +83,13 @@ class DifferentialRevisionEditor { $aux_fields = mpull($aux_fields, null, 'getCommitMessageKey'); foreach ($fields as $field => $value) { - - if ($field == 'tasks') { - // TODO: Deprecate once this can be fully supported with custom fields. - // This is just to prevent a backcompat break for Facebook. - $this->setTasks($value); - continue; - } - if (empty($aux_fields[$field])) { + if ($field == 'tasks') { + // TODO: Deprecate once this can be fully supported with custom + // fields. This is just to prevent a backcompat break for Facebook. + $this->setTasks($value); + continue; + } throw new Exception( "Parsed commit message contains unrecognized field '{$field}'."); } @@ -253,7 +251,7 @@ class DifferentialRevisionEditor { $xscript_phid = $xscript->getPHID(); $xscript_header = $xscript->getXHeraldRulesHeader(); - HeraldTranscript::saveXHeraldRulesHeader( + $xscript_header = HeraldTranscript::saveXHeraldRulesHeader( $revision->getPHID(), $xscript_header); diff --git a/src/applications/differential/field/specification/ccs/DifferentialCCsFieldSpecification.php b/src/applications/differential/field/specification/ccs/DifferentialCCsFieldSpecification.php index 6bf514fb7f..701fb47d12 100644 --- a/src/applications/differential/field/specification/ccs/DifferentialCCsFieldSpecification.php +++ b/src/applications/differential/field/specification/ccs/DifferentialCCsFieldSpecification.php @@ -19,7 +19,7 @@ final class DifferentialCCsFieldSpecification extends DifferentialFieldSpecification { - private $ccs; + private $ccs = array(); public function shouldAppearOnRevisionView() { return true; diff --git a/src/applications/differential/field/specification/summary/DifferentialSummaryFieldSpecification.php b/src/applications/differential/field/specification/summary/DifferentialSummaryFieldSpecification.php index 377e052e6c..46209c907f 100644 --- a/src/applications/differential/field/specification/summary/DifferentialSummaryFieldSpecification.php +++ b/src/applications/differential/field/specification/summary/DifferentialSummaryFieldSpecification.php @@ -19,7 +19,7 @@ final class DifferentialSummaryFieldSpecification extends DifferentialFieldSpecification { - private $summary; + private $summary = ''; public function shouldAppearOnEdit() { $this->summary = $this->getRevision()->getSummary(); @@ -51,7 +51,7 @@ final class DifferentialSummaryFieldSpecification } public function setValueFromParsedCommitMessage($value) { - $this->summary = $value; + $this->summary = (string)$value; return $this; } diff --git a/src/applications/herald/storage/transcript/base/HeraldTranscript.php b/src/applications/herald/storage/transcript/base/HeraldTranscript.php index fc16e9ae72..3706f770dd 100644 --- a/src/applications/herald/storage/transcript/base/HeraldTranscript.php +++ b/src/applications/herald/storage/transcript/base/HeraldTranscript.php @@ -60,6 +60,13 @@ class HeraldTranscript extends HeraldDAO { } public static function saveXHeraldRulesHeader($phid, $header) { + + // Combine any existing header with the new header, listing all rules + // which have ever triggered for this object. + $header = self::combineXHeraldRulesHeaders( + self::loadXHeraldRulesHeader($phid), + $header); + queryfx( id(new HeraldTranscript())->establishConnection('w'), 'INSERT INTO %T (phid, header) VALUES (%s, %s) @@ -67,6 +74,16 @@ class HeraldTranscript extends HeraldDAO { self::TABLE_SAVED_HEADER, $phid, $header); + + return $header; + } + + private static function combineXHeraldRulesHeaders($u, $v) { + $u = preg_split('/[, ]+/', $u); + $v = preg_split('/[, ]+/', $v); + + $combined = array_unique(array_filter(array_merge($u, $v))); + return implode(', ', $combined); } public static function loadXHeraldRulesHeader($phid) {