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

@@ -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',

src/applications/herald/adapter/HeraldDryRunAdapter.php

@@ -1,44 +0,0 @@
-<?php
-
-final class HeraldDryRunAdapter extends HeraldAdapter {
-
-  public function getPHID() {
-    return 0;
-  }
-
-  public function isEnabled() {
-    return false;
-  }
-
-  public function getAdapterContentName() {
-    return null;
-  }
-
-  public function getHeraldName() {
-    return 'Dry Run';
-  }
-
-  public function getHeraldField($field) {
-    return null;
-  }
-
-  public function getFields() {
-    return array();
-  }
-
-  public function getActions($rule_type) {
-    return array();
-  }
-
-  public function applyHeraldEffects(array $effects) {
-    assert_instances_of($effects, 'HeraldEffect');
-    $results = array();
-    foreach ($effects as $effect) {
-      $results[] = new HeraldApplyTranscript(
-        $effect,
-        false,
-        pht('This was a dry run, so no actions were actually taken.'));
-    }
-    return $results;
-  }
-}

src/applications/herald/controller/HeraldTestConsoleController.php

@@ -76,11 +76,11 @@ final class HeraldTestConsoleController extends HeraldController {
             ->needValidateAuthors(true)
             ->execute();
 
-          $engine = new HeraldEngine();
+          $engine = id(new HeraldEngine())
-          $effects = $engine->applyRules($rules, $adapter);
+            ->setDryRun(true);
 
-          $dry_run = new HeraldDryRunAdapter();
+          $effects = $engine->applyRules($rules, $adapter);
-          $engine->applyEffects($effects, $dry_run, $rules);
+          $engine->applyEffects($effects, $adapter, $rules);
 
           $xscript = $engine->getTranscript();
 

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);
+    if ($this->getDryRun()) {
-    foreach ($xscripts as $apply_xscript) {
+      $xscripts = array();
-      if (!($apply_xscript instanceof HeraldApplyTranscript)) {
+      foreach ($effects as $effect) {
-        throw new Exception(
+        $xscripts[] = new HeraldApplyTranscript(
-          "Heraldable must return HeraldApplyTranscripts from ".
+          $effect,
-          "applyHeraldEffect().");
+          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');
+    $rules = mpull($rules, null, 'getID');
-      $applied_ids = array();
+    $applied_ids = array();
-      $first_policy = HeraldRepetitionPolicyConfig::toInt(
+    $first_policy = HeraldRepetitionPolicyConfig::toInt(
-        HeraldRepetitionPolicyConfig::FIRST);
+      HeraldRepetitionPolicyConfig::FIRST);
 
-      // Mark all the rules that have had their effects applied as having been
+    // Mark all the rules that have had their effects applied as having been
-      // executed for the current object.
+    // executed for the current object.
-      $rule_ids = mpull($xscripts, 'getRuleID');
+    $rule_ids = mpull($xscripts, 'getRuleID');
 
-      foreach ($rule_ids as $rule_id) {
+    foreach ($rule_ids as $rule_id) {
-        if (!$rule_id) {
+      if (!$rule_id) {
-          // Some apply transcripts are purely informational and not associated
+        // Some apply transcripts are purely informational and not associated
-          // with a rule, e.g. carryover emails from earlier revisions.
+        // with a rule, e.g. carryover emails from earlier revisions.
-          continue;
+        continue;
-        }
-
-        $rule = idx($rules, $rule_id);
-        if (!$rule) {
-          continue;
-        }
-
-        if ($rule->getRepetitionPolicy() == $first_policy) {
-          $applied_ids[] = $rule_id;
-        }
       }
 
-      if ($applied_ids) {
+      $rule = idx($rules, $rule_id);
-        $conn_w = id(new HeraldRule())->establishConnection('w');
+      if (!$rule) {
-        $sql = array();
+        continue;
-        foreach ($applied_ids as $id) {
+      }
-          $sql[] = qsprintf(
+
-            $conn_w,
+      if ($rule->getRepetitionPolicy() == $first_policy) {
-            '(%s, %d)',
+        $applied_ids[] = $rule_id;
-            $object->getPHID(),
+      }
-            $id);
+    }
-        }
+
-        queryfx(
+    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',
+          '(%s, %d)',
-          HeraldRule::TABLE_RULE_APPLIED,
+          $adapter->getPHID(),
-          implode(', ', $sql));
+          $id);
       }
+      queryfx(
+        $conn_w,
+        'INSERT IGNORE INTO %T (phid, ruleID) VALUES %Q',
+        HeraldRule::TABLE_RULE_APPLIED,
+        implode(', ', $sql));
     }
   }
 

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();