Modularize Differential Diff transactions and attach binaries

Summary:
Fully modularizing DifferentialDiffTransaction, and then fix T16472 by attaching any binary files uploaded to the diff object.

Refs T16485 T16070.

Test Plan:
Set the default view-policy of new files to something that isn't very public.
Create new diffs using `arc diff` that includes a picture of a small cat. Other kinds of images are out of scope.

- Users that don't have permission to see the revision should not be able to see the file
- Users that have permission to see the revision, should be able to see the file.
- Changing the policies on the created file to "no one" will not be make a difference.
- The file's page will show the diff in the Attachment list.

Also:
- Herald can still block the creation of the diffs.

Reviewers: O1 Blessed Committers, valerio.bozzolan

Reviewed By: O1 Blessed Committers, valerio.bozzolan

Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno, jdelker

Maniphest Tasks: T16485, T16472, T16070

Differential Revision: https://we.phorge.it/D26724

src/__phutil_library_map__.php

···
     'DifferentialDiffContentHeraldField' => 'applications/differential/herald/DifferentialDiffContentHeraldField.php',
     'DifferentialDiffContentRemovedHeraldField' => 'applications/differential/herald/DifferentialDiffContentRemovedHeraldField.php',
     'DifferentialDiffCreateController' => 'applications/differential/controller/DifferentialDiffCreateController.php',
+    'DifferentialDiffCreateTransaction' => 'applications/differential/xaction/DifferentialDiffCreateTransaction.php',
     'DifferentialDiffEditor' => 'applications/differential/editor/DifferentialDiffEditor.php',
     'DifferentialDiffExtractionEngine' => 'applications/differential/engine/DifferentialDiffExtractionEngine.php',
     'DifferentialDiffHeraldField' => 'applications/differential/herald/DifferentialDiffHeraldField.php',
···
     'DifferentialDiffTestCase' => 'applications/differential/storage/__tests__/DifferentialDiffTestCase.php',
     'DifferentialDiffTransaction' => 'applications/differential/storage/DifferentialDiffTransaction.php',
     'DifferentialDiffTransactionQuery' => 'applications/differential/query/DifferentialDiffTransactionQuery.php',
+    'DifferentialDiffTransactionType' => 'applications/differential/xaction/DifferentialDiffTransactionType.php',
     'DifferentialDiffViewController' => 'applications/differential/controller/DifferentialDiffViewController.php',
     'DifferentialDoorkeeperRevisionFeedStoryPublisher' => 'applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php',
     'DifferentialDraftField' => 'applications/differential/customfield/DifferentialDraftField.php',
···
     'DifferentialDiffContentHeraldField' => 'DifferentialDiffHeraldField',
     'DifferentialDiffContentRemovedHeraldField' => 'DifferentialDiffHeraldField',
     'DifferentialDiffCreateController' => 'DifferentialController',
+    'DifferentialDiffCreateTransaction' => 'DifferentialDiffTransactionType',
     'DifferentialDiffEditor' => 'PhabricatorApplicationTransactionEditor',
     'DifferentialDiffExtractionEngine' => 'Phobject',
     'DifferentialDiffHeraldField' => 'HeraldField',
···
     'DifferentialDiffSearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod',
     'DifferentialDiffSearchEngine' => 'PhabricatorApplicationSearchEngine',
     'DifferentialDiffTestCase' => 'PhabricatorTestCase',
-    'DifferentialDiffTransaction' => 'PhabricatorApplicationTransaction',
+    'DifferentialDiffTransaction' => 'PhabricatorModularTransaction',
     'DifferentialDiffTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
+    'DifferentialDiffTransactionType' => 'PhabricatorModularTransactionType',
     'DifferentialDiffViewController' => 'DifferentialController',
     'DifferentialDoorkeeperRevisionFeedStoryPublisher' => 'DoorkeeperFeedStoryPublisher',
     'DifferentialDraftField' => 'DifferentialCoreCustomField',

