Split setup check phases into "preflight" and "post-config"

Summary:
Ref T11589. This runs:

  - preflight checks (critical checks: PHP version stuff, extensions);
  - configuration;
  - normal checks.

The PHP checks are split into critical ("bad version") and noncritical ("sub-optimal config").

I tidied up the extension checks slightly, we realistically depend on `cURL` nowadays.

Test Plan:
  - Faked a preflight failure.
  - Hit preflight check.
  - Got expected error screen.
  - Loaded normal pages.
  - Hit a normal setup check.
  - Used DarkConsole "Startup" tab to verify that preflight checks take <1ms to run (we run them on every page without caching, at least for now, but they only do trivial checks like PHP versions).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11589

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

src/__phutil_library_map__.php

@@ -2998,6 +2998,7 @@ phutil_register_library_map(array(
     'PhabricatorPHPASTApplication' => 'applications/phpast/application/PhabricatorPHPASTApplication.php',
     'PhabricatorPHPConfigSetupCheck' => 'applications/config/check/PhabricatorPHPConfigSetupCheck.php',
     'PhabricatorPHPMailerConfigOptions' => 'applications/config/option/PhabricatorPHPMailerConfigOptions.php',
+    'PhabricatorPHPPreflightSetupCheck' => 'applications/config/check/PhabricatorPHPPreflightSetupCheck.php',
     'PhabricatorPackagesApplication' => 'applications/packages/application/PhabricatorPackagesApplication.php',
     'PhabricatorPackagesController' => 'applications/packages/controller/PhabricatorPackagesController.php',
     'PhabricatorPackagesCreatePublisherCapability' => 'applications/packages/capability/PhabricatorPackagesCreatePublisherCapability.php',
@@ -7855,6 +7856,7 @@ phutil_register_library_map(array(
     'PhabricatorPHPASTApplication' => 'PhabricatorApplication',
     'PhabricatorPHPConfigSetupCheck' => 'PhabricatorSetupCheck',
     'PhabricatorPHPMailerConfigOptions' => 'PhabricatorApplicationConfigOptions',
+    'PhabricatorPHPPreflightSetupCheck' => 'PhabricatorSetupCheck',
     'PhabricatorPackagesApplication' => 'PhabricatorApplication',
     'PhabricatorPackagesController' => 'PhabricatorController',
     'PhabricatorPackagesCreatePublisherCapability' => 'PhabricatorPolicyCapability',

src/aphront/configuration/AphrontApplicationConfiguration.php

@@ -69,6 +69,15 @@ abstract class AphrontApplicationConfiguration extends Phobject {
     // request object first.
     $write_guard = new AphrontWriteGuard('id');
 
+    PhabricatorStartup::beginStartupPhase('preflight');
+
+    $response = PhabricatorSetupCheck::willPreflightRequest();
+    if ($response) {
+      PhabricatorStartup::endOutputCapture();
+      $sink->writeResponse($response);
+      return;
+    }
+
     PhabricatorStartup::beginStartupPhase('env.init');
     PhabricatorEnv::initializeWebEnvironment();
 

src/applications/config/check/PhabricatorExtensionsSetupCheck.php

@@ -12,7 +12,6 @@ final class PhabricatorExtensionsSetupCheck extends PhabricatorSetupCheck {
 
   protected function executeChecks() {
     // TODO: Make 'mbstring' and 'iconv' soft requirements.
-    // TODO: Make 'curl' a soft requirement.
 
     $required = array(
       'hash',
@@ -22,9 +21,8 @@ final class PhabricatorExtensionsSetupCheck extends PhabricatorSetupCheck {
       'iconv',
       'ctype',
 
-      // There is a chance we might not need this, but some configurations (like
+      // There is a tiny chance we might not need this, but a significant
-      // OAuth or Amazon SES) will require it. Just mark it 'required' since
+      // number of applications require it and it's widely available.
-      // it's widely available and relatively core.
       'curl',
     );
 

src/applications/config/check/PhabricatorPHPConfigSetupCheck.php

@@ -1,202 +1,17 @@
 <?php
 
+/**
+ * Noncritical PHP configuration checks.
+ *
+ * For critical checks, see @{class:PhabricatorPHPPreflightSetupCheck}.
+ */
 final class PhabricatorPHPConfigSetupCheck extends PhabricatorSetupCheck {
 
   public function getDefaultGroup() {
     return self::GROUP_PHP;
   }
 
-  public function isPreflightCheck() {
-    return true;
-  }
-
   protected function executeChecks() {
-    if (version_compare(phpversion(), 7, '>=')) {
-      $message = pht(
-        'This version of Phabricator does not support PHP 7. You '.
-        'are running PHP %s.',
-        phpversion());
-
-      $this->newIssue('php.version7')
-        ->setIsFatal(true)
-        ->setName(pht('PHP 7 Not Supported'))
-        ->setMessage($message)
-        ->addLink(
-          'https://phurl.io/u/php7',
-          pht('Phabricator PHP 7 Compatibility Information'));
-
-      return;
-    }
-
-    $safe_mode = ini_get('safe_mode');
-    if ($safe_mode) {
-      $message = pht(
-        "You have '%s' enabled in your PHP configuration, but Phabricator ".
-        "will not run in safe mode. Safe mode has been deprecated in PHP 5.3 ".
-        "and removed in PHP 5.4.\n\nDisable safe mode to continue.",
-        'safe_mode');
-
-      $this->newIssue('php.safe_mode')
-        ->setIsFatal(true)
-        ->setName(pht('Disable PHP %s', 'safe_mode'))
-        ->setMessage($message)
-        ->addPHPConfig('safe_mode');
-      return;
-    }
-
-    // Check for `disable_functions` or `disable_classes`. Although it's
-    // possible to disable a bunch of functions (say, `array_change_key_case()`)
-    // and classes and still have Phabricator work fine, it's unreasonably
-    // difficult for us to be sure we'll even survive setup if these options
-    // are enabled. Phabricator needs access to the most dangerous functions,
-    // so there is no reasonable configuration value here which actually
-    // provides a benefit while guaranteeing Phabricator will run properly.
-
-    $disable_options = array('disable_functions', 'disable_classes');
-    foreach ($disable_options as $disable_option) {
-      $disable_value = ini_get($disable_option);
-      if ($disable_value) {
-
-        // By default Debian installs the pcntl extension but disables all of
-        // its functions using configuration. Whitelist disabling these
-        // functions so that Debian PHP works out of the box (we do not need to
-        // call these functions from the web UI). This is pretty ridiculous but
-        // it's not the users' fault and they haven't done anything crazy to
-        // get here, so don't make them pay for Debian's unusual choices.
-        // See: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=605571
-        $fatal = true;
-        if ($disable_option == 'disable_functions') {
-          $functions = preg_split('/[, ]+/', $disable_value);
-          $functions = array_filter($functions);
-          foreach ($functions as $k => $function) {
-            if (preg_match('/^pcntl_/', $function)) {
-              unset($functions[$k]);
-            }
-          }
-          if (!$functions) {
-            $fatal = false;
-          }
-        }
-
-        if ($fatal) {
-          $message = pht(
-            "You have '%s' enabled in your PHP configuration.\n\n".
-            "This option is not compatible with Phabricator. Remove ".
-            "'%s' from your configuration to continue.",
-            $disable_option,
-            $disable_option);
-
-          $this->newIssue('php.'.$disable_option)
-            ->setIsFatal(true)
-            ->setName(pht('Remove PHP %s', $disable_option))
-            ->setMessage($message)
-            ->addPHPConfig($disable_option);
-        }
-      }
-    }
-
-    $overload_option = 'mbstring.func_overload';
-    $func_overload = ini_get($overload_option);
-    if ($func_overload) {
-      $message = pht(
-        "You have '%s' enabled in your PHP configuration.\n\n".
-        "This option is not compatible with Phabricator. Disable ".
-        "'%s' in your PHP configuration to continue.",
-        $overload_option,
-        $overload_option);
-
-      $this->newIssue('php'.$overload_option)
-        ->setIsFatal(true)
-        ->setName(pht('Disable PHP %s', $overload_option))
-        ->setMessage($message)
-        ->addPHPConfig($overload_option);
-    }
-
-    $open_basedir = ini_get('open_basedir');
-    if ($open_basedir) {
-
-      // 'open_basedir' restricts which files we're allowed to access with
-      // file operations. This might be okay -- we don't need to write to
-      // arbitrary places in the filesystem -- but we need to access certain
-      // resources. This setting is unlikely to be providing any real measure
-      // of security so warn even if things look OK.
-
-      $failures = array();
-
-      try {
-        $open_libphutil = class_exists('Future');
-      } catch (Exception $ex) {
-        $failures[] = $ex->getMessage();
-      }
-
-      try {
-        $open_arcanist = class_exists('ArcanistDiffParser');
-      } catch (Exception $ex) {
-        $failures[] = $ex->getMessage();
-      }
-
-      $open_urandom = false;
-      try {
-        Filesystem::readRandomBytes(1);
-        $open_urandom = true;
-      } catch (FilesystemException $ex) {
-        $failures[] = $ex->getMessage();
-      }
-
-      try {
-        $tmp = new TempFile();
-        file_put_contents($tmp, '.');
-        $open_tmp = @fopen((string)$tmp, 'r');
-        if (!$open_tmp) {
-          $failures[] = pht(
-            "Unable to read temporary file '%s'.",
-            (string)$tmp);
-        }
-      } catch (Exception $ex) {
-        $message = $ex->getMessage();
-        $dir = sys_get_temp_dir();
-        $failures[] = pht(
-          "Unable to open temp files from '%s': %s",
-          $dir,
-          $message);
-      }
-
-      $issue = $this->newIssue('php.open_basedir')
-        ->setName(pht('Disable PHP %s', 'open_basedir'))
-        ->addPHPConfig('open_basedir');
-
-      if ($failures) {
-        $message = pht(
-          "Your server is configured with '%s', which prevents Phabricator ".
-          "from opening files it requires access to.\n\n".
-          "Disable this setting to continue.\n\nFailures:\n\n%s",
-          'open_basedir',
-          implode("\n\n", $failures));
-
-        $issue
-          ->setIsFatal(true)
-          ->setMessage($message);
-
-        return;
-      } else {
-        $summary = pht(
-          "You have '%s' configured in your PHP settings, which ".
-          "may cause some features to fail.",
-          'open_basedir');
-
-        $message = pht(
-          "You have '%s' configured in your PHP settings. Although this ".
-          "setting appears permissive enough that Phabricator will work ".
-          "properly, you may still run into problems because of it.\n\n".
-          "Consider disabling '%s'.",
-          'open_basedir',
-          'open_basedir');
-
-        $issue
-          ->setSummary($summary)
-          ->setMessage($message);
-      }
-    }
 
     if (empty($_SERVER['REMOTE_ADDR'])) {
       $doc_href = PhabricatorEnv::getDocLink('Configuring a Preamble Script');
@@ -245,5 +60,7 @@ final class PhabricatorPHPConfigSetupCheck extends PhabricatorSetupCheck {
         ->setMessage($message)
         ->addPHPConfig('always_populate_raw_post_data');
     }
+
   }
+
 }

src/applications/config/check/PhabricatorPHPPreflightSetupCheck.php

@@ -0,0 +1,201 @@
+<?php
+
+final class PhabricatorPHPPreflightSetupCheck extends PhabricatorSetupCheck {
+
+  public function getDefaultGroup() {
+    return self::GROUP_PHP;
+  }
+
+  public function isPreflightCheck() {
+    return true;
+  }
+
+  protected function executeChecks() {
+    if (version_compare(phpversion(), 7, '>=')) {
+      $message = pht(
+        'This version of Phabricator does not support PHP 7. You '.
+        'are running PHP %s.',
+        phpversion());
+
+      $this->newIssue('php.version7')
+        ->setIsFatal(true)
+        ->setName(pht('PHP 7 Not Supported'))
+        ->setMessage($message)
+        ->addLink(
+          'https://phurl.io/u/php7',
+          pht('Phabricator PHP 7 Compatibility Information'));
+
+      return;
+    }
+
+    $safe_mode = ini_get('safe_mode');
+    if ($safe_mode) {
+      $message = pht(
+        "You have '%s' enabled in your PHP configuration, but Phabricator ".
+        "will not run in safe mode. Safe mode has been deprecated in PHP 5.3 ".
+        "and removed in PHP 5.4.\n\nDisable safe mode to continue.",
+        'safe_mode');
+
+      $this->newIssue('php.safe_mode')
+        ->setIsFatal(true)
+        ->setName(pht('Disable PHP %s', 'safe_mode'))
+        ->setMessage($message)
+        ->addPHPConfig('safe_mode');
+      return;
+    }
+
+    // Check for `disable_functions` or `disable_classes`. Although it's
+    // possible to disable a bunch of functions (say, `array_change_key_case()`)
+    // and classes and still have Phabricator work fine, it's unreasonably
+    // difficult for us to be sure we'll even survive setup if these options
+    // are enabled. Phabricator needs access to the most dangerous functions,
+    // so there is no reasonable configuration value here which actually
+    // provides a benefit while guaranteeing Phabricator will run properly.
+
+    $disable_options = array('disable_functions', 'disable_classes');
+    foreach ($disable_options as $disable_option) {
+      $disable_value = ini_get($disable_option);
+      if ($disable_value) {
+
+        // By default Debian installs the pcntl extension but disables all of
+        // its functions using configuration. Whitelist disabling these
+        // functions so that Debian PHP works out of the box (we do not need to
+        // call these functions from the web UI). This is pretty ridiculous but
+        // it's not the users' fault and they haven't done anything crazy to
+        // get here, so don't make them pay for Debian's unusual choices.
+        // See: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=605571
+        $fatal = true;
+        if ($disable_option == 'disable_functions') {
+          $functions = preg_split('/[, ]+/', $disable_value);
+          $functions = array_filter($functions);
+          foreach ($functions as $k => $function) {
+            if (preg_match('/^pcntl_/', $function)) {
+              unset($functions[$k]);
+            }
+          }
+          if (!$functions) {
+            $fatal = false;
+          }
+        }
+
+        if ($fatal) {
+          $message = pht(
+            "You have '%s' enabled in your PHP configuration.\n\n".
+            "This option is not compatible with Phabricator. Remove ".
+            "'%s' from your configuration to continue.",
+            $disable_option,
+            $disable_option);
+
+          $this->newIssue('php.'.$disable_option)
+            ->setIsFatal(true)
+            ->setName(pht('Remove PHP %s', $disable_option))
+            ->setMessage($message)
+            ->addPHPConfig($disable_option);
+        }
+      }
+    }
+
+    $overload_option = 'mbstring.func_overload';
+    $func_overload = ini_get($overload_option);
+    if ($func_overload) {
+      $message = pht(
+        "You have '%s' enabled in your PHP configuration.\n\n".
+        "This option is not compatible with Phabricator. Disable ".
+        "'%s' in your PHP configuration to continue.",
+        $overload_option,
+        $overload_option);
+
+      $this->newIssue('php'.$overload_option)
+        ->setIsFatal(true)
+        ->setName(pht('Disable PHP %s', $overload_option))
+        ->setMessage($message)
+        ->addPHPConfig($overload_option);
+    }
+
+    $open_basedir = ini_get('open_basedir');
+    if ($open_basedir) {
+
+      // 'open_basedir' restricts which files we're allowed to access with
+      // file operations. This might be okay -- we don't need to write to
+      // arbitrary places in the filesystem -- but we need to access certain
+      // resources. This setting is unlikely to be providing any real measure
+      // of security so warn even if things look OK.
+
+      $failures = array();
+
+      try {
+        $open_libphutil = class_exists('Future');
+      } catch (Exception $ex) {
+        $failures[] = $ex->getMessage();
+      }
+
+      try {
+        $open_arcanist = class_exists('ArcanistDiffParser');
+      } catch (Exception $ex) {
+        $failures[] = $ex->getMessage();
+      }
+
+      $open_urandom = false;
+      try {
+        Filesystem::readRandomBytes(1);
+        $open_urandom = true;
+      } catch (FilesystemException $ex) {
+        $failures[] = $ex->getMessage();
+      }
+
+      try {
+        $tmp = new TempFile();
+        file_put_contents($tmp, '.');
+        $open_tmp = @fopen((string)$tmp, 'r');
+        if (!$open_tmp) {
+          $failures[] = pht(
+            "Unable to read temporary file '%s'.",
+            (string)$tmp);
+        }
+      } catch (Exception $ex) {
+        $message = $ex->getMessage();
+        $dir = sys_get_temp_dir();
+        $failures[] = pht(
+          "Unable to open temp files from '%s': %s",
+          $dir,
+          $message);
+      }
+
+      $issue = $this->newIssue('php.open_basedir')
+        ->setName(pht('Disable PHP %s', 'open_basedir'))
+        ->addPHPConfig('open_basedir');
+
+      if ($failures) {
+        $message = pht(
+          "Your server is configured with '%s', which prevents Phabricator ".
+          "from opening files it requires access to.\n\n".
+          "Disable this setting to continue.\n\nFailures:\n\n%s",
+          'open_basedir',
+          implode("\n\n", $failures));
+
+        $issue
+          ->setIsFatal(true)
+          ->setMessage($message);
+
+        return;
+      } else {
+        $summary = pht(
+          "You have '%s' configured in your PHP settings, which ".
+          "may cause some features to fail.",
+          'open_basedir');
+
+        $message = pht(
+          "You have '%s' configured in your PHP settings. Although this ".
+          "setting appears permissive enough that Phabricator will work ".
+          "properly, you may still run into problems because of it.\n\n".
+          "Consider disabling '%s'.",
+          'open_basedir',
+          'open_basedir');
+
+        $issue
+          ->setSummary($summary)
+          ->setMessage($message);
+      }
+    }
+  }
+}

src/applications/config/check/PhabricatorSetupCheck.php

@@ -129,16 +129,39 @@ abstract class PhabricatorSetupCheck extends Phobject {
       ));
   }
 
+  final public static function willPreflightRequest() {
+    $checks = self::loadAllChecks();
+
+    foreach ($checks as $check) {
+      if (!$check->isPreflightCheck()) {
+        continue;
+      }
+
+      $check->runSetupChecks();
+
+      foreach ($check->getIssues() as $key => $issue) {
+        return self::newIssueResponse($issue);
+      }
+    }
+
+    return null;
+  }
+
+  private static function newIssueResponse(PhabricatorSetupIssue $issue) {
+    $view = id(new PhabricatorSetupIssueView())
+      ->setIssue($issue);
+
+    return id(new PhabricatorConfigResponse())
+      ->setView($view);
+  }
+
   final public static function willProcessRequest() {
     $issue_keys = self::getOpenSetupIssueKeys();
     if ($issue_keys === null) {
-      $issues = self::runAllChecks();
+      $issues = self::runNormalChecks();
       foreach ($issues as $issue) {
         if ($issue->getIsFatal()) {
-          $view = id(new PhabricatorSetupIssueView())
+          return self::newIssueResponse($issue);
-            ->setIssue($issue);
-          return id(new PhabricatorConfigResponse())
-            ->setView($view);
         }
       }
       $issue_keys = self::getUnignoredIssueKeys($issues);
@@ -176,9 +199,15 @@ abstract class PhabricatorSetupCheck extends Phobject {
       ->execute();
   }
 
-  final public static function runAllChecks() {
+  final public static function runNormalChecks() {
     $checks = self::loadAllChecks();
 
+    foreach ($checks as $key => $check) {
+      if ($check->isPreflightCheck()) {
+        unset($checks[$key]);
+      }
+    }
+
     $issues = array();
     foreach ($checks as $check) {
       $check->runSetupChecks();

src/applications/config/controller/PhabricatorConfigIssueListController.php

@@ -9,7 +9,7 @@ final class PhabricatorConfigIssueListController
     $nav = $this->buildSideNavView();
     $nav->selectFilter('issue/');
 
-    $issues = PhabricatorSetupCheck::runAllChecks();
+    $issues = PhabricatorSetupCheck::runNormalChecks();
     PhabricatorSetupCheck::setOpenSetupIssueKeys(
       PhabricatorSetupCheck::getUnignoredIssueKeys($issues),
       $update_database = true);

src/applications/config/controller/PhabricatorConfigIssuePanelController.php

@@ -6,7 +6,7 @@ final class PhabricatorConfigIssuePanelController
   public function handleRequest(AphrontRequest $request) {
     $viewer = $request->getViewer();
     $open_items = PhabricatorSetupCheck::getOpenSetupIssueKeys();
-    $issues = PhabricatorSetupCheck::runAllChecks();
+    $issues = PhabricatorSetupCheck::runNormalChecks();
     PhabricatorSetupCheck::setOpenSetupIssueKeys(
       PhabricatorSetupCheck::getUnignoredIssueKeys($issues),
       $update_database = true);

src/applications/config/controller/PhabricatorConfigIssueViewController.php

@@ -7,7 +7,7 @@ final class PhabricatorConfigIssueViewController
     $viewer = $request->getViewer();
     $issue_key = $request->getURIData('key');
 
-    $issues = PhabricatorSetupCheck::runAllChecks();
+    $issues = PhabricatorSetupCheck::runNormalChecks();
     PhabricatorSetupCheck::setOpenSetupIssueKeys(
       PhabricatorSetupCheck::getUnignoredIssueKeys($issues),
       $update_database = true);

src/infrastructure/env/PhabricatorEnv.php

@@ -315,6 +315,14 @@ final class PhabricatorEnv extends Phobject {
       return self::$cache[$key];
     }
 
+    if (!self::$sourceStack) {
+      throw new Exception(
+        pht(
+          'Trying to read configuration "%s" before configuration has been '.
+          'initialized.',
+          $key));
+    }
+
     $result = self::$sourceStack->getKeys(array($key));
     if (array_key_exists($key, $result)) {
       self::$cache[$key] = $result[$key];
@@ -327,7 +335,6 @@ final class PhabricatorEnv extends Phobject {
     }
   }
 
-
   /**
    * Get the current configuration setting for a given key. If the key
    * does not exist, return a default value instead of throwing. This is