From 4f49ec1cff1c5a53ccdd4d2d78e98a39a3b462a6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 7 Aug 2013 06:47:55 -0700 Subject: [PATCH] Remove HeraldDryRunAdapter Summary: Ref T2769. This isn't a real adapter and its methods are increasingly hacky messes. Make "dry run" a first-class concept on the HeraldEngine instead and remove the adapter. Test Plan: Ran Herald via test console and via CLI. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T2769 Differential Revision: https://secure.phabricator.com/D6693 --- src/__phutil_library_map__.php | 2 - .../herald/adapter/HeraldDryRunAdapter.php | 44 ------- .../HeraldTestConsoleController.php | 8 +- .../herald/engine/HeraldEngine.php | 109 ++++++++++-------- ...habricatorRepositoryCommitHeraldWorker.php | 3 +- 5 files changed, 69 insertions(+), 97 deletions(-) delete mode 100644 src/applications/herald/adapter/HeraldDryRunAdapter.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 008bc91901..172c796c8a 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -604,7 +604,6 @@ phutil_register_library_map(array( 'HeraldDAO' => 'applications/herald/storage/HeraldDAO.php', 'HeraldDeleteController' => 'applications/herald/controller/HeraldDeleteController.php', 'HeraldDifferentialRevisionAdapter' => 'applications/herald/adapter/HeraldDifferentialRevisionAdapter.php', - 'HeraldDryRunAdapter' => 'applications/herald/adapter/HeraldDryRunAdapter.php', 'HeraldEditLogQuery' => 'applications/herald/query/HeraldEditLogQuery.php', 'HeraldEffect' => 'applications/herald/engine/HeraldEffect.php', 'HeraldEngine' => 'applications/herald/engine/HeraldEngine.php', @@ -2615,7 +2614,6 @@ phutil_register_library_map(array( 'HeraldDAO' => 'PhabricatorLiskDAO', 'HeraldDeleteController' => 'HeraldController', 'HeraldDifferentialRevisionAdapter' => 'HeraldAdapter', - 'HeraldDryRunAdapter' => 'HeraldAdapter', 'HeraldEditLogQuery' => 'PhabricatorOffsetPagedQuery', 'HeraldInvalidActionException' => 'Exception', 'HeraldInvalidConditionException' => 'Exception', diff --git a/src/applications/herald/adapter/HeraldDryRunAdapter.php b/src/applications/herald/adapter/HeraldDryRunAdapter.php deleted file mode 100644 index 0b5526b823..0000000000 --- a/src/applications/herald/adapter/HeraldDryRunAdapter.php +++ /dev/null @@ -1,44 +0,0 @@ -needValidateAuthors(true) ->execute(); - $engine = new HeraldEngine(); - $effects = $engine->applyRules($rules, $adapter); + $engine = id(new HeraldEngine()) + ->setDryRun(true); - $dry_run = new HeraldDryRunAdapter(); - $engine->applyEffects($effects, $dry_run, $rules); + $effects = $engine->applyRules($rules, $adapter); + $engine->applyEffects($effects, $adapter, $rules); $xscript = $engine->getTranscript(); diff --git a/src/applications/herald/engine/HeraldEngine.php b/src/applications/herald/engine/HeraldEngine.php index 895ef95cb9..d99aa22ac1 100644 --- a/src/applications/herald/engine/HeraldEngine.php +++ b/src/applications/herald/engine/HeraldEngine.php @@ -9,6 +9,16 @@ final class HeraldEngine { protected $fieldCache = array(); protected $object = null; + private $dryRun; + + public function setDryRun($dry_run) { + $this->dryRun = $dry_run; + return $this; + } + + public function getDryRun() { + return $this->dryRun; + } public function getRule($id) { return idx($this->rules, $id); @@ -111,67 +121,76 @@ final class HeraldEngine { public function applyEffects( array $effects, - HeraldAdapter $object, + HeraldAdapter $adapter, array $rules) { assert_instances_of($effects, 'HeraldEffect'); assert_instances_of($rules, 'HeraldRule'); - $this->transcript->setDryRun((int)($object instanceof HeraldDryRunAdapter)); + $this->transcript->setDryRun((int)$this->getDryRun()); - $xscripts = $object->applyHeraldEffects($effects); - foreach ($xscripts as $apply_xscript) { - if (!($apply_xscript instanceof HeraldApplyTranscript)) { - throw new Exception( - "Heraldable must return HeraldApplyTranscripts from ". - "applyHeraldEffect()."); + if ($this->getDryRun()) { + $xscripts = array(); + foreach ($effects as $effect) { + $xscripts[] = new HeraldApplyTranscript( + $effect, + false, + pht('This was a dry run, so no actions were actually taken.')); } + } else { + $xscripts = $adapter->applyHeraldEffects($effects); + } + + assert_instances_of($xscripts, 'HeraldApplyTranscript'); + foreach ($xscripts as $apply_xscript) { $this->transcript->addApplyTranscript($apply_xscript); } - if (!$this->transcript->getDryRun()) { + // For dry runs, don't mark the rule as having applied to the object. + if ($this->getDryRun()) { + return; + } - $rules = mpull($rules, null, 'getID'); - $applied_ids = array(); - $first_policy = HeraldRepetitionPolicyConfig::toInt( - HeraldRepetitionPolicyConfig::FIRST); + $rules = mpull($rules, null, 'getID'); + $applied_ids = array(); + $first_policy = HeraldRepetitionPolicyConfig::toInt( + HeraldRepetitionPolicyConfig::FIRST); - // Mark all the rules that have had their effects applied as having been - // executed for the current object. - $rule_ids = mpull($xscripts, 'getRuleID'); + // Mark all the rules that have had their effects applied as having been + // executed for the current object. + $rule_ids = mpull($xscripts, 'getRuleID'); - foreach ($rule_ids as $rule_id) { - if (!$rule_id) { - // Some apply transcripts are purely informational and not associated - // with a rule, e.g. carryover emails from earlier revisions. - continue; - } - - $rule = idx($rules, $rule_id); - if (!$rule) { - continue; - } - - if ($rule->getRepetitionPolicy() == $first_policy) { - $applied_ids[] = $rule_id; - } + foreach ($rule_ids as $rule_id) { + if (!$rule_id) { + // Some apply transcripts are purely informational and not associated + // with a rule, e.g. carryover emails from earlier revisions. + continue; } - if ($applied_ids) { - $conn_w = id(new HeraldRule())->establishConnection('w'); - $sql = array(); - foreach ($applied_ids as $id) { - $sql[] = qsprintf( - $conn_w, - '(%s, %d)', - $object->getPHID(), - $id); - } - queryfx( + $rule = idx($rules, $rule_id); + if (!$rule) { + continue; + } + + if ($rule->getRepetitionPolicy() == $first_policy) { + $applied_ids[] = $rule_id; + } + } + + if ($applied_ids) { + $conn_w = id(new HeraldRule())->establishConnection('w'); + $sql = array(); + foreach ($applied_ids as $id) { + $sql[] = qsprintf( $conn_w, - 'INSERT IGNORE INTO %T (phid, ruleID) VALUES %Q', - HeraldRule::TABLE_RULE_APPLIED, - implode(', ', $sql)); + '(%s, %d)', + $adapter->getPHID(), + $id); } + queryfx( + $conn_w, + 'INSERT IGNORE INTO %T (phid, ruleID) VALUES %Q', + HeraldRule::TABLE_RULE_APPLIED, + implode(', ', $sql)); } } diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php index 9ea559a369..963c22799d 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php @@ -38,6 +38,7 @@ final class PhabricatorRepositoryCommitHeraldWorker $effects = $engine->applyRules($rules, $adapter); $engine->applyEffects($effects, $adapter, $rules); + $xscript = $engine->getTranscript(); $audit_phids = $adapter->getAuditMap(); if ($audit_phids) { @@ -63,8 +64,6 @@ final class PhabricatorRepositoryCommitHeraldWorker return; } - $xscript = $engine->getTranscript(); - $revision = $adapter->loadDifferentialRevision(); if ($revision) { $name = $revision->getTitle();