src/applications/differential/editor/DifferentialDiffEditor.php

···
 final class DifferentialDiffEditor
   extends PhabricatorApplicationTransactionEditor {
 
-  private $diffDataDict;
   private $lookupRepository = true;
 
   public function setLookupRepository($bool) {
···
     return $types;
   }
 
-  protected function getCustomTransactionOldValue(
+  protected function didApplyInternalEffects(
     PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case DifferentialDiffTransaction::TYPE_DIFF_CREATE:
-        return null;
-    }
+    array $xactions) {
 
-    return parent::getCustomTransactionOldValue($object, $xaction);
-  }
+    // This method wasn't modularized.
 
-  protected function getCustomTransactionNewValue(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case DifferentialDiffTransaction::TYPE_DIFF_CREATE:
-        $this->diffDataDict = $xaction->getNewValue();
-        return true;
-    }
-
-    return parent::getCustomTransactionNewValue($object, $xaction);
-  }
-
-  protected function applyCustomInternalTransaction(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case DifferentialDiffTransaction::TYPE_DIFF_CREATE:
-        $dict = $this->diffDataDict;
-        $this->updateDiffFromDict($object, $dict);
-        return;
+    foreach ($xactions as $xaction) {
+      switch ($xaction->getTransactionType()) {
+        case DifferentialDiffTransaction::TYPE_DIFF_CREATE:
+          $xaction->setNewValue(true);
+          break;
+      }
     }
 
-    return parent::applyCustomInternalTransaction($object, $xaction);
+    return $xactions;
   }
 
-  protected function applyCustomExternalTransaction(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case DifferentialDiffTransaction::TYPE_DIFF_CREATE:
-        return;
-    }
-
-    return parent::applyCustomExternalTransaction($object, $xaction);
-    }
 
   protected function applyFinalEffects(
     PhabricatorLiskDAO $object,
···
     return $xactions;
   }
 
