Implement "replace" transactions in Pholio without "applyInitialEffects"

Summary:
Depends on D19923. Ref T11351. Currently, this transaction takes an `Image` as the `newValue` and uses some magic to reduce it into PHIDs by the time we're done.

This creates some problems today where I'd like to get rid of `applyInitialEffects` for MFA code. In the future, it creates a problem becuase there's no way to pass an entire `Image` object over the API.

Instead, create the `Image` first, then just provide the PHID. This is generally simpler, will work well with the API in the future, and stops us from needing any `applyInitialEffects` stuff.

Test Plan: Replaced images in a Pholio mock.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T11351

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

src/applications/pholio/controller/PholioMockEditController.php

@@ -143,20 +143,22 @@ final class PholioMockEditController extends PholioController {
           $replace_image = PholioImage::initializeNewImage()
             ->setAuthorPHID($viewer->getPHID())
             ->setReplacesImagePHID($replaces_image_phid)
-            ->setFilePhid($file_phid)
+            ->setFilePHID($file_phid)
             ->attachFile($file)
             ->setName(strlen($title) ? $title : $file->getName())
             ->setDescription($description)
-            ->setSequence($sequence);
+            ->setSequence($sequence)
+            ->save();
+
           $xactions[] = id(new PholioTransaction())
-            ->setTransactionType(
-              PholioImageReplaceTransaction::TRANSACTIONTYPE)
-            ->setNewValue($replace_image);
+            ->setTransactionType(PholioImageReplaceTransaction::TRANSACTIONTYPE)
+            ->setNewValue($replace_image->getPHID());
+
           $posted_mock_images[] = $replace_image;
         } else if (!$existing_image) { // this is an add
           $add_image = PholioImage::initializeNewImage()
             ->setAuthorPHID($viewer->getPHID())
-            ->setFilePhid($file_phid)
+            ->setFilePHID($file_phid)
             ->attachFile($file)
             ->setName(strlen($title) ? $title : $file->getName())
             ->setDescription($description)
@@ -202,7 +204,6 @@ final class PholioMockEditController extends PholioController {
           ->setMetadataValue('edge:type', $proj_edge_type)
           ->setNewValue(array('=' => array_fuse($v_projects)));
 
-        $mock->openTransaction();
         $editor = id(new PholioMockEditor())
           ->setContentSourceFromRequest($request)
           ->setContinueOnNoEffect(true)
@@ -210,8 +211,6 @@ final class PholioMockEditController extends PholioController {
 
         $xactions = $editor->applyTransactions($mock, $xactions);
 
-        $mock->saveTransaction();
-
         return id(new AphrontRedirectResponse())
           ->setURI('/M'.$mock->getID());
       }

src/applications/pholio/editor/PholioMockEditor.php

@@ -4,6 +4,8 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor {
 
   private $newImages = array();
 
+  private $images = array();
+
   public function getEditorApplicationClass() {
     return 'PhabricatorPholioApplication';
   }
@@ -48,9 +50,7 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor {
     foreach ($xactions as $xaction) {
       switch ($xaction->getTransactionType()) {
         case PholioImageFileTransaction::TRANSACTIONTYPE:
-        case PholioImageReplaceTransaction::TRANSACTIONTYPE:
           return true;
-          break;
       }
     }
     return false;
@@ -75,11 +75,6 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor {
             }
           }
           break;
-        case PholioImageReplaceTransaction::TRANSACTIONTYPE:
-          $image = $xaction->getNewValue();
-          $image->save();
-          $new_images[] = $image;
-          break;
       }
     }
     $this->setNewImages($new_images);
@@ -275,4 +270,37 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor {
     return parent::shouldImplyCC($object, $xaction);
   }
 
+  public function loadPholioImage($object, $phid) {
+    if (!isset($this->images[$phid])) {
+
+      $image = id(new PholioImageQuery())
+        ->setViewer($this->getActor())
+        ->withPHIDs(array($phid))
+        ->executeOne();
+
+      if (!$image) {
+        throw new Exception(
+          pht(
+            'No image exists with PHID "%s".',
+            $phid));
+      }
+
+      $mock_phid = $image->getMockPHID();
+      if ($mock_phid) {
+        if ($mock_phid !== $object->getPHID()) {
+          throw new Exception(
+            pht(
+              'Image ("%s") belongs to the wrong object ("%s", expected "%s").',
+              $phid,
+              $mock_phid,
+              $object->getPHID()));
+        }
+      }
+
+      $this->images[$phid] = $image;
+    }
+
+    return $this->images[$phid];
+  }
+
 }

src/applications/pholio/xaction/PholioImageReplaceTransaction.php

@@ -6,25 +6,25 @@ final class PholioImageReplaceTransaction
   const TRANSACTIONTYPE = 'image-replace';
 
   public function generateOldValue($object) {
-    $new_image = $this->getNewValue();
-    return $new_image->getReplacesImagePHID();
+    $editor = $this->getEditor();
+    $new_phid = $this->getNewValue();
+
+    return $editor->loadPholioImage($object, $new_phid)
+      ->getReplacesImagePHID();
   }
 
-  public function generateNewValue($object, $value) {
-    return $value->getPHID();
-  }
+  public function applyExternalEffects($object, $value) {
+    $editor = $this->getEditor();
+    $old_phid = $this->getOldValue();
 
-  public function applyInternalEffects($object, $value) {
-    $old = $this->getOldValue();
-    $images = $object->getImages();
-    foreach ($images as $seq => $image) {
-      if ($image->getPHID() == $old) {
-        $image->setIsObsolete(1);
-        $image->save();
-        unset($images[$seq]);
-      }
-    }
-    $object->attachImages($images);
+    $old_image = $editor->loadPholioImage($object, $old_phid)
+      ->setIsObsolete(1)
+      ->save();
+
+    $editor->loadPholioImage($object, $value)
+      ->setMockPHID($object->getPHID())
+      ->setSequence($old_image->getSequence())
+      ->save();
   }
 
   public function getTitle() {
@@ -54,27 +54,87 @@ final class PholioImageReplaceTransaction
     $object,
     PhabricatorApplicationTransaction $u,
     PhabricatorApplicationTransaction $v) {
-    $u_img = $u->getNewValue();
-    $v_img = $v->getNewValue();
-    if ($u_img->getReplacesImagePHID() == $v_img->getReplacesImagePHID()) {
+
+    $u_phid = $u->getOldValue();
+    $v_phid = $v->getOldValue();
+
+    if ($u_phid === $v_phid) {
       return $v;
     }
+
+    return null;
   }
 
   public function extractFilePHIDs($object, $value) {
-    $file_phids = array();
+    $editor = $this->getEditor();
+
+    $file_phid = $editor->loadPholioImage($object, $value)
+      ->getFilePHID();
+
+    return array($file_phid);
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    $mock_phid = $object->getPHID();
 
     $editor = $this->getEditor();
-    $images = $editor->getNewImages();
-    foreach ($images as $image) {
-      if ($image->getPHID() !== $value) {
+    foreach ($xactions as $xaction) {
+      $new_phid = $xaction->getNewValue();
+
+      try {
+        $new_image = $editor->loadPholioImage($object, $new_phid);
+      } catch (Exception $ex) {
+        $errors[] = $this->newInvalidError(
+          pht(
+            'Unable to load replacement image ("%s"): %s',
+            $new_phid,
+            $ex->getMessage()),
+          $xaction);
         continue;
       }
 
-      $file_phids[] = $image->getFilePHID();
+      $old_phid = $new_image->getReplacesImagePHID();
+      if (!$old_phid) {
+        $errors[] = $this->newInvalidError(
+          pht(
+            'Image ("%s") does not specify which image it replaces.',
+            $new_phid),
+          $xaction);
+        continue;
+      }
+
+      try {
+        $old_image = $editor->loadPholioImage($object, $old_phid);
+      } catch (Exception $ex) {
+        $errors[] = $this->newInvalidError(
+          pht(
+            'Unable to load replaced image ("%s"): %s',
+            $old_phid,
+            $ex->getMessage()),
+          $xaction);
+        continue;
+      }
+
+      if ($old_image->getMockPHID() !== $mock_phid) {
+        $errors[] = $this->newInvalidError(
+          pht(
+            'Replaced image ("%s") belongs to the wrong mock ("%s", expected '.
+            '"%s").',
+            $old_phid,
+            $old_image->getMockPHID(),
+            $mock_phid),
+          $xaction);
+        continue;
+      }
+
+      // TODO: You shouldn't be able to replace an image if it has already
+      // been replaced.
+
     }
 
-    return $file_phids;
+    return $errors;
   }
 
 }