Fix improper selection of the chunk engine as a writable engine

Summary:
Fixes T7621. The engine selection code started out making sense, but didn't make as much sense by the time I was done with it.

Specifically, from the vanilla file upload, we may incorrectly try to write directly to the chunk storage engine. This is incorrect, and produces a confusing/bad error.

Make chunk storage engines explicit and don't try to do single-file one-shot writes to them.

Test Plan:
  - Tried to upload a large file with vanilla uploader, got better error message.
  - Uploaded small and large files with drag and drop.
  - Viewed {nav Files > Help/Options}.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7621

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

src/applications/config/check/PhabricatorStorageSetupCheck.php

@@ -10,15 +10,8 @@ final class PhabricatorStorageSetupCheck extends PhabricatorSetupCheck {
    * @phutil-external-symbol class PhabricatorStartup
    */
   protected function executeChecks() {
-    $chunk_engine_active = false;
+    $engines = PhabricatorFileStorageEngine::loadWritableChunkEngines();
-
+    $chunk_engine_active = (bool)$engines;
-    $engines = PhabricatorFileStorageEngine::loadWritableEngines();
-    foreach ($engines as $engine) {
-      if ($engine->isChunkEngine()) {
-        $chunk_engine_active = true;
-        break;
-      }
-    }
 
     if (!$chunk_engine_active) {
       $doc_href = PhabricatorEnv::getDocLink('Configuring File Storage');

src/applications/files/applicationpanel/PhabricatorFilesApplicationStorageEnginePanel.php

@@ -18,6 +18,7 @@ final class PhabricatorFilesApplicationStorageEnginePanel
 
     $engines = PhabricatorFileStorageEngine::loadAllEngines();
     $writable_engines = PhabricatorFileStorageEngine::loadWritableEngines();
+    $chunk_engines = PhabricatorFileStorageEngine::loadWritableChunkEngines();
 
     $yes = pht('Yes');
     $no = pht('No');
@@ -44,7 +45,7 @@ final class PhabricatorFilesApplicationStorageEnginePanel
         $test = $no;
       }
 
-      if (isset($writable_engines[$key])) {
+      if (isset($writable_engines[$key]) || isset($chunk_engines[$key])) {
         $rowc[] = 'highlighted';
       } else {
         $rowc[] = null;

src/applications/files/conduit/FileAllocateConduitAPIMethod.php

@@ -85,24 +85,20 @@ final class FileAllocateConduitAPIMethod
       );
     }
 
+    // If there are any non-chunk engines which this file can fit into,
+    // just tell the client to upload the file.
     $engines = PhabricatorFileStorageEngine::loadStorageEngines($length);
     if ($engines) {
-
-      // Pick the first engine. If the file is small enough to fit into a
-      // single engine without chunking, this will be a non-chunk engine and
-      // we'll just tell the client to upload the file.
-      $engine = head($engines);
-      if ($engine) {
-        if (!$engine->isChunkEngine()) {
       return array(
         'upload' => true,
         'filePHID' => null,
       );
     }
 
-        // Otherwise, this is a large file and we need to perform a chunked
+    // Otherwise, this is a large file and we want to perform a chunked
-        // upload.
+    // upload if we have a chunk engine available.
-
+    $chunk_engines = PhabricatorFileStorageEngine::loadWritableChunkEngines();
+    if ($chunk_engines) {
       $chunk_properties = $properties;
 
       if ($hash) {
@@ -111,14 +107,14 @@ final class FileAllocateConduitAPIMethod
         );
       }
 
-        $file = $engine->allocateChunks($length, $chunk_properties);
+      $chunk_engine = head($chunk_engines);
+      $file = $chunk_engine->allocateChunks($length, $chunk_properties);
 
       return array(
         'upload' => true,
         'filePHID' => $file->getPHID(),
       );
     }
-    }
 
     // None of the storage engines can accept this file.
     if (PhabricatorFileStorageEngine::loadWritableEngines()) {

src/applications/files/engine/PhabricatorFileStorageEngine.php

@@ -12,6 +12,7 @@
  * @task construct Constructing an Engine
  * @task meta Engine Metadata
  * @task file Managing File Data
+ * @task load Loading Storage Engines
  */
 abstract class PhabricatorFileStorageEngine {
 
@@ -188,12 +189,17 @@ abstract class PhabricatorFileStorageEngine {
   abstract public function deleteFile($handle);
 
 
+
+/* -(  Loading Storage Engines  )-------------------------------------------- */
+
+
   /**
    * Select viable default storage engines according to configuration. We'll
    * select the MySQL and Local Disk storage engines if they are configured
    * to allow a given file.
    *
    * @param int File size in bytes.
+   * @task load
    */
   public static function loadStorageEngines($length) {
     $engines = self::loadWritableEngines();
@@ -213,6 +219,10 @@ abstract class PhabricatorFileStorageEngine {
     return $writable;
   }
 
+
+  /**
+   * @task load
+   */
   public static function loadAllEngines() {
     static $engines;
 
@@ -246,28 +256,72 @@ abstract class PhabricatorFileStorageEngine {
     return $engines;
   }
 
-  public static function loadWritableEngines() {
+
+  /**
+   * @task load
+   */
+  private static function loadProductionEngines() {
     $engines = self::loadAllEngines();
 
-    $writable = array();
+    $active = array();
     foreach ($engines as $key => $engine) {
       if ($engine->isTestEngine()) {
         continue;
       }
 
+      $active[$key] = $engine;
+    }
+
+    return $active;
+  }
+
+
+  /**
+   * @task load
+   */
+  public static function loadWritableEngines() {
+    $engines = self::loadProductionEngines();
+
+    $writable = array();
+    foreach ($engines as $key => $engine) {
       if (!$engine->canWriteFiles()) {
         continue;
       }
 
+      if ($engine->isChunkEngine()) {
+        // Don't select chunk engines as writable.
+        continue;
+      }
       $writable[$key] = $engine;
     }
 
     return $writable;
   }
 
+  /**
+   * @task load
+   */
+  public static function loadWritableChunkEngines() {
+    $engines = self::loadProductionEngines();
+
+    $chunk = array();
+    foreach ($engines as $key => $engine) {
+      if (!$engine->canWriteFiles()) {
+        continue;
+      }
+      if (!$engine->isChunkEngine()) {
+        continue;
+      }
+      $chunk[$key] = $engine;
+    }
+
+    return $chunk;
+  }
+
+
 
   /**
-   * Return the largest file size which can be uploaded without chunking.
+   * Return the largest file size which can not be uploaded in chunks.
    *
    * Files smaller than this will always upload in one request, so clients
    * can safely skip the allocation step.
@@ -275,14 +329,10 @@ abstract class PhabricatorFileStorageEngine {
    * @return int|null Byte size, or `null` if there is no chunk support.
    */
   public static function getChunkThreshold() {
-    $engines = self::loadWritableEngines();
+    $engines = self::loadWritableChunkEngines();
 
     $min = null;
     foreach ($engines as $engine) {
-      if (!$engine->isChunkEngine()) {
-        continue;
-      }
-
       if (!$min) {
         $min = $engine;
         continue;