Consolidate handling of special properties for newly uploaded files

Summary:
Fixes T5849. When a new file is created, we might have to actually write the data to a storage engine, or we might be able to just point at data which is already there.

Currently, these two paths handle `$params` with different code and mild behavioral differences. Instead, have them call the same code so they get the same behavior.

Test Plan:
  - Uploaded the same file multiple times to home page.
  - Uploaded the same file multiple times as profile picture.
  - Generated files via Diffusion.
  - All the files got the expected properties, whether they were reusing data or not.

Reviewers: btrahan, 20after4

Reviewed By: 20after4

Subscribers: epriestley

Maniphest Tasks: T5849

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

src/applications/files/storage/PhabricatorFile.php

@@ -1,5 +1,21 @@
 <?php
 
+/**
+ * Parameters
+ * ==========
+ *
+ * When creating a new file using a method like @{method:newFromFileData}, these
+ * parameters are supported:
+ *
+ *   | name | Human readable filename.
+ *   | authorPHID | User PHID of uploader.
+ *   | ttl | Temporary file lifetime, in seconds.
+ *   | viewPolicy | File visibility policy.
+ *   | isExplicitUpload | Used to show users files they explicitly uploaded.
+ *   | canCDN | Allows the file to be cached and delivered over a CDN.
+ *   | mime-type | Optional, explicit file MIME type.
+ *
+ */
 final class PhabricatorFile extends PhabricatorFileDAO
   implements
     PhabricatorTokenReceiverInterface,
@@ -159,10 +175,11 @@ final class PhabricatorFile extends PhabricatorFileDAO
     return $file;
   }
 
-  public static function newFileFromContentHash($hash, $params) {
+  public static function newFileFromContentHash($hash, array $params) {
     // Check to see if a file with same contentHash exist
     $file = id(new PhabricatorFile())->loadOneWhere(
-      'contentHash = %s LIMIT 1', $hash);
+      'contentHash = %s LIMIT 1',
+      $hash);
 
     if ($file) {
       // copy storageEngine, storageHandle, storageFormat
@@ -172,21 +189,9 @@ final class PhabricatorFile extends PhabricatorFileDAO
       $copy_of_byteSize = $file->getByteSize();
       $copy_of_mimeType = $file->getMimeType();
 
-      $file_name = idx($params, 'name');
-      $file_name = self::normalizeFileName($file_name);
-      $file_ttl = idx($params, 'ttl');
-      $authorPHID = idx($params, 'authorPHID');
-
       $new_file = new PhabricatorFile();
 
-      $new_file->setName($file_name);
       $new_file->setByteSize($copy_of_byteSize);
-      $new_file->setAuthorPHID($authorPHID);
-      $new_file->setTtl($file_ttl);
-
-      if (idx($params, 'viewPolicy')) {
-        $new_file->setViewPolicy($params['viewPolicy']);
-      }
 
       $new_file->setContentHash($hash);
       $new_file->setStorageEngine($copy_of_storage_engine);
@@ -195,6 +200,8 @@ final class PhabricatorFile extends PhabricatorFileDAO
       $new_file->setMimeType($copy_of_mimeType);
       $new_file->copyDimensions($file);
 
+      $new_file->readPropertiesFromParameters($params);
+
       $new_file->save();
 
       return $new_file;
@@ -253,39 +260,19 @@ final class PhabricatorFile extends PhabricatorFileDAO
         $exceptions);
     }
 
-    $file_name = idx($params, 'name');
-    $file_name = self::normalizeFileName($file_name);
-    $file_ttl = idx($params, 'ttl');
-
-    // If for whatever reason, authorPHID isn't passed as a param
-    // (always the case with newFromFileDownload()), store a ''
-    $authorPHID = idx($params, 'authorPHID');
-
-    $file->setName($file_name);
     $file->setByteSize(strlen($data));
-    $file->setAuthorPHID($authorPHID);
-    $file->setTtl($file_ttl);
     $file->setContentHash(self::hashFileContent($data));
 
-    if (idx($params, 'viewPolicy')) {
-      $file->setViewPolicy($params['viewPolicy']);
-    }
-
-    if (idx($params, 'canCDN')) {
-      $file->setCanCDN(true);
-    }
-
     $file->setStorageEngine($engine_identifier);
     $file->setStorageHandle($data_handle);
 
     // TODO: This is probably YAGNI, but allows for us to do encryption or
     // compression later if we want.
     $file->setStorageFormat(self::STORAGE_FORMAT_RAW);
-    $file->setIsExplicitUpload(idx($params, 'isExplicitUpload') ? 1 : 0);
 
-    if (isset($params['mime-type'])) {
+    $file->readPropertiesFromParameters($params);
-      $file->setMimeType($params['mime-type']);
+
-    } else {
+    if (!$file->getMimeType()) {
       $tmp = new TempFile();
       Filesystem::writeFile($tmp, $data);
       $file->setMimeType(Filesystem::getMimeType($tmp));
@@ -936,6 +923,49 @@ final class PhabricatorFile extends PhabricatorFileDAO
   }
 
 
+  /**
+   * Configure a newly created file object according to specified parameters.
+   *
+   * This method is called both when creating a file from fresh data, and
+   * when creating a new file which reuses existing storage.
+   *
+   * @param map<string, wild>   Bag of parameters, see @{class:PhabricatorFile}
+   *  for documentation.
+   * @return this
+   */
+  private function readPropertiesFromParameters(array $params) {
+    $file_name = idx($params, 'name');
+    $file_name = self::normalizeFileName($file_name);
+    $this->setName($file_name);
+
+    $author_phid = idx($params, 'authorPHID');
+    $this->setAuthorPHID($author_phid);
+
+    $file_ttl = idx($params, 'ttl');
+    $this->setTtl($file_ttl);
+
+    $view_policy = idx($params, 'viewPolicy');
+    if ($view_policy) {
+      $this->setViewPolicy($params['viewPolicy']);
+    }
+
+    $is_explicit = (idx($params, 'isExplicitUpload') ? 1 : 0);
+    $this->setIsExplicitUpload($is_explicit);
+
+    $can_cdn = idx($params, 'canCDN');
+    if ($can_cdn) {
+      $this->setCanCDN(true);
+    }
+
+    $mime_type = idx($params, 'mime-type');
+    if ($mime_type) {
+      $this->setMimeType($mime_type);
+    }
+
+    return $this;
+  }
+
+
 /* -(  PhabricatorPolicyInterface Implementation  )-------------------------- */