Improve ApplicationTransaction behavior for poorly constructed transactions

Summary:
Ref T2222. Five very small improvements:

  - I hit this exception and it took a bit to understand which transaction was causing problems. Add an `Exception` subclass which does a better job of making the message debuggable.
  - The `oldValue` of a transaction may be `null`, legitimately (for example, changing the `repositoryPHID` for a revision from `null` to some valid PHID). Do a check to see if `setOldValue()` has been called, instead of a check for a `null` value.
  - Add an additional check for the other case (shouldn't have a value, but does).
  - When we're not generating a value, don't bother calling the code to generate it. The best case scenario is that it has no effect; any effect it might have (changing the value) is always wrong.
  - Maniphest didn't fall back to the parent correctly when computing this flag, so it got the wrong result for `CustomField` transactions.

Test Plan: Resolved the issue I was hitting more easily, made updates to a `null`-valued custom field, and applied other normal sorts of transactions successfully.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4557, T2222

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

src/__phutil_library_map__.php

@@ -1196,6 +1196,7 @@ phutil_register_library_map(array(
     'PhabricatorApplicationTransactionPHIDTypeTransaction' => 'applications/transactions/phid/PhabricatorApplicationTransactionPHIDTypeTransaction.php',
     'PhabricatorApplicationTransactionQuery' => 'applications/transactions/query/PhabricatorApplicationTransactionQuery.php',
     'PhabricatorApplicationTransactionResponse' => 'applications/transactions/response/PhabricatorApplicationTransactionResponse.php',
+    'PhabricatorApplicationTransactionStructureException' => 'applications/transactions/exception/PhabricatorApplicationTransactionStructureException.php',
     'PhabricatorApplicationTransactionTextDiffDetailView' => 'applications/transactions/view/PhabricatorApplicationTransactionTextDiffDetailView.php',
     'PhabricatorApplicationTransactionValidationError' => 'applications/transactions/error/PhabricatorApplicationTransactionValidationError.php',
     'PhabricatorApplicationTransactionValidationException' => 'applications/transactions/exception/PhabricatorApplicationTransactionValidationException.php',
@@ -3895,6 +3896,7 @@ phutil_register_library_map(array(
     'PhabricatorApplicationTransactionPHIDTypeTransaction' => 'PhabricatorPHIDType',
     'PhabricatorApplicationTransactionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorApplicationTransactionResponse' => 'AphrontProxyResponse',
+    'PhabricatorApplicationTransactionStructureException' => 'Exception',
     'PhabricatorApplicationTransactionTextDiffDetailView' => 'AphrontView',
     'PhabricatorApplicationTransactionValidationError' => 'Phobject',
     'PhabricatorApplicationTransactionValidationException' => 'Exception',

src/applications/maniphest/storage/ManiphestTransaction.php

@@ -28,17 +28,13 @@ final class ManiphestTransaction
   }
 
   public function shouldGenerateOldValue() {
-    $generate = true;
     switch ($this->getTransactionType()) {
       case self::TYPE_PROJECT_COLUMN:
       case self::TYPE_EDGE:
-        $generate = false;
+        return false;
-        break;
-      default:
-        $generate = true;
-        break;
     }
-    return $generate;
+
+    return parent::shouldGenerateOldValue();
   }
 
   public function getRequiredHandlePHIDs() {

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -122,8 +122,11 @@ abstract class PhabricatorApplicationTransactionEditor
   private function adjustTransactionValues(
     PhabricatorLiskDAO $object,
     PhabricatorApplicationTransaction $xaction) {
+
+    if ($xaction->shouldGenerateOldValue()) {
       $old = $this->getTransactionOldValue($object, $xaction);
       $xaction->setOldValue($old);
+    }
 
     $new = $this->getTransactionNewValue($object, $xaction);
     $xaction->setNewValue($new);
@@ -723,37 +726,61 @@ abstract class PhabricatorApplicationTransactionEditor
     assert_instances_of($xactions, 'PhabricatorApplicationTransaction');
     foreach ($xactions as $xaction) {
       if ($xaction->getPHID() || $xaction->getID()) {
-        throw new Exception(
+        throw new PhabricatorApplicationTransactionStructureException(
-          "You can not apply transactions which already have IDs/PHIDs!");
+          $xaction,
+          pht(
+            "You can not apply transactions which already have IDs/PHIDs!"));
       }
       if ($xaction->getObjectPHID()) {
-        throw new Exception(
+        throw new PhabricatorApplicationTransactionStructureException(
-          "You can not apply transactions which already have objectPHIDs!");
+          $xaction,
+          pht(
+            "You can not apply transactions which already have objectPHIDs!"));
       }
       if ($xaction->getAuthorPHID()) {
-        throw new Exception(
+        throw new PhabricatorApplicationTransactionStructureException(
-          "You can not apply transactions which already have authorPHIDs!");
+          $xaction,
+          pht(
+            'You can not apply transactions which already have authorPHIDs!'));
       }
       if ($xaction->getCommentPHID()) {
-        throw new Exception(
+        throw new PhabricatorApplicationTransactionStructureException(
-          "You can not apply transactions which already have commentPHIDs!");
+          $xaction,
+          pht(
+            'You can not apply transactions which already have '.
+            'commentPHIDs!'));
       }
       if ($xaction->getCommentVersion() !== 0) {
-        throw new Exception(
+        throw new PhabricatorApplicationTransactionStructureException(
-          "You can not apply transactions which already have commentVersions!");
+          $xaction,
+          pht(
+            'You can not apply transactions which already have '.
+            'commentVersions!'));
       }
 
-      if (!$xaction->shouldGenerateOldValue()) {
+      $expect_value = !$xaction->shouldGenerateOldValue();
-        if ($xaction->getOldValue() === null) {
+      $has_value = $xaction->hasOldValue();
-          throw new Exception(
+
-            'You can not apply transactions which should already have '.
+      if ($expect_value && !$has_value) {
-            'oldValue but do not!');
+        throw new PhabricatorApplicationTransactionStructureException(
+          $xaction,
+          pht(
+            'This transaction is supposed to have an oldValue set, but '.
+            'it does not!'));
       }
+
+      if ($has_value && !$expect_value) {
+        throw new PhabricatorApplicationTransactionStructureException(
+          $xaction,
+          pht(
+            'This transaction should generate its oldValue automatically, '.
+            'but has already had one set!'));
       }
 
       $type = $xaction->getTransactionType();
       if (empty($types[$type])) {
-        throw new Exception(
+        throw new PhabricatorApplicationTransactionStructureException(
+          $xaction,
           pht(
             'Transaction has type "%s", but that transaction type is not '.
             'supported by this editor (%s).',

src/applications/transactions/exception/PhabricatorApplicationTransactionStructureException.php

@@ -0,0 +1,20 @@
+<?php
+
+final class PhabricatorApplicationTransactionStructureException
+  extends Exception {
+
+  public function __construct(
+    PhabricatorApplicationTransaction $xaction,
+    $message) {
+
+    $full_message = pht(
+      'Attempting to apply a transaction (of class "%s", with type "%s") '.
+      'which has not been constructed correctly: %s',
+      get_class($xaction),
+      $xaction->getTransactionType(),
+      $message);
+
+    parent::__construct($full_message);
+  }
+
+}

src/applications/transactions/storage/PhabricatorApplicationTransaction.php

@@ -30,6 +30,7 @@ abstract class PhabricatorApplicationTransaction
   private $transactionGroup = array();
   private $viewer = self::ATTACHABLE;
   private $object = self::ATTACHABLE;
+  private $oldValueHasBeenSet = false;
 
   private $ignoreOnNoEffect;
 
@@ -169,6 +170,16 @@ abstract class PhabricatorApplicationTransaction
     return $blocks;
   }
 
+  public function setOldValue($value) {
+    $this->oldValueHasBeenSet = true;
+    $this->writeField('oldValue', $value);
+    return $this;
+  }
+
+  public function hasOldValue() {
+    return $this->oldValueHasBeenSet;
+  }
+
 
 /* -(  Rendering  )---------------------------------------------------------- */