Read "$_POST" before hooking the profiler, and remove "aphront.default-application-configuration-class"

Summary:
Ref T4369. Ref T12297. Ref T13242. Ref PHI1010. I want to take a quick look at `transaction.search` and see if there's anything quick and obvious we can do to improve performance.

On `secure`, the `__profile__` flag does not survive POST like it's supposed to: when you profile a page and then submit a form on the page, the result is supposed to be profiled. The intent is to make it easier to profile Conduit calls.

I believe this is because we're hooking the profiler, then rebuilding POST data a little later -- so `$_POST['__profile__']` isn't set yet when the profiler checks.

Move the POST rebuild a little earlier to fix this.

Also, remove the very ancient "aphront.default-application-configuration-class". I believe this was only used by Facebook to do CIDR checks against corpnet or something like that. It is poorly named and long-obsolete now, and `AphrontSite` does everything we might reasonably have wanted it to do.

Test Plan: Poked around locally without any issues. Will check if this fixes the issue on `secure`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13242, T12297, T4369

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

src/__phutil_library_map__.php

@@ -183,7 +183,6 @@ phutil_register_library_map(array(
     'AphrontCalendarEventView' => 'applications/calendar/view/AphrontCalendarEventView.php',
     'AphrontController' => 'aphront/AphrontController.php',
     'AphrontCursorPagerView' => 'view/control/AphrontCursorPagerView.php',
-    'AphrontDefaultApplicationConfiguration' => 'aphront/configuration/AphrontDefaultApplicationConfiguration.php',
     'AphrontDialogResponse' => 'aphront/response/AphrontDialogResponse.php',
     'AphrontDialogView' => 'view/AphrontDialogView.php',
     'AphrontEpochHTTPParameterType' => 'aphront/httpparametertype/AphrontEpochHTTPParameterType.php',
@@ -5631,7 +5630,6 @@ phutil_register_library_map(array(
     'AphrontCalendarEventView' => 'AphrontView',
     'AphrontController' => 'Phobject',
     'AphrontCursorPagerView' => 'AphrontView',
-    'AphrontDefaultApplicationConfiguration' => 'AphrontApplicationConfiguration',
     'AphrontDialogResponse' => 'AphrontResponse',
     'AphrontDialogView' => array(
       'AphrontView',

src/aphront/configuration/AphrontApplicationConfiguration.php

@@ -5,55 +5,81 @@
  * @task response Response Handling
  * @task exception Exception Handling
  */
-abstract class AphrontApplicationConfiguration extends Phobject {
+final class AphrontApplicationConfiguration
+  extends Phobject {
 
   private $request;
   private $host;
   private $path;
   private $console;
 
-  abstract public function buildRequest();
-  abstract public function build404Controller();
-  abstract public function buildRedirectController($uri, $external);
+  public function buildRequest() {
+    $parser = new PhutilQueryStringParser();
 
-  final public function setRequest(AphrontRequest $request) {
+    $data = array();
+    $data += $_POST;
+    $data += $parser->parseQueryString(idx($_SERVER, 'QUERY_STRING', ''));
+
+    $cookie_prefix = PhabricatorEnv::getEnvConfig('phabricator.cookie-prefix');
+
+    $request = new AphrontRequest($this->getHost(), $this->getPath());
+    $request->setRequestData($data);
+    $request->setApplicationConfiguration($this);
+    $request->setCookiePrefix($cookie_prefix);
+
+    return $request;
+  }
+
+  public function build404Controller() {
+    return array(new Phabricator404Controller(), array());
+  }
+
+  public function buildRedirectController($uri, $external) {
+    return array(
+      new PhabricatorRedirectController(),
+      array(
+        'uri' => $uri,
+        'external' => $external,
+      ),
+    );
+  }
+
+  public function setRequest(AphrontRequest $request) {
     $this->request = $request;
     return $this;
   }
 
-  final public function getRequest() {
+  public function getRequest() {
     return $this->request;
   }
 
-  final public function getConsole() {
+  public function getConsole() {
     return $this->console;
   }
 
-  final public function setConsole($console) {
+  public function setConsole($console) {
     $this->console = $console;
     return $this;
   }
 
-  final public function setHost($host) {
+  public function setHost($host) {
     $this->host = $host;
     return $this;
   }
 
-  final public function getHost() {
+  public function getHost() {
     return $this->host;
   }
 
-  final public function setPath($path) {
+  public function setPath($path) {
     $this->path = $path;
     return $this;
   }
 
-  final public function getPath() {
+  public function getPath() {
     return $this->path;
   }
 
-  public function willBuildRequest() {}
-
 
   /**
    * @phutil-external-symbol class PhabricatorStartup
@@ -126,6 +152,8 @@ abstract class AphrontApplicationConfiguration extends Phobject {
         'M' => idx($_SERVER, 'REQUEST_METHOD', '-'),
       ));
 
+    self::readHTTPPOSTData();
+
     DarkConsoleXHProfPluginAPI::hookProfiler();
 
     // We just activated the profiler, so we don't need to keep track of
@@ -142,16 +170,10 @@ abstract class AphrontApplicationConfiguration extends Phobject {
     $host = AphrontRequest::getHTTPHeader('Host');
     $path = $_REQUEST['__path__'];
 
-    switch ($host) {
-      default:
-        $config_key = 'aphront.default-application-configuration-class';
-        $application = PhabricatorEnv::newObjectFromConfig($config_key);
-        break;
-    }
+    $application = new self();
 
     $application->setHost($host);
     $application->setPath($path);
-    $application->willBuildRequest();
     $request = $application->buildRequest();
 
     // Now that we have a request, convert the write guard into one which
@@ -313,7 +335,7 @@ abstract class AphrontApplicationConfiguration extends Phobject {
    *                                      parameters.
    * @task routing
    */
-  final private function buildController() {
+  private function buildController() {
     $request = $this->getRequest();
 
     // If we're configured to operate in cluster mode, reject requests which
@@ -708,4 +730,84 @@ abstract class AphrontApplicationConfiguration extends Phobject {
       ->setContent($result);
   }
 
+  private static function readHTTPPOSTData() {
+    $request_method = idx($_SERVER, 'REQUEST_METHOD');
+    if ($request_method === 'PUT') {
+      // For PUT requests, do nothing: in particular, do NOT read input. This
+      // allows us to stream input later and process very large PUT requests,
+      // like those coming from Git LFS.
+      return;
+    }
+
+
+    // For POST requests, we're going to read the raw input ourselves here
+    // if we can. Among other things, this corrects variable names with
+    // the "." character in them, which PHP normally converts into "_".
+
+    // There are two major considerations here: whether the
+    // `enable_post_data_reading` option is set, and whether the content
+    // type is "multipart/form-data" or not.
+
+    // If `enable_post_data_reading` is off, we're free to read the entire
+    // raw request body and parse it -- and we must, because $_POST and
+    // $_FILES are not built for us. If `enable_post_data_reading` is on,
+    // which is the default, we may not be able to read the body (the
+    // documentation says we can't, but empirically we can at least some
+    // of the time).
+
+    // If the content type is "multipart/form-data", we need to build both
+    // $_POST and $_FILES, which is involved. The body itself is also more
+    // difficult to parse than other requests.
+    $raw_input = PhabricatorStartup::getRawInput();
+    $parser = new PhutilQueryStringParser();
+
+    if (strlen($raw_input)) {
+      $content_type = idx($_SERVER, 'CONTENT_TYPE');
+      $is_multipart = preg_match('@^multipart/form-data@i', $content_type);
+      if ($is_multipart && !ini_get('enable_post_data_reading')) {
+        $multipart_parser = id(new AphrontMultipartParser())
+          ->setContentType($content_type);
+
+        $multipart_parser->beginParse();
+        $multipart_parser->continueParse($raw_input);
+        $parts = $multipart_parser->endParse();
+
+        $query_string = array();
+        foreach ($parts as $part) {
+          if (!$part->isVariable()) {
+            continue;
+          }
+
+          $name = $part->getName();
+          $value = $part->getVariableValue();
+
+          $query_string[] = urlencode($name).'='.urlencode($value);
+        }
+        $query_string = implode('&', $query_string);
+        $post = $parser->parseQueryString($query_string);
+
+        $files = array();
+        foreach ($parts as $part) {
+          if ($part->isVariable()) {
+            continue;
+          }
+
+          $files[$part->getName()] = $part->getPHPFileDictionary();
+        }
+        $_FILES = $files;
+      } else {
+        $post = $parser->parseQueryString($raw_input);
+      }
+
+      $_POST = $post;
+      PhabricatorStartup::rebuildRequest();
+    } else if ($_POST) {
+      $post = filter_input_array(INPUT_POST, FILTER_UNSAFE_RAW);
+      if (is_array($post)) {
+        $_POST = $post;
+        PhabricatorStartup::rebuildRequest();
+      }
+    }
+  }
+
 }

src/aphront/configuration/AphrontDefaultApplicationConfiguration.php

@@ -1,121 +0,0 @@
-<?php
-
-/**
- * NOTE: Do not extend this!
- *
- * @concrete-extensible
- */
-class AphrontDefaultApplicationConfiguration
-  extends AphrontApplicationConfiguration {
-
-  /**
-   * @phutil-external-symbol class PhabricatorStartup
-   */
-  public function buildRequest() {
-    $parser = new PhutilQueryStringParser();
-    $data   = array();
-
-    $request_method = idx($_SERVER, 'REQUEST_METHOD');
-    if ($request_method === 'PUT') {
-      // For PUT requests, do nothing: in particular, do NOT read input. This
-      // allows us to stream input later and process very large PUT requests,
-      // like those coming from Git LFS.
-    } else {
-      // For POST requests, we're going to read the raw input ourselves here
-      // if we can. Among other things, this corrects variable names with
-      // the "." character in them, which PHP normally converts into "_".
-
-      // There are two major considerations here: whether the
-      // `enable_post_data_reading` option is set, and whether the content
-      // type is "multipart/form-data" or not.
-
-      // If `enable_post_data_reading` is off, we're free to read the entire
-      // raw request body and parse it -- and we must, because $_POST and
-      // $_FILES are not built for us. If `enable_post_data_reading` is on,
-      // which is the default, we may not be able to read the body (the
-      // documentation says we can't, but empirically we can at least some
-      // of the time).
-
-      // If the content type is "multipart/form-data", we need to build both
-      // $_POST and $_FILES, which is involved. The body itself is also more
-      // difficult to parse than other requests.
-      $raw_input = PhabricatorStartup::getRawInput();
-      if (strlen($raw_input)) {
-        $content_type = idx($_SERVER, 'CONTENT_TYPE');
-        $is_multipart = preg_match('@^multipart/form-data@i', $content_type);
-        if ($is_multipart && !ini_get('enable_post_data_reading')) {
-          $multipart_parser = id(new AphrontMultipartParser())
-            ->setContentType($content_type);
-
-          $multipart_parser->beginParse();
-          $multipart_parser->continueParse($raw_input);
-          $parts = $multipart_parser->endParse();
-
-          $query_string = array();
-          foreach ($parts as $part) {
-            if (!$part->isVariable()) {
-              continue;
-            }
-
-            $name = $part->getName();
-            $value = $part->getVariableValue();
-
-            $query_string[] = urlencode($name).'='.urlencode($value);
-          }
-          $query_string = implode('&', $query_string);
-          $post = $parser->parseQueryString($query_string);
-
-          $files = array();
-          foreach ($parts as $part) {
-            if ($part->isVariable()) {
-              continue;
-            }
-
-            $files[$part->getName()] = $part->getPHPFileDictionary();
-          }
-          $_FILES = $files;
-        } else {
-          $post = $parser->parseQueryString($raw_input);
-        }
-
-        $_POST = $post;
-        PhabricatorStartup::rebuildRequest();
-
-        $data += $post;
-      } else if ($_POST) {
-        $post = filter_input_array(INPUT_POST, FILTER_UNSAFE_RAW);
-        if (is_array($post)) {
-          $_POST = $post;
-          PhabricatorStartup::rebuildRequest();
-        }
-        $data += $_POST;
-      }
-    }
-
-    $data += $parser->parseQueryString(idx($_SERVER, 'QUERY_STRING', ''));
-
-    $cookie_prefix = PhabricatorEnv::getEnvConfig('phabricator.cookie-prefix');
-
-    $request = new AphrontRequest($this->getHost(), $this->getPath());
-    $request->setRequestData($data);
-    $request->setApplicationConfiguration($this);
-    $request->setCookiePrefix($cookie_prefix);
-
-    return $request;
-  }
-
-  public function build404Controller() {
-    return array(new Phabricator404Controller(), array());
-  }
-
-  public function buildRedirectController($uri, $external) {
-    return array(
-      new PhabricatorRedirectController(),
-      array(
-        'uri' => $uri,
-        'external' => $external,
-      ),
-    );
-  }
-
-}

src/applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php

@@ -12,7 +12,7 @@ final class PhabricatorAccessControlTestCase extends PhabricatorTestCase {
     $root = dirname(phutil_get_library_root('phabricator'));
     require_once $root.'/support/startup/PhabricatorStartup.php';
 
-    $application_configuration = new AphrontDefaultApplicationConfiguration();
+    $application_configuration = new AphrontApplicationConfiguration();
 
     $host = 'meow.example.com';
 

src/applications/config/check/PhabricatorExtraConfigSetupCheck.php

@@ -416,6 +416,10 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck {
       'metamta.pholio.subject-prefix' => $prefix_reason,
       'metamta.phriction.subject-prefix' => $prefix_reason,
 
+      'aphront.default-application-configuration-class' => pht(
+        'This ancient extension point has been replaced with other '.
+        'mechanisms, including "AphrontSite".'),
+
     );
 
     return $ancient_config;

src/applications/config/option/PhabricatorExtendingPhabricatorConfigOptions.php

@@ -36,14 +36,6 @@ final class PhabricatorExtendingPhabricatorConfigOptions
             'occur. Specify a list of classes which extend '.
             'PhabricatorEventListener here.'))
         ->addExample('MyEventListener', pht('Valid Setting')),
-       $this->newOption(
-         'aphront.default-application-configuration-class',
-         'class',
-         'AphrontDefaultApplicationConfiguration')
-        ->setLocked(true)
-        ->setBaseClass('AphrontApplicationConfiguration')
-        // TODO: This could probably use some better documentation.
-        ->setDescription(pht('Application configuration class.')),
     );
   }