From 814e6d2de927a7b2137d7198e9377382ee8d497e Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 25 Feb 2019 19:13:55 -0800
Subject: [PATCH] Add more type checking to transactions queued by Herald

Summary:
See PHI1096. Depends on D20213. An install is reporting a hard-to-reproduce issue where a non-transaction gets queued by Herald somehow. This might be in third-party code.

Sprinkle the relevant parts of the code with `final` and type checking to try to catch the problem before it causes a fatal we can't pull a stack trace out of.

Test Plan: Poked around locally (e.g., edited revisions to cause Herald to trigger), but hard to know if this will do what it's supposed to or not without deploying and seeing if it catches anything.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20214
---
 .../editor/DifferentialDiffEditor.php         |  9 --------
 .../herald/adapter/HeraldAdapter.php          | 21 +++++++++++++++----
 ...habricatorApplicationTransactionEditor.php |  9 ++++++--
 3 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/src/applications/differential/editor/DifferentialDiffEditor.php b/src/applications/differential/editor/DifferentialDiffEditor.php
index 261f6f1598..e78e08d808 100644
--- a/src/applications/differential/editor/DifferentialDiffEditor.php
+++ b/src/applications/differential/editor/DifferentialDiffEditor.php
@@ -208,15 +208,6 @@ final class DifferentialDiffEditor
     return $adapter;
   }
 
-  protected function didApplyHeraldRules(
-    PhabricatorLiskDAO $object,
-    HeraldAdapter $adapter,
-    HeraldTranscript $transcript) {
-
-    $xactions = array();
-    return $xactions;
-  }
-
   private function updateDiffFromDict(DifferentialDiff $diff, $dict) {
     $diff
       ->setSourcePath(idx($dict, 'sourcePath'))
diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php
index a266e21f39..69f538afcc 100644
--- a/src/applications/herald/adapter/HeraldAdapter.php
+++ b/src/applications/herald/adapter/HeraldAdapter.php
@@ -186,15 +186,16 @@ abstract class HeraldAdapter extends Phobject {
     return $this->appliedTransactions;
   }
 
-  public function queueTransaction($transaction) {
+  final public function queueTransaction(
+    PhabricatorApplicationTransaction $transaction) {
     $this->queuedTransactions[] = $transaction;
   }
 
-  public function getQueuedTransactions() {
+  final public function getQueuedTransactions() {
     return $this->queuedTransactions;
   }
 
-  public function newTransaction() {
+  final public function newTransaction() {
     $object = $this->newObject();
 
     if (!($object instanceof PhabricatorApplicationTransactionInterface)) {
@@ -205,7 +206,19 @@ abstract class HeraldAdapter extends Phobject {
           'PhabricatorApplicationTransactionInterface'));
     }
 
-    return $object->getApplicationTransactionTemplate();
+    $xaction = $object->getApplicationTransactionTemplate();
+
+    if (!($xaction instanceof PhabricatorApplicationTransaction)) {
+      throw new Exception(
+        pht(
+          'Expected object (of class "%s") to return a transaction template '.
+          '(of class "%s"), but it returned something else ("%s").',
+          get_class($object),
+          'PhabricatorApplicationTransaction',
+          phutil_describe_type($xaction)));
+    }
+
+    return $xaction;
   }
 
 
diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
index 9460dd3030..3a46784a33 100644
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -3779,9 +3779,14 @@ abstract class PhabricatorApplicationTransactionEditor
 
     $this->mustEncrypt = $adapter->getMustEncryptReasons();
 
+    $apply_xactions = $this->didApplyHeraldRules($object, $adapter, $xscript);
+    assert_instances_of($apply_xactions, 'PhabricatorApplicationTransaction');
+
+    $queue_xactions = $adapter->getQueuedTransactions();
+
     return array_merge(
-      $this->didApplyHeraldRules($object, $adapter, $xscript),
-      $adapter->getQueuedTransactions());
+      array_values($apply_xactions),
+      array_values($queue_xactions));
   }
 
   protected function didApplyHeraldRules(