Modernize file storage engine selection

Summary:
Fixes T5843. File storage engines use a very old "selector" mechanism which makes them difficult to extend.

This mechanism predates widespread use of `PhutilSymbolLoader` to discover available implementations at runtime. Runtime discovery has generally proven more flexible and easier to use than explicit selection (although it sometimes needs more UI to support it in cases where order or enabled/disabled flags can not be directly determined).

Use a modern runtime discovery mechanism instead of an explicit selector. This might break any installs which subclassed the `Selector`, but I believe almost no such installs exist, and they'll receive a meaningful exception upon upgrading (any custom engines will no longer implement all of the required methods).

Looking forward, this modernizes infrastructure to prepare for new "virtual" chunked-storage engines, with the eventual goal of supporting very large file uploads and data import into the Phacility cluster.

This uses D12051 to add UI to make it easier to understand the state of storage engines.

Test Plan:
Used new UI panel to assess storage engines:

{F336270}

  - Uploaded a small file, saw it go to MySQL engine.
  - Uploaded a larger file, saw it go to S3 engine.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5843

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

resources/celerity/map.php

@@ -7,7 +7,7 @@
  */
 return array(
   'names' => array(
-    'core.pkg.css' => '70320e8a',
+    'core.pkg.css' => 'efdeeb14',
     'core.pkg.js' => '5f50c01b',
     'darkconsole.pkg.js' => '8ab24e01',
     'differential.pkg.css' => '1940be3f',
@@ -34,7 +34,7 @@ return array(
     'rsrc/css/aphront/typeahead.css' => '0e403212',
     'rsrc/css/application/almanac/almanac.css' => 'dbb9b3af',
     'rsrc/css/application/auth/auth.css' => '1e655982',
-    'rsrc/css/application/base/main-menu-view.css' => 'f9f5cd1b',
+    'rsrc/css/application/base/main-menu-view.css' => '58db7ad2',
     'rsrc/css/application/base/notification-menu.css' => '6aa0a74b',
     'rsrc/css/application/base/phabricator-application-launch-view.css' => '16ca323f',
     'rsrc/css/application/base/standard-page-view.css' => 'df338a4b',
@@ -44,7 +44,7 @@ return array(
     'rsrc/css/application/config/config-welcome.css' => '6abd79be',
     'rsrc/css/application/config/setup-issue.css' => '22270af2',
     'rsrc/css/application/config/unhandled-exception.css' => '37d4f9a2',
-    'rsrc/css/application/conpherence/durable-column.css' => '7abcc3f2',
+    'rsrc/css/application/conpherence/durable-column.css' => 'acefcb30',
     'rsrc/css/application/conpherence/menu.css' => 'c6ac5299',
     'rsrc/css/application/conpherence/message-pane.css' => '5930260a',
     'rsrc/css/application/conpherence/notification.css' => '04a6e10a',
@@ -105,12 +105,12 @@ return array(
     'rsrc/css/application/tokens/tokens.css' => '3d0f239e',
     'rsrc/css/application/uiexample/example.css' => '528b19de',
     'rsrc/css/core/core.css' => '86bfbe8c',
-    'rsrc/css/core/remarkup.css' => '2dbff225',
+    'rsrc/css/core/remarkup.css' => 'bc65f3cc',
     'rsrc/css/core/syntax.css' => '56c1ba38',
     'rsrc/css/core/z-index.css' => '2db67397',
     'rsrc/css/diviner/diviner-shared.css' => '38813222',
     'rsrc/css/font/font-awesome.css' => 'ae9a7b4d',
-    'rsrc/css/font/font-source-sans-pro.css' => '0d859f60',
+    'rsrc/css/font/font-source-sans-pro.css' => '4a2430d7',
     'rsrc/css/font/phui-font-icon-base.css' => '3dad2ae3',
     'rsrc/css/layout/phabricator-filetree-view.css' => 'fccf9f82',
     'rsrc/css/layout/phabricator-hovercard-view.css' => '893f4783',
@@ -128,7 +128,7 @@ return array(
     'rsrc/css/phui/phui-crumbs-view.css' => '594d719e',
     'rsrc/css/phui/phui-document.css' => '0f83a7df',
     'rsrc/css/phui/phui-feed-story.css' => 'c9f3a0b5',
-    'rsrc/css/phui/phui-fontkit.css' => 'd30f4fa3',
+    'rsrc/css/phui/phui-fontkit.css' => '1fa79503',
     'rsrc/css/phui/phui-form-view.css' => '78d729fe',
     'rsrc/css/phui/phui-form.css' => 'f535f938',
     'rsrc/css/phui/phui-header-view.css' => '083669db',
@@ -352,9 +352,9 @@ return array(
     'rsrc/js/application/aphlict/behavior-aphlict-status.js' => 'ea681761',
     'rsrc/js/application/auth/behavior-persona-login.js' => '9414ff18',
     'rsrc/js/application/config/behavior-reorder-fields.js' => '14a827de',
-    'rsrc/js/application/conpherence/ConpherenceThreadManager.js' => 'efef202b',
+    'rsrc/js/application/conpherence/ConpherenceThreadManager.js' => '0324970d',
-    'rsrc/js/application/conpherence/behavior-durable-column.js' => 'aa3b6c22',
+    'rsrc/js/application/conpherence/behavior-durable-column.js' => '9142e483',
-    'rsrc/js/application/conpherence/behavior-menu.js' => 'e476c952',
+    'rsrc/js/application/conpherence/behavior-menu.js' => 'c4151295',
     'rsrc/js/application/conpherence/behavior-pontificate.js' => '21ba5861',
     'rsrc/js/application/conpherence/behavior-quicksand-blacklist.js' => '7927a7d3',
     'rsrc/js/application/conpherence/behavior-widget-pane.js' => '2c1cd7f5',
@@ -514,11 +514,11 @@ return array(
     'changeset-view-manager' => '88be0133',
     'config-options-css' => '7fedf08b',
     'config-welcome-css' => '6abd79be',
-    'conpherence-durable-column-view' => '7abcc3f2',
+    'conpherence-durable-column-view' => 'acefcb30',
     'conpherence-menu-css' => 'c6ac5299',
     'conpherence-message-pane-css' => '5930260a',
     'conpherence-notification-css' => '04a6e10a',
-    'conpherence-thread-manager' => 'efef202b',
+    'conpherence-thread-manager' => '0324970d',
     'conpherence-update-css' => '1099a660',
     'conpherence-widget-pane-css' => '3d575438',
     'differential-changeset-view-css' => '6a8b172a',
@@ -534,7 +534,7 @@ return array(
     'diffusion-source-css' => '66fdf661',
     'diviner-shared-css' => '38813222',
     'font-fontawesome' => 'ae9a7b4d',
-    'font-source-sans-pro' => '0d859f60',
+    'font-source-sans-pro' => '4a2430d7',
     'global-drag-and-drop-css' => '697324ad',
     'harbormaster-css' => '49d64eb4',
     'herald-css' => '826075fa',
@@ -558,7 +558,7 @@ return array(
     'javelin-behavior-boards-dropdown' => '0ec56e1d',
     'javelin-behavior-choose-control' => '6153c708',
     'javelin-behavior-config-reorder-fields' => '14a827de',
-    'javelin-behavior-conpherence-menu' => 'e476c952',
+    'javelin-behavior-conpherence-menu' => 'c4151295',
     'javelin-behavior-conpherence-pontificate' => '21ba5861',
     'javelin-behavior-conpherence-widget-pane' => '2c1cd7f5',
     'javelin-behavior-countdown-timer' => 'e4cc26b3',
@@ -585,7 +585,7 @@ return array(
     'javelin-behavior-diffusion-locate-file' => '6d3e1947',
     'javelin-behavior-diffusion-pull-lastmodified' => '2b228192',
     'javelin-behavior-doorkeeper-tag' => 'e5822781',
-    'javelin-behavior-durable-column' => 'aa3b6c22',
+    'javelin-behavior-durable-column' => '9142e483',
     'javelin-behavior-error-log' => '6882e80a',
     'javelin-behavior-fancy-datepicker' => 'c51ae228',
     'javelin-behavior-global-drag-and-drop' => '07f199d8',
@@ -730,7 +730,7 @@ return array(
     'phabricator-hovercard-view-css' => '893f4783',
     'phabricator-keyboard-shortcut' => '1ae869f2',
     'phabricator-keyboard-shortcut-manager' => 'c1700f6f',
-    'phabricator-main-menu-view' => 'f9f5cd1b',
+    'phabricator-main-menu-view' => '58db7ad2',
     'phabricator-nav-view-css' => '7aeaf435',
     'phabricator-notification' => '0c6946e7',
     'phabricator-notification-css' => '9c279160',
@@ -739,7 +739,7 @@ return array(
     'phabricator-phtize' => 'd254d646',
     'phabricator-prefab' => '72da38cc',
     'phabricator-profile-css' => '1a20dcbf',
-    'phabricator-remarkup-css' => '2dbff225',
+    'phabricator-remarkup-css' => 'bc65f3cc',
     'phabricator-search-results-css' => '559cc554',
     'phabricator-shaped-request' => '7cbe244b',
     'phabricator-side-menu-view-css' => '7e8c6341',
@@ -783,7 +783,7 @@ return array(
     'phui-document-view-css' => '0f83a7df',
     'phui-feed-story-css' => 'c9f3a0b5',
     'phui-font-icon-base-css' => '3dad2ae3',
-    'phui-fontkit-css' => 'd30f4fa3',
+    'phui-fontkit-css' => '1fa79503',
     'phui-form-css' => 'f535f938',
     'phui-form-view-css' => '78d729fe',
     'phui-header-view-css' => '083669db',
@@ -845,6 +845,16 @@ return array(
     '029a133d' => array(
       'aphront-dialog-view-css',
     ),
+    '0324970d' => array(
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-install',
+      'javelin-workflow',
+      'javelin-router',
+      'javelin-behavior-device',
+      'javelin-vector',
+    ),
     '03d6ed07' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1141,6 +1151,9 @@ return array(
       'javelin-request',
       'javelin-util',
     ),
+    '4a2430d7' => array(
+      'phui-fontkit-css',
+    ),
     '4d94d9c3' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1533,6 +1546,16 @@ return array(
       'javelin-uri',
       'phabricator-notification',
     ),
+    '9142e483' => array(
+      'javelin-behavior',
+      'javelin-dom',
+      'javelin-stratcom',
+      'javelin-behavior-device',
+      'javelin-scrollbar',
+      'javelin-quicksand',
+      'phabricator-keyboard-shortcut',
+      'conpherence-thread-manager',
+    ),
     '92eb531d' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1651,15 +1674,6 @@ return array(
       'javelin-util',
       'phabricator-prefab',
     ),
-    'aa3b6c22' => array(
-      'javelin-behavior',
-      'javelin-dom',
-      'javelin-stratcom',
-      'javelin-scrollbar',
-      'javelin-quicksand',
-      'phabricator-keyboard-shortcut',
-      'conpherence-thread-manager',
-    ),
     'b1f0ccee' => array(
       'javelin-install',
       'javelin-dom',
@@ -1744,6 +1758,18 @@ return array(
       'javelin-dom',
       'javelin-vector',
     ),
+    'c4151295' => array(
+      'javelin-behavior',
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-workflow',
+      'javelin-behavior-device',
+      'javelin-history',
+      'javelin-vector',
+      'phabricator-shaped-request',
+      'conpherence-thread-manager',
+    ),
     'c51ae228' => array(
       'javelin-behavior',
       'javelin-util',
@@ -1847,18 +1873,6 @@ return array(
       'javelin-dom',
       'javelin-uri',
     ),
-    'e476c952' => array(
-      'javelin-behavior',
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-workflow',
-      'javelin-behavior-device',
-      'javelin-history',
-      'javelin-vector',
-      'phabricator-shaped-request',
-      'conpherence-thread-manager',
-    ),
     'e4cc26b3' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1906,16 +1920,6 @@ return array(
       'javelin-install',
       'javelin-util',
     ),
-    'efef202b' => array(
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-install',
-      'javelin-workflow',
-      'javelin-router',
-      'javelin-behavior-device',
-      'javelin-vector',
-    ),
     'f24f3253' => array(
       'javelin-behavior',
       'javelin-dom',

src/__phutil_library_map__.php

@@ -1689,7 +1689,6 @@ phutil_register_library_map(array(
     'PhabricatorDataNotAttachedException' => 'infrastructure/storage/lisk/PhabricatorDataNotAttachedException.php',
     'PhabricatorDatabaseSetupCheck' => 'applications/config/check/PhabricatorDatabaseSetupCheck.php',
     'PhabricatorDebugController' => 'applications/system/controller/PhabricatorDebugController.php',
-    'PhabricatorDefaultFileStorageEngineSelector' => 'applications/files/engineselector/PhabricatorDefaultFileStorageEngineSelector.php',
     'PhabricatorDefaultSearchEngineSelector' => 'applications/search/selector/PhabricatorDefaultSearchEngineSelector.php',
     'PhabricatorDestructibleInterface' => 'applications/system/interface/PhabricatorDestructibleInterface.php',
     'PhabricatorDestructionEngine' => 'applications/system/engine/PhabricatorDestructionEngine.php',
@@ -1813,7 +1812,6 @@ phutil_register_library_map(array(
     'PhabricatorFileStorageBlob' => 'applications/files/storage/PhabricatorFileStorageBlob.php',
     'PhabricatorFileStorageConfigurationException' => 'applications/files/exception/PhabricatorFileStorageConfigurationException.php',
     'PhabricatorFileStorageEngine' => 'applications/files/engine/PhabricatorFileStorageEngine.php',
-    'PhabricatorFileStorageEngineSelector' => 'applications/files/engineselector/PhabricatorFileStorageEngineSelector.php',
     'PhabricatorFileTemporaryGarbageCollector' => 'applications/files/garbagecollector/PhabricatorFileTemporaryGarbageCollector.php',
     'PhabricatorFileTestCase' => 'applications/files/storage/__tests__/PhabricatorFileTestCase.php',
     'PhabricatorFileTestDataGenerator' => 'applications/files/lipsum/PhabricatorFileTestDataGenerator.php',
@@ -1826,6 +1824,7 @@ phutil_register_library_map(array(
     'PhabricatorFileUploadException' => 'applications/files/exception/PhabricatorFileUploadException.php',
     'PhabricatorFileinfoSetupCheck' => 'applications/config/check/PhabricatorFileinfoSetupCheck.php',
     'PhabricatorFilesApplication' => 'applications/files/application/PhabricatorFilesApplication.php',
+    'PhabricatorFilesApplicationStorageEnginePanel' => 'applications/files/applicationpanel/PhabricatorFilesApplicationStorageEnginePanel.php',
     'PhabricatorFilesConfigOptions' => 'applications/files/config/PhabricatorFilesConfigOptions.php',
     'PhabricatorFilesManagementCompactWorkflow' => 'applications/files/management/PhabricatorFilesManagementCompactWorkflow.php',
     'PhabricatorFilesManagementEnginesWorkflow' => 'applications/files/management/PhabricatorFilesManagementEnginesWorkflow.php',
@@ -4977,7 +4976,6 @@ phutil_register_library_map(array(
     'PhabricatorDataNotAttachedException' => 'Exception',
     'PhabricatorDatabaseSetupCheck' => 'PhabricatorSetupCheck',
     'PhabricatorDebugController' => 'PhabricatorController',
-    'PhabricatorDefaultFileStorageEngineSelector' => 'PhabricatorFileStorageEngineSelector',
     'PhabricatorDefaultSearchEngineSelector' => 'PhabricatorSearchEngineSelector',
     'PhabricatorDestructionEngine' => 'Phobject',
     'PhabricatorDeveloperConfigOptions' => 'PhabricatorApplicationConfigOptions',
@@ -5122,6 +5120,7 @@ phutil_register_library_map(array(
     'PhabricatorFileUploadException' => 'Exception',
     'PhabricatorFileinfoSetupCheck' => 'PhabricatorSetupCheck',
     'PhabricatorFilesApplication' => 'PhabricatorApplication',
+    'PhabricatorFilesApplicationStorageEnginePanel' => 'PhabricatorApplicationConfigurationPanel',
     'PhabricatorFilesConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorFilesManagementCompactWorkflow' => 'PhabricatorFilesManagementWorkflow',
     'PhabricatorFilesManagementEnginesWorkflow' => 'PhabricatorFilesManagementWorkflow',

src/applications/config/check/PhabricatorExtraConfigSetupCheck.php

@@ -211,6 +211,9 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck {
       'phd.start-taskmasters' => pht(
         'Taskmasters now use an autoscaling pool. You can configure the '.
         'pool size with `phd.taskmasters`.'),
+      'storage.engine-selector' => pht(
+        'Phabricator now automatically discovers available storage engines '.
+        'at runtime.'),
     );
 
     return $ancient_config;

src/applications/files/applicationpanel/PhabricatorFilesApplicationStorageEnginePanel.php

@@ -0,0 +1,100 @@
+<?php
+
+final class PhabricatorFilesApplicationStorageEnginePanel
+  extends PhabricatorApplicationConfigurationPanel {
+
+  public function getPanelKey() {
+    return 'storage';
+  }
+
+  public function shouldShowForApplication(
+    PhabricatorApplication $application) {
+    return ($application instanceof PhabricatorFilesApplication);
+  }
+
+  public function buildConfigurationPagePanel() {
+    $viewer = $this->getViewer();
+    $application = $this->getApplication();
+
+    $engines = PhabricatorFileStorageEngine::loadAllEngines();
+    $writable_engines = PhabricatorFileStorageEngine::loadWritableEngines();
+
+    $yes = pht('Yes');
+    $no = pht('No');
+
+    $rows = array();
+    $rowc = array();
+    foreach ($engines as $key => $engine) {
+      $limited = $no;
+      $limit = null;
+      if ($engine->hasFilesizeLimit()) {
+        $limited = $yes;
+        $limit = phutil_format_bytes($engine->getFilesizeLimit());
+      }
+
+      if ($engine->canWriteFiles()) {
+        $writable = $yes;
+      } else {
+        $writable = $no;
+      }
+
+      if ($engine->isTestEngine()) {
+        $test = $yes;
+      } else {
+        $test = $no;
+      }
+
+      if (isset($writable_engines[$key])) {
+        $rowc[] = 'highlighted';
+      } else {
+        $rowc[] = null;
+      }
+
+      $rows[] = array(
+        $key,
+        get_class($engine),
+        $test,
+        $writable,
+        $limited,
+        $limit,
+      );
+    }
+
+    $table =
+
+    $table = id(new AphrontTableView($rows))
+      ->setNoDataString(pht('No storage engines available.'))
+      ->setHeaders(
+        array(
+          pht('Key'),
+          pht('Class'),
+          pht('Unit Test'),
+          pht('Writable'),
+          pht('Has Limit'),
+          pht('Limit'),
+        ))
+      ->setRowClasses($rowc)
+      ->setColumnClasses(
+        array(
+          '',
+          'wide',
+          '',
+          '',
+          '',
+          'n',
+        ));
+
+    $box = id(new PHUIObjectBoxView())
+      ->setHeaderText(pht('Storage Engines'))
+      ->appendChild($table);
+
+    return $box;
+  }
+
+  public function handlePanelRequest(
+    AphrontRequest $request,
+    PhabricatorController $controller) {
+    return new Aphront404Response();
+  }
+
+}

src/applications/files/config/PhabricatorFilesConfigOptions.php

@@ -147,22 +147,6 @@ final class PhabricatorFilesConfigOptions
             "Set this to a valid Amazon S3 bucket to store files there. You ".
             "must also configure S3 access keys in the 'Amazon Web Services' ".
             "group.")),
-     $this->newOption(
-       'storage.engine-selector',
-       'class',
-       'PhabricatorDefaultFileStorageEngineSelector')
-        ->setBaseClass('PhabricatorFileStorageEngineSelector')
-        ->setSummary(pht('Storage engine selector.'))
-        ->setDescription(
-          pht(
-            'Phabricator uses a storage engine selector to choose which '.
-            'storage engine to use when writing file data. If you add new '.
-            'storage engines or want to provide very custom rules (e.g., '.
-            'write images to one storage engine and other files to a '.
-            'different one), you can provide an alternate implementation '.
-            'here. The default engine will use choose MySQL, Local Disk, and '.
-            'S3, in that order, if they have valid configurations above and '.
-            'a file fits within configured limits.')),
      $this->newOption('storage.upload-size-limit', 'string', null)
         ->setSummary(
           pht('Limit to users in interfaces which allow uploading.'))

src/applications/files/engine/PhabricatorFileStorageEngine.php

@@ -41,6 +41,78 @@ abstract class PhabricatorFileStorageEngine {
   abstract public function getEngineIdentifier();
 
 
+  /**
+   * Prioritize this engine relative to other engines.
+   *
+   * Engines with a smaller priority number get an opportunity to write files
+   * first. Generally, lower-latency filestores should have lower priority
+   * numbers, and higher-latency filestores should have higher priority
+   * numbers. Setting priority to approximately the number of milliseconds of
+   * read latency will generally produce reasonable results.
+   *
+   * In conjunction with filesize limits, the goal is to store small files like
+   * profile images, thumbnails, and text snippets in lower-latency engines,
+   * and store large files in higher-capacity engines.
+   *
+   * @return float Engine priority.
+   * @task meta
+   */
+  abstract public function getEnginePriority();
+
+
+  /**
+   * Return `true` if the engine is currently writable.
+   *
+   * Engines that are disabled or missing configuration should return `false`
+   * to prevent new writes. If writes were made with this engine in the past,
+   * the application may still try to perform reads.
+   *
+   * @return bool True if this engine can support new writes.
+   * @task meta
+   */
+  abstract public function canWriteFiles();
+
+
+  /**
+   * Return `true` if the engine has a filesize limit on storable files.
+   *
+   * The @{method:getFilesizeLimit} method can retrieve the actual limit. This
+   * method just removes the ambiguity around the meaning of a `0` limit.
+   *
+   * @return bool `true` if the engine has a filesize limit.
+   * @task meta
+   */
+  abstract public function hasFilesizeLimit();
+
+
+  /**
+   * Return maximum storable file size, in bytes.
+   *
+   * Not all engines have a limit; use @{method:getFilesizeLimit} to check if
+   * an engine has a limit. Engines without a limit can store files of any
+   * size.
+   *
+   * @return int Maximum storable file size, in bytes.
+   * @task meta
+   */
+  public function getFilesizeLimit() {
+    throw new PhutilMethodNotImplementedException();
+  }
+
+
+  /**
+   * Identifies storage engines that support unit tests.
+   *
+   * These engines are not used for production writes.
+   *
+   * @return bool True if this is a test engine.
+   * @task meta
+   */
+  public function isTestEngine() {
+    return false;
+  }
+
+
 /* -(  Managing File Data  )------------------------------------------------- */
 
 
@@ -90,4 +162,82 @@ abstract class PhabricatorFileStorageEngine {
    */
   abstract public function deleteFile($handle);
 
+
+  /**
+   * 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.
+   */
+  public static function loadStorageEngines($length) {
+    $engines = self::loadWritableEngines();
+
+    $writable = array();
+    foreach ($engines as $key => $engine) {
+      if ($engine->hasFilesizeLimit()) {
+        $limit = $engine->getFilesizeLimit();
+        if ($limit < $length) {
+          continue;
+        }
+      }
+
+      $writable[$key] = $engine;
+    }
+
+    return $writable;
+  }
+
+  public static function loadAllEngines() {
+    static $engines;
+
+    if ($engines === null) {
+      $objects = id(new PhutilSymbolLoader())
+        ->setAncestorClass(__CLASS__)
+        ->loadObjects();
+
+      $map = array();
+      foreach ($objects as $engine) {
+        $key = $engine->getEngineIdentifier();
+        if (empty($map[$key])) {
+          $map[$key] = $engine;
+        } else {
+          throw new Exception(
+            pht(
+              'Storage engines "%s" and "%s" have the same engine '.
+              'identifier "%s". Each storage engine must have a unique '.
+              'identifier.',
+              get_class($engine),
+              get_class($map[$key]),
+              $key));
+        }
+      }
+
+      $map = msort($map, 'getEnginePriority');
+
+      $engines = $map;
+    }
+
+    return $engines;
+  }
+
+  public static function loadWritableEngines() {
+    $engines = self::loadAllEngines();
+
+    $writable = array();
+    foreach ($engines as $key => $engine) {
+      if ($engine->isTestEngine()) {
+        continue;
+      }
+
+      if (!$engine->canWriteFiles()) {
+        continue;
+      }
+
+      $writable[$key] = $engine;
+    }
+
+    return $writable;
+  }
+
 }

src/applications/files/engine/PhabricatorLocalDiskFileStorageEngine.php

@@ -4,24 +4,39 @@
  * Local disk storage engine. Keeps files on local disk. This engine is easy
  * to set up, but it doesn't work if you have multiple web frontends!
  *
- * @task impl     Implementation
  * @task internal Internals
  */
 final class PhabricatorLocalDiskFileStorageEngine
   extends PhabricatorFileStorageEngine {
 
 
-/* -(  Implementation  )----------------------------------------------------- */
+/* -(  Engine Metadata  )---------------------------------------------------- */
 
 
   /**
    * This engine identifies as "local-disk".
-   * @task impl
    */
   public function getEngineIdentifier() {
     return 'local-disk';
   }
 
+  public function getEnginePriority() {
+    return 5;
+  }
+
+  public function canWriteFiles() {
+    $path = PhabricatorEnv::getEnvConfig('storage.local-disk.path');
+    return (bool)strlen($path);
+  }
+
+
+  public function hasFilesizeLimit() {
+    return false;
+  }
+
+
+/* -(  Managing File Data  )------------------------------------------------- */
+
 
   /**
    * Write the file data to local disk. Returns the relative path as the

src/applications/files/engine/PhabricatorMySQLFileStorageEngine.php

@@ -7,30 +7,47 @@
  * It uses the @{class:PhabricatorFileStorageBlob} to actually access the
  * underlying database table.
  *
- * @task impl     Implementation
  * @task internal Internals
  */
 final class PhabricatorMySQLFileStorageEngine
   extends PhabricatorFileStorageEngine {
 
-/* -(  Implementation  )----------------------------------------------------- */
+
+/* -(  Engine Metadata  )---------------------------------------------------- */
 
 
   /**
    * For historical reasons, this engine identifies as "blob".
-   *
-   * @task impl
    */
   public function getEngineIdentifier() {
     return 'blob';
   }
 
+  public function getEnginePriority() {
+    return 1;
+  }
+
+  public function canWriteFiles() {
+    return ($this->getFilesizeLimit() > 0);
+  }
+
+
+  public function hasFilesizeLimit() {
+    return true;
+  }
+
+
+  public function getFilesizeLimit() {
+    return PhabricatorEnv::getEnvConfig('storage.mysql-engine.max-size');
+  }
+
+
+/* -(  Managing File Data  )------------------------------------------------- */
+
 
   /**
    * Write file data into the big blob store table in MySQL. Returns the row
    * ID as the file data handle.
-   *
-   * @task impl
    */
   public function writeFile($data, array $params) {
     $blob = new PhabricatorFileStorageBlob();
@@ -43,7 +60,6 @@ final class PhabricatorMySQLFileStorageEngine
 
   /**
    * Load a stored blob from MySQL.
-   * @task impl
    */
   public function readFile($handle) {
     return $this->loadFromMySQLFileStorage($handle)->getData();
@@ -52,7 +68,6 @@ final class PhabricatorMySQLFileStorageEngine
 
   /**
    * Delete a blob from MySQL.
-   * @task impl
    */
   public function deleteFile($handle) {
     $this->loadFromMySQLFileStorage($handle)->delete();

src/applications/files/engine/PhabricatorS3FileStorageEngine.php

@@ -10,7 +10,7 @@ final class PhabricatorS3FileStorageEngine
   extends PhabricatorFileStorageEngine {
 
 
-/* -(  Implementation  )----------------------------------------------------- */
+/* -(  Engine Metadata  )---------------------------------------------------- */
 
 
   /**
@@ -20,6 +20,26 @@ final class PhabricatorS3FileStorageEngine
     return 'amazon-s3';
   }
 
+  public function getEnginePriority() {
+    return 100;
+  }
+
+  public function canWriteFiles() {
+    $bucket = PhabricatorEnv::getEnvConfig('storage.s3.bucket');
+    $access_key = PhabricatorEnv::getEnvConfig('amazon-s3.access-key');
+    $secret_key = PhabricatorEnv::getEnvConfig('amazon-s3.secret-key');
+
+    return (strlen($bucket) && strlen($access_key) && strlen($secret_key));
+  }
+
+
+  public function hasFilesizeLimit() {
+    return false;
+  }
+
+
+/* -(  Managing File Data  )------------------------------------------------- */
+
 
   /**
    * Writes file data into Amazon S3.

src/applications/files/engine/PhabricatorTestStorageEngine.php

@@ -13,6 +13,22 @@ final class PhabricatorTestStorageEngine
     return 'unit-test';
   }
 
+  public function getEnginePriority() {
+    return 1000;
+  }
+
+  public function isTestEngine() {
+    return true;
+  }
+
+  public function canWriteFiles() {
+    return true;
+  }
+
+  public function hasFilesizeLimit() {
+    return false;
+  }
+
   public function writeFile($data, array $params) {
     AphrontWriteGuard::willWrite();
     self::$storage[self::$nextHandle] = $data;

src/applications/files/engineselector/PhabricatorDefaultFileStorageEngineSelector.php

@@ -1,52 +0,0 @@
-<?php
-
-/**
- * Default storage engine selector. See
- * @{class:PhabricatorFileStorageEngineSelector} and @{article:File Storage
- * Technical Documentation} for more information.
- */
-final class PhabricatorDefaultFileStorageEngineSelector
-  extends PhabricatorFileStorageEngineSelector {
-
-  /**
-   * 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.
-   */
-  public function selectStorageEngines($data, array $params) {
-    $length = strlen($data);
-
-    $mysql_key = 'storage.mysql-engine.max-size';
-    $mysql_limit = PhabricatorEnv::getEnvConfig($mysql_key);
-
-    $engines = array();
-    if ($mysql_limit && $length <= $mysql_limit) {
-      $engines[] = new PhabricatorMySQLFileStorageEngine();
-    }
-
-    $local_key = 'storage.local-disk.path';
-    $local_path = PhabricatorEnv::getEnvConfig($local_key);
-    if ($local_path) {
-      $engines[] = new PhabricatorLocalDiskFileStorageEngine();
-    }
-
-    $s3_key = 'storage.s3.bucket';
-    if (PhabricatorEnv::getEnvConfig($s3_key)) {
-      $engines[] = new PhabricatorS3FileStorageEngine();
-    }
-
-    if ($mysql_limit && empty($engines)) {
-      // If we return no engines, an exception will be thrown but it will be
-      // a little vague ("No valid storage engines"). Since this is a default
-      // case, throw a more specific exception.
-      throw new Exception(
-        'This file exceeds the configured MySQL storage engine filesize '.
-        'limit, but no other storage engines are configured. Increase the '.
-        'MySQL storage engine limit or configure a storage engine suitable '.
-        'for larger files.');
-    }
-
-    return $engines;
-  }
-
-}

src/applications/files/engineselector/PhabricatorFileStorageEngineSelector.php

@@ -1,46 +0,0 @@
-<?php
-
-/**
- * Chooses appropriate storage engine(s) for files. When Phabricator needs
- * to write a blob of file data, it uses the configured selector to get a list
- * of suitable @{class:PhabricatorFileStorageEngine}s. For more information,
- * see @{article:File Storage Technical Documentation}.
- *
- * @task  select  Selecting Storage Engines
- */
-abstract class PhabricatorFileStorageEngineSelector {
-
-  final public function __construct() {
-    // <empty>
-  }
-
-
-/* -(  Selecting Storage Engines  )------------------------------------------ */
-
-
-  /**
-   * Select valid storage engines for a file. This method will be called by
-   * Phabricator when it needs to store a file permanently. It must return a
-   * list of valid @{class:PhabricatorFileStorageEngine}s.
-   *
-   * If you are extending this class to provide a custom selector, you
-   * probably just want it to look like this:
-   *
-   *   return array(new MyCustomFileStorageEngine());
-   *
-   * ...that is, store every file in whatever storage engine you're using.
-   * However, you can also provide multiple storage engines, or store some files
-   * in one engine and some files in a different engine by implementing a more
-   * complex selector.
-   *
-   * @param string  File data.
-   * @param dict    Dictionary of optional file metadata. This may be empty, or
-   *                have some additional keys like 'file' and 'author' which
-   *                provide metadata.
-   * @return list   List of @{class:PhabricatorFileStorageEngine}s, ordered by
-   *                preference.
-   * @task select
-   */
-  abstract public function selectStorageEngines($data, array $params);
-
-}

src/applications/files/storage/PhabricatorFile.php

@@ -269,14 +269,21 @@ final class PhabricatorFile extends PhabricatorFileDAO
     if (isset($params['storageEngines'])) {
       $engines = $params['storageEngines'];
     } else {
-      $selector = PhabricatorEnv::newObjectFromConfig(
+      $size = strlen($data);
-        'storage.engine-selector');
+      $engines = PhabricatorFileStorageEngine::loadStorageEngines($size);
-      $engines = $selector->selectStorageEngines($data, $params);
+
+      if (!$engines) {
+        throw new Exception(
+          pht(
+            'No configured storage engine can store this file. See '.
+            '"Configuring File Storage" in the documentation for '.
+            'information on configuring storage engines.'));
+      }
     }
 
     assert_instances_of($engines, 'PhabricatorFileStorageEngine');
     if (!$engines) {
-      throw new Exception('No valid storage engines are available!');
+      throw new Exception(pht('No valid storage engines are available!'));
     }
 
     $file = PhabricatorFile::initializeNewFile();

src/docs/tech/files.diviner

@@ -1,39 +0,0 @@
-@title File Storage Technical Documentation
-@group filestorage
-
-Phabricator file storage details.
-
-= Overview =
-
-Phabricator has a simple, general-purpose file storage system with configurable
-storage backends that allows you to choose where files are stored. For a user
-guide, see @{article:Configuring File Storage}.
-
-= Class Relationships =
-
-@{class:PhabricatorFile} holds file metadata (name, author, phid), including an
-identifier for a @{class:PhabricatorFileStorageEngine} where the actual file
-data is stored, and a data handle which identifies the data within that storage
-engine.
-
-When writing data, a @{class:PhabricatorFileStorageEngineSelector} is
-instantiated (by default, @{class:PhabricatorDefaultFileStorageEngineSelector},
-but you can change this by setting the ##storage.engine-selector## key in your
-configuration). The selector returns a list of satisfactory
-@{class:PhabricatorFileStorageEngine}s, in order of preference.
-
-For instance, suppose the user is uploading a picture. The upload pipeline would
-instantiate the configured selector, which might return a
-@{class:PhabricatorMySQLFileStorageEngine} and a
-@{class:PhabricatorLocalDiskFileStorageEngine}, indicating that the picture may
-be stored in either storage engine but MySQL is preferred. If a given storage
-engine fails to perform the write, it will fall back to the next engine.
-
-= Adding New Storage Engines =
-
-To add a new storage engine, extend @{class:PhabricatorFileStorageEngine}. In
-order to make files actually get written to it, you also need to extend
-@{class:PhabricatorFileStorageEngineSelector}, provide an implementation which
-selects your storage engine for whatever files you want to store there, and then
-configure Phabricator to use your selector by setting
-`storage.engine-selector`.

src/docs/user/configuration/configuring_file_storage.diviner

@@ -3,32 +3,35 @@
 
 Setup how Phabricator will store files.
 
-= Overview =
+Overview
+========
 
 Phabricator allows users to upload files, and several applications use file
 storage (for instance, Maniphest allows you to attach files to tasks). You can
-configure several different storage systems:
+configure several different storage systems.
 
-  - you can store data in MySQL: this is the easiest to set up, but doesn't
+| System | Setup | Cost | Notes |
-    scale well;
+|========|=======|======|=======|
-  - you can store data on local disk: this is also easy to set up but won't
+| MySQL  | Automatic | Free | May not scale well. |
-    scale to multiple web frontends without NFS;
+| Local Disk | Easy | Free | Does not scale well. |
-  - or you can build a custom storage engine.
+| Amazon S3 | Easy | Cheap | Scales well. |
+| Custom | Hard | Varies | Implement a custom storage engine. |
 
 By default, Phabricator is configured to store files up to 1MB in MySQL, and
-reject files larger than 1MB. It is recommended you set up local disk storage
+reject files larger than 1MB. To store larger files, you can either:
-for files larger than 1MB. This should be sufficient for most installs. If you
-have a larger install or more unique requirements, you may want to customize
-this further.
 
-For technical documentation (including instructions on building custom storage
+  - configure local disk storage; or
-engines) see @{article:File Storage Technical Documentation}.
+  - configure Amazon S3 storage; or
+  - raise the limits on MySQL.
+
+See the rest of this document for some additional discussion of engines.
 
 You don't have to fully configure this immediately, the defaults are okay until
 you need to upload larger files and it's relatively easy to port files between
 storage engines later.
 
-= Storage Engines =
+Storage Engines
+===============
 
 Builtin storage engines and information on how to configure them.
 
@@ -41,23 +44,24 @@ Builtin storage engines and information on how to configure them.
 MySQL storage is configured by default, for files up to (just under) 1MB. You
 can configure it with these keys:
 
-  - ##storage.mysql-engine.max-size##: Change the filesize limit. Set to 0
+  - `storage.mysql-engine.max-size`: Change the filesize limit. Set to 0
     to disable.
 
-For most installs, it is recommended you configure local disk storage below,
+For most installs, it is reasonable to leave this engine as-is and let small
-and then either leave this as is or disable it, depending on how upset you feel
+files (like thumbnails and profile images) be stored in MySQL, which is usually
-about putting files in a database.
+the lowest-latency filestore.
+
+To support larger files, configure another engine or increase this limit.
 
 == Local Disk ==
 
   - **Pros**: Very simple. Almost no setup required.
   - **Cons**: Doesn't scale to multiple web frontends without NFS.
 
-For most installs, it is **strongly recommended** that you configure local disk
+To upload larger files:
-storage. To do this, set the configuration key:
 
-  - ##storage.local-disk.path##: Set to some writable directory on local disk.
+  - `storage.local-disk.path`: Set to some writable directory on local disk.
-    Make that directory. You're done.
+    Make that directory.
 
 == Amazon S3 ==
 
@@ -70,11 +74,6 @@ To enable file storage in S3, set these key:
   - ##amazon-s3.secret-key## Your AWS secret key.
   - ##storage.s3.bucket## S3 bucket name where files should be stored.
 
-== Custom Engine ==
-
-For details about writing a custom storage engine, see @{article:File Storage
-Technical Documentation}.
-
 = Testing Storage Engines =
 
 You can test that things are correctly configured by going to the Files