When performing complex edits, pause sub-editors before they publish to propagate "Must Encrypt" and other state

Summary:
See PHI1134. Previously, see T13082 and D19969 for some sort-of-related stuff.

Currently, edits work roughly like this:

  - Suppose we're editing object X, and we're also going to edit some other object, Y, because X mentioned Y or the edit is making X a child or parent of Y, or unblocking Y.
  - Do the actual edit to X, including inverse edits ("alice mentioned Y on X.", "alice added a child revision: X", etc) which apply to Y.
  - Run Herald rules on X.
  - Publish the edit to X.

The "inverse edits" currently do this whole process inline, in a sub-editor. So the flow expands like this:

  - Begin editing X.
  - Update properties on X.
    - Begin inverse-edge editing Y.
    - Update properties on Y.
    - Run (actually, skip) Herald rules on Y.
    - Publish edits to Y.
  - Run Herald rules on X.
  - Publish edits to X.

Notably, the "Y" stuff publishes before the "X" Herald rules run. This creates potential problems:

  - Herald rules may change the name or visibility policy of "X", but we'll publish mail about it via the edits to Y before those edits apply. This is a problem only in theory, we don't ship any upstream rules like this today.
  - Herald rules may "Require Secure Mail", but we won't know that at the time we're building mail about the indirect change to "Y". This is a problem in practice.

Instead, switch to this new flow, where we stop the sub-editors before they publish, then publish everything at the very end once all the edits are complete:

  - Begin editing X.
  - Update properties on X.
    - Begin inverse-edge editing Y.
    - Update properties on Y.
    - Skip Herald on Y.
  - Run Herald rules on X.
  - Publish X.
    - Publish all child-editors of X.
      - Publish Y.

Test Plan:
  - Created "Must Encrypt" Herald rules for Tasks and Revisions.
  - Edited object "A", an object which the rules applied to directly, and set object "B" (a different object which the rules did not hit) as its parent/child and/or unblocked it.
  - In `bin/mail list-outbound`, saw:
    - Mail about object "A" all flagged as "Must Encrypt".
    - Normal mail from object B not flagged "Must Encrypt".
    - Mail from object B about changing relationships to object A flagged as "Must Encrypt".

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20283

src/applications/maniphest/editor/ManiphestTransactionEditor.php

@@ -134,10 +134,7 @@ final class ManiphestTransactionEditor
             $parent_xaction->setMetadataValue('blocker.new', true);
           }
 
-          id(new ManiphestTransactionEditor())
-            ->setActor($this->getActor())
-            ->setActingAsPHID($this->getActingAsPHID())
-            ->setContentSource($this->getContentSource())
+          $this->newSubEditor()
             ->setContinueOnNoEffect(true)
             ->setContinueOnMissingFields(true)
             ->applyTransactions($blocked_task, array($parent_xaction));

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -72,7 +72,7 @@ abstract class PhabricatorApplicationTransactionEditor
   private $mailShouldSend = false;
   private $modularTypes;
   private $silent;
-  private $mustEncrypt;
+  private $mustEncrypt = array();
   private $stampTemplates = array();
   private $mailStamps = array();
   private $oldTo = array();
@@ -90,6 +90,11 @@ abstract class PhabricatorApplicationTransactionEditor
   private $cancelURI;
   private $extensions;
 
+  private $parentEditor;
+  private $subEditors = array();
+  private $publishableObject;
+  private $publishableTransactions;
+
   const STORAGE_ENCODING_BINARY = 'binary';
 
   /**
@@ -1272,10 +1277,9 @@ abstract class PhabricatorApplicationTransactionEditor
         $herald_source = PhabricatorContentSource::newForSource(
           PhabricatorHeraldContentSource::SOURCECONST);
 
-        $herald_editor = newv(get_class($this), array())
+        $herald_editor = $this->newEditorCopy()
           ->setContinueOnNoEffect(true)
           ->setContinueOnMissingFields(true)
-          ->setParentMessageID($this->getParentMessageID())
           ->setIsHeraldEditor(true)
           ->setActor($herald_actor)
           ->setActingAsPHID($herald_phid)
@@ -1330,6 +1334,38 @@ abstract class PhabricatorApplicationTransactionEditor
     }
     $this->heraldHeader = $herald_header;
 
+    // See PHI1134. If we're a subeditor, we don't publish information about
+    // the edit yet. Our parent editor still needs to finish applying
+    // transactions and execute Herald, which may change the information we
+    // publish.
+
+    // For example, Herald actions may change the parent object's title or
+    // visibility, or Herald may apply rules like "Must Encrypt" that affect
+    // email.
+
+    // Once the parent finishes work, it will queue its own publish step and
+    // then queue publish steps for its children.
+
+    $this->publishableObject = $object;
+    $this->publishableTransactions = $xactions;
+    if (!$this->parentEditor) {
+      $this->queuePublishing();
+    }
+
+    return $xactions;
+  }
+
+  final private function queuePublishing() {
+    $object = $this->publishableObject;
+    $xactions = $this->publishableTransactions;
+
+    if (!$object) {
+      throw new Exception(
+        pht(
+          'Editor method "queuePublishing()" was called, but no publishable '.
+          'object is present. This Editor is not ready to publish.'));
+    }
+
     // We're going to compute some of the data we'll use to publish these
     // transactions here, before queueing a worker.
     //
@@ -1392,9 +1428,11 @@ abstract class PhabricatorApplicationTransactionEditor
         'priority' => PhabricatorWorker::PRIORITY_ALERTS,
       ));
 
-    $this->flushTransactionQueue($object);
+    foreach ($this->subEditors as $sub_editor) {
+      $sub_editor->queuePublishing();
+    }
 
-    return $xactions;
+    $this->flushTransactionQueue($object);
   }
 
   protected function didCatchDuplicateKeyException(
@@ -3818,6 +3856,11 @@ abstract class PhabricatorApplicationTransactionEditor
 
     $this->mustEncrypt = $adapter->getMustEncryptReasons();
 
+    // See PHI1134. Propagate "Must Encrypt" state to sub-editors.
+    foreach ($this->subEditors as $sub_editor) {
+      $sub_editor->mustEncrypt = $this->mustEncrypt;
+    }
+
     $apply_xactions = $this->didApplyHeraldRules($object, $adapter, $xscript);
     assert_instances_of($apply_xactions, 'PhabricatorApplicationTransaction');
 
@@ -4034,15 +4077,10 @@ abstract class PhabricatorApplicationTransactionEditor
         ->setOldValue($old_phids)
         ->setNewValue($new_phids);
 
-      $editor
+      $editor = $this->newSubEditor($editor)
         ->setContinueOnNoEffect(true)
         ->setContinueOnMissingFields(true)
-        ->setParentMessageID($this->getParentMessageID())
-        ->setIsInverseEdgeEditor(true)
-        ->setIsSilent($this->getIsSilent())
-        ->setActor($this->requireActor())
-        ->setActingAsPHID($this->getActingAsPHID())
-        ->setContentSource($this->getContentSource());
+        ->setIsInverseEdgeEditor(true);
 
       $editor->applyTransactions($node, array($template));
     }
@@ -4551,23 +4589,41 @@ abstract class PhabricatorApplicationTransactionEditor
     $xactions = $this->transactionQueue;
     $this->transactionQueue = array();
 
-    $editor = $this->newQueueEditor();
+    $editor = $this->newEditorCopy();
 
     return $editor->applyTransactions($object, $xactions);
   }
 
-  private function newQueueEditor() {
-    $editor = id(newv(get_class($this), array()))
+  final protected function newSubEditor(
+    PhabricatorApplicationTransactionEditor $template = null) {
+    $editor = $this->newEditorCopy($template);
+
+    $editor->parentEditor = $this;
+    $this->subEditors[] = $editor;
+
+    return $editor;
+  }
+
+  private function newEditorCopy(
+    PhabricatorApplicationTransactionEditor $template = null) {
+    if ($template === null) {
+      $template = newv(get_class($this), array());
+    }
+
+    $editor = id(clone $template)
       ->setActor($this->getActor())
       ->setContentSource($this->getContentSource())
       ->setContinueOnNoEffect($this->getContinueOnNoEffect())
       ->setContinueOnMissingFields($this->getContinueOnMissingFields())
+      ->setParentMessageID($this->getParentMessageID())
       ->setIsSilent($this->getIsSilent());
 
     if ($this->actingAsPHID !== null) {
       $editor->setActingAsPHID($this->actingAsPHID);
     }
 
+    $editor->mustEncrypt = $this->mustEncrypt;
+
     return $editor;
   }