-  /**
-   * We run Herald as part of transaction validation because Herald can
-   * block diff creation for Differential diffs. Its important to do this
-   * separately so no Herald logs are saved; these logs could expose
-   * information the Herald rules are intended to block.
-   */
-  protected function validateTransaction(
-    PhabricatorLiskDAO $object,
-    $type,
-    array $xactions) {
-
-    $errors = parent::validateTransaction($object, $type, $xactions);
-
-    foreach ($xactions as $xaction) {
-      switch ($type) {
-        case DifferentialDiffTransaction::TYPE_DIFF_CREATE:
-          $diff = clone $object;
-          $diff = $this->updateDiffFromDict($diff, $xaction->getNewValue());
-
-          $adapter = $this->buildHeraldAdapter($diff, $xactions);
-          $adapter->setContentSource($this->getContentSource());
-          $adapter->setIsNewObject($this->getIsNewObject());
-
-          $engine = new HeraldEngine();
-
-          $rules = $engine->loadRulesForAdapter($adapter);
-          $rules = mpull($rules, null, 'getID');
-
-          $effects = $engine->applyRules($rules, $adapter);
-          $action_block = DifferentialBlockHeraldAction::ACTIONCONST;
-
-          $blocking_effect = null;
-          foreach ($effects as $effect) {
-            if ($effect->getAction() == $action_block) {
-              $blocking_effect = $effect;
-              break;
-            }
-          }
-
-          if ($blocking_effect) {
-            $rule = $blocking_effect->getRule();
-
-            $message = $effect->getTarget();
-            if (!strlen($message)) {
-              $message = pht('(None.)');
-            }
-
-            $errors[] = new PhabricatorApplicationTransactionValidationError(
-              $type,
-              pht('Rejected by Herald'),
-              pht(
-                "Creation of this diff was rejected by Herald rule %s.\n".
-                "  Rule: %s\n".
-                "Reason: %s",
-                $rule->getMonogram(),
-                $rule->getName(),
-                $message));
-          }
-          break;
-      }
-    }
-
-    return $errors;
-  }
-
-
-  protected function shouldPublishFeedStory(
-    PhabricatorLiskDAO $object,
-    array $xactions) {
-    return false;
-  }
-
-  protected function shouldSendMail(
-    PhabricatorLiskDAO $object,
-    array $xactions) {
-    return false;
-  }
-
-  protected function supportsSearch() {
-    return false;
-  }
-
 /* -(  Herald Integration  )------------------------------------------------- */
 
   /**
···
     return false;
   }
 
-  protected function buildHeraldAdapter(
-    PhabricatorLiskDAO $object,
-    array $xactions) {
-
-    $adapter = id(new HeraldDifferentialDiffAdapter())
-      ->setDiff($object);
-
-    return $adapter;
-  }
-
-  private function updateDiffFromDict(DifferentialDiff $diff, $dict) {
-    $diff
-      ->setSourcePath(idx($dict, 'sourcePath'))
-      ->setSourceMachine(idx($dict, 'sourceMachine'))
-      ->setBranch(idx($dict, 'branch'))
-      ->setCreationMethod(idx($dict, 'creationMethod'))
-      ->setAuthorPHID(idx($dict, 'authorPHID', $this->getActor()))
-      ->setBookmark(idx($dict, 'bookmark'))
-      ->setRepositoryPHID(idx($dict, 'repositoryPHID'))
-      ->setRepositoryUUID(idx($dict, 'repositoryUUID'))
-      ->setSourceControlSystem(idx($dict, 'sourceControlSystem'))
-      ->setSourceControlPath(idx($dict, 'sourceControlPath'))
-      ->setSourceControlBaseRevision(idx($dict, 'sourceControlBaseRevision'))
-      ->setLintStatus(idx($dict, 'lintStatus'))
-      ->setUnitStatus(idx($dict, 'unitStatus'));
-
-    return $diff;
-  }
 }

src/applications/differential/storage/DifferentialDiff.php

···
     return self::buildChangesetsFromRawChanges($diff, $changes);
   }
 
+  /**
+   * @param array<ArcanistDiffChange> $changes
+   */
   private static function buildChangesetsFromRawChanges(
     DifferentialDiff $diff,
     array $changes) {

src/applications/differential/storage/DifferentialDiffTransaction.php

···
 <?php
 
 final class DifferentialDiffTransaction
-  extends PhabricatorApplicationTransaction {
+  extends PhabricatorModularTransaction {
 
   const TYPE_DIFF_CREATE = 'differential:diff:create';
+
+  public function getBaseTransactionClass() {
+    return DifferentialDiffTransactionType::class;
+  }
 
   public function getApplicationName() {
     return 'differential';
···
 
   public function getApplicationTransactionType() {
     return DifferentialDiffPHIDType::TYPECONST;
-  }
-
-  public function shouldHideForMail(array $xactions) {
-    return true;
-  }
-
-  public function getActionName() {
-    switch ($this->getTransactionType()) {
-      case self::TYPE_DIFF_CREATE:
-        return pht('Created');
-    }
-
-    return parent::getActionName();
-  }
-
-  public function getTitle() {
-    $author_phid = $this->getAuthorPHID();
-    $author_handle = $this->renderHandleLink($author_phid);
-
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    switch ($this->getTransactionType()) {
-      case self::TYPE_DIFF_CREATE:
-        return pht(
-          '%s created this diff.',
-          $author_handle);
-    }
-
-    return parent::getTitle();
-  }
-
-  public function getIcon() {
-    switch ($this->getTransactionType()) {
-      case self::TYPE_DIFF_CREATE:
-        return 'fa-refresh';
-    }
-
-    return parent::getIcon();
-  }
-
-  public function getColor() {
-    switch ($this->getTransactionType()) {
-      case self::TYPE_DIFF_CREATE:
-        return PhabricatorTransactions::COLOR_SKY;
-    }
-
-    return parent::getColor();
   }
 
 }

src/applications/differential/xaction/DifferentialDiffCreateTransaction.php

···
+<?php
+
+final class DifferentialDiffCreateTransaction
+  extends DifferentialDiffTransactionType {
+
+  const TRANSACTIONTYPE = DifferentialDiffTransaction::TYPE_DIFF_CREATE;
+
+  public function shouldHide() {
+    return true;
+  }
+
+  public function generateOldValue($object) {
+    return null;
+  }
+
+  public function getActionName() {
+    return pht('Created');
+  }
+
+  public function getTitle() {
+    return pht('%s created this diff.', $this->renderAuthor());
+  }
+
+  public function getIcon() {
+    return 'fa-refresh';
+  }
+
+  public function getColor() {
+    return PhabricatorTransactions::COLOR_SKY;
+  }
+
+  public function extractFilePHIDs($object, $value) {
+
+    /** @var array<DifferentialChangeset> $changesets */
+    $changesets = $object->getChangesets();
+
+    $file_phids = array();
+    foreach ($changesets as $change) {
+      $file_phids[] = $change->getNewFileObjectPHID();
+      $file_phids[] = $change->getOldFileObjectPHID();
+    }
+
+    return array_filter($file_phids);
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $this->updateDiffFromDict($object, $value);
+  }
+
+
+  /**
+   * We run Herald as part of transaction validation because Herald can
+   * block diff creation for Differential diffs. Its important to do this
+   * separately so no Herald logs are saved; these logs could expose
+   * information the Herald rules are intended to block.
+   */
+  public function validateTransactions($object, array $xactions) {
+
+    $errors = array();
+
+    foreach ($xactions as $xaction) {
+      $diff = clone $object;
+      $diff = $this->updateDiffFromDict($diff, $xaction->getNewValue());
+
+      $adapter = $this->buildHeraldAdapter($diff, $xactions);
+      $adapter->setContentSource($xaction->getContentSource());
+      $adapter->setIsNewObject(true);
+
+      $engine = new HeraldEngine();
+
+      $rules = $engine->loadRulesForAdapter($adapter);
+      $rules = mpull($rules, null, 'getID');
+
+      $effects = $engine->applyRules($rules, $adapter);
+      $action_block = DifferentialBlockHeraldAction::ACTIONCONST;
+
+      $blocking_effect = null;
+      foreach ($effects as $effect) {
+        if ($effect->getAction() == $action_block) {
+          $blocking_effect = $effect;
+          break;
+        }
+      }
+
+      if ($blocking_effect) {
+        $rule = $blocking_effect->getRule();
+
+        $message = $blocking_effect->getTarget();
+        if (!is_string($message) || !strlen($message)) {
+          $message = pht('(None.)');
+        }
+
+        $errors[] = $this->newError(
+          pht('Rejected by Herald'),
+          pht(
+            "Creation of this diff was rejected by Herald rule %s.\n".
+            "  Rule: %s\n".
+            "Reason: %s",
+            $rule->getMonogram(),
+            $rule->getName(),
+            $message));
+      }
+    }
+
+    return $errors;
+  }
+
+  private function buildHeraldAdapter($object) {
+
+    return id(new HeraldDifferentialDiffAdapter())
+      ->setDiff($object);
+  }
+
+  private function updateDiffFromDict(DifferentialDiff $diff, $dict) {
+    $diff
+      ->setSourcePath(idx($dict, 'sourcePath'))
+      ->setSourceMachine(idx($dict, 'sourceMachine'))
+      ->setBranch(idx($dict, 'branch'))
+      ->setCreationMethod(idx($dict, 'creationMethod'))
+      ->setAuthorPHID(idx($dict, 'authorPHID', $this->getActor()))
+      ->setBookmark(idx($dict, 'bookmark'))
+      ->setRepositoryPHID(idx($dict, 'repositoryPHID'))
+      ->setRepositoryUUID(idx($dict, 'repositoryUUID'))
+      ->setSourceControlSystem(idx($dict, 'sourceControlSystem'))
+      ->setSourceControlPath(idx($dict, 'sourceControlPath'))
+      ->setSourceControlBaseRevision(idx($dict, 'sourceControlBaseRevision'))
+      ->setLintStatus(idx($dict, 'lintStatus'))
+      ->setUnitStatus(idx($dict, 'unitStatus'));
+
+    return $diff;
+  }
+}

src/applications/differential/xaction/DifferentialDiffTransactionType.php

···
+<?php
+
+abstract class DifferentialDiffTransactionType
+  extends PhabricatorModularTransactionType {}

src/applications/files/controller/PhabricatorFileViewController.php

···
     } else {
       $curtain->addAction(
         id(new PhabricatorActionView())
-          ->setUser($viewer)
+          ->setViewer($viewer)
           ->setDownload($can_download)
           ->setName(pht('Download File'))
           ->setIcon('fa-download')
···
   }
 
   private function loadStorageEngine(PhabricatorFile $file) {
-    $engine = null;
 
     try {
-      $engine = $file->instantiateStorageEngine();
-    } catch (Exception $ex) {
+      return $file->instantiateStorageEngine();
+    } catch (Throwable $ex) {
       // Don't bother raising this anywhere for now.
     }
 
-    return $engine;
+    return null;
   }
 
   private function newFileContent(PhabricatorFile $file) {
···
     $rows = array();
 
     $mode_map = PhabricatorFileAttachment::getModeNameMap();
-    $mode_attach = PhabricatorFileAttachment::MODE_ATTACH;
 
     foreach ($attachments as $attachment) {
       $object_phid = $attachment->getObjectPHID();
···
         ->setColor(PHUIButtonView::GREY)
         ->setSize(PHUIButtonView::SMALL)
         ->setText(pht('Detach File'));
-
-      javelin_tag(
-        'a',
-        array(
-          'href' => $detach_uri,
-          'sigil' => 'workflow',
-          'disabled' => true,
-          'class' => 'small button button-grey disabled',
-        ),
-        pht('Detach File'));
 
       $rows[] = array(
         $handle->renderLink(),

src/applications/files/storage/PhabricatorFile.php

···
     $view_policy = $app->getPolicy(
       FilesDefaultViewCapability::CAPABILITY);
 
-    return id(new PhabricatorFile())
+    return id(new self())
       ->setViewPolicy($view_policy)
       ->setIsPartial(0)
       ->attachOriginalFile(null)
···
         // If an engine is outright misconfigured (or misimplemented), raise
         // that immediately since it probably needs attention.
         throw $ex;
-      } catch (Exception $ex) {
+      } catch (Throwable $ex) {
         phlog($ex);
 
         // If an engine doesn't work, keep trying all the other valid engines
···
 
     try {
       $file->updateDimensions(false);
-    } catch (Exception $ex) {
+    } catch (Throwable $ex) {
       // Do nothing.
     }
 
···
 
           return self::newFromFileData($body, $params);
         }
-      } catch (Exception $ex) {
+      } catch (Throwable $ex) {
         if ($redirects) {
           throw new Exception(
             pht(
···
     $handle) {
 
     // Check to see if any files are using storage.
-    $usage = id(new PhabricatorFile())->loadAllWhere(
+    $usage = id(new self())->loadAllWhere(
       'storageEngine = %s AND storageHandle = %s LIMIT 1',
       $engine_identifier,
       $handle);
···
     if (!$usage) {
       try {
         $engine->deleteFile($handle);
-      } catch (Exception $ex) {
+      } catch (Throwable $ex) {
         // In the worst case, we're leaving some data stranded in a storage
         // engine, which is not a big deal.
         phlog($ex);

src/applications/herald/engine/HeraldEffect.php

···
   }
 
   /**
-   * @return array|null
+   * @return mixed
    */
   public function getTarget() {
     return $this->target;