Fix an issue with URIs missing trailing slashes, like "/maniphest"

Summary:
In D3144, I made us look in application maps to find routing rules. However, we don't process //all// the maps when we 404 and try to do a "/" redirect. Process all of the maps.

Additionally, in D3146 I made the menu items application-driven. However, some pages (like 404) don't have a controller. Drop the requirement that the controller be nonnull.

Test Plan:
  - Visited "/maniphest", got a redirect after this patch.
  - Visited "/asldknfalksfn", got a 404 after this patch.

Reviewers: davidreuss, vrana, btrahan

Reviewed By: davidreuss

CC: aran

Maniphest Tasks: T1607

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

src/aphront/configuration/AphrontApplicationConfiguration.php

@@ -17,6 +17,7 @@
  */
 
 /**
+ * @task  routing URI Routing
  * @group aphront
  */
 abstract class AphrontApplicationConfiguration {
@@ -50,58 +51,6 @@ abstract class AphrontApplicationConfiguration {
     return $this;
   }
 
-  final public function buildController() {
-    $request = $this->getRequest();
-    $path = $request->getPath();
-
-    $maps = array();
-    $maps[] = array(null, $this->getURIMap());
-
-    $applications = PhabricatorApplication::getAllInstalledApplications();
-    foreach ($applications as $application) {
-      $maps[] = array($application, $application->getRoutes());
-    }
-
-    $current_application = null;
-    foreach ($maps as $map_info) {
-      list($application, $map) = $map_info;
-
-      $mapper = new AphrontURIMapper($map);
-      list($controller_class, $uri_data) = $mapper->mapPath($path);
-
-      if ($controller_class) {
-        if ($application) {
-          $current_application = $application;
-        }
-        break;
-      }
-    }
-
-    if (!$controller_class) {
-      if (!preg_match('@/$@', $path)) {
-        // If we failed to match anything but don't have a trailing slash, try
-        // to add a trailing slash and issue a redirect if that resolves.
-        list($controller_class, $uri_data) = $mapper->mapPath($path.'/');
-
-        // NOTE: For POST, just 404 instead of redirecting, since the redirect
-        // will be a GET without parameters.
-        if ($controller_class && !$request->isHTTPPost()) {
-          $uri = $request->getRequestURI()->setPath($path.'/');
-          return $this->buildRedirectController($uri);
-        }
-      }
-
-      return $this->build404Controller();
-    }
-
-    $controller = newv($controller_class, array($request));
-    if ($current_application) {
-      $controller->setCurrentApplication($current_application);
-    }
-
-    return array($controller, $uri_data);
-  }
-
   final public function setHost($host) {
     $this->host = $host;
     return $this;
@@ -135,4 +84,107 @@ abstract class AphrontApplicationConfiguration {
     return;
   }
 
+
+/* -(  URI Routing  )-------------------------------------------------------- */
+
+
+  /**
+   * Using builtin and application routes, build the appropriate
+   * @{class:AphrontController} class for the request. To route a request, we
+   * test the URI against all builtin routes from @{method:getURIMap}, then
+   * against all application routes from installed
+   * @{class:PhabricatorApplication}s.
+   *
+   * If we match a route, we construct the controller it points at, build it,
+   * and return it.
+   *
+   * If we fail to match a route, but the current path is missing a trailing
+   * "/", we try routing the same path with a trailing "/" and do a redirect
+   * if that has a valid route. The idea is to canoncalize URIs for consistency,
+   * but avoid breaking noncanonical URIs that we can easily salvage.
+   *
+   * NOTE: We only redirect on GET. On POST, we'd drop parameters and most
+   * likely mutate the request implicitly, and a bad POST usually indicates a
+   * programming error rather than a sloppy typist.
+   *
+   * If the failing path already has a trailing "/", or we can't route the
+   * version with a "/", we call @{method:build404Controller}, which build a
+   * fallback @{class:AphrontController}.
+   *
+   * @return pair<AphrontController,dict> Controller and dictionary of request
+   *                                      parameters.
+   * @task routing
+   */
+  final public function buildController() {
+    $request = $this->getRequest();
+    $path = $request->getPath();
+
+    list($controller, $uri_data) = $this->buildControllerForPath($path);
+    if (!$controller) {
+      if (!preg_match('@/$@', $path)) {
+        // If we failed to match anything but don't have a trailing slash, try
+        // to add a trailing slash and issue a redirect if that resolves.
+        list($controller, $uri_data) = $this->buildControllerForPath($path.'/');
+
+        // NOTE: For POST, just 404 instead of redirecting, since the redirect
+        // will be a GET without parameters.
+
+        if ($controller && !$request->isHTTPPost()) {
+          $uri = $request->getRequestURI()->setPath($path.'/');
+          return $this->buildRedirectController($uri);
+        }
+      }
+      return $this->build404Controller();
+    }
+
+    return array($controller, $uri_data);
+  }
+
+
+  /**
+   * Map a specific path to the corresponding controller. For a description
+   * of routing, see @{method:buildController}.
+   *
+   * @return pair<AphrontController,dict> Controller and dictionary of request
+   *                                      parameters.
+   * @task routing
+   */
+  private function buildControllerForPath($path) {
+    $maps = array();
+    $maps[] = array(null, $this->getURIMap());
+
+    $applications = PhabricatorApplication::getAllInstalledApplications();
+    foreach ($applications as $application) {
+      $maps[] = array($application, $application->getRoutes());
+    }
+
+    $current_application = null;
+    $controller_class = null;
+    foreach ($maps as $map_info) {
+      list($application, $map) = $map_info;
+
+      $mapper = new AphrontURIMapper($map);
+      list($controller_class, $uri_data) = $mapper->mapPath($path);
+
+      if ($controller_class) {
+        if ($application) {
+          $current_application = $application;
+        }
+        break;
+      }
+    }
+
+    if (!$controller_class) {
+      return array(null, null);
+    }
+
+    $request = $this->getRequest();
+
+    $controller = newv($controller_class, array($request));
+    if ($current_application) {
+      $controller->setCurrentApplication($current_application);
+    }
+
+    return array($controller, $uri_data);
+  }
 }

src/applications/auth/application/PhabricatorApplicationAuth.php

@@ -24,7 +24,7 @@ final class PhabricatorApplicationAuth extends PhabricatorApplication {
 
   public function buildMainMenuItems(
     PhabricatorUser $user,
-    PhabricatorController $controller) {
+    PhabricatorController $controller = null) {
 
     $items = array();
 

src/applications/base/PhabricatorApplication.php

@@ -18,6 +18,7 @@
 
 /**
  * @task  info  Application Information
+ * @task  ui    UI Integration
  * @task  uri   URI Routing
  * @task  fact  Fact Integration
  * @task  meta  Application Management
@@ -78,16 +79,26 @@ abstract class PhabricatorApplication {
   }
 
 
-/* -(  Launch Integration  )------------------------------------------------- */
+/* -(  UI Integration  )----------------------------------------------------- */
 
 
   public function loadStatus(PhabricatorUser $user) {
     return array();
   }
 
+
+  /**
+   * Build items for the main menu.
+   *
+   * @param  PhabricatorUser    The viewing user.
+   * @param  AphrontController  The current controller. May be null for special
+   *                            pages like 404, exception handlers, etc.
+   * @return list<PhabricatorMainMenuIconView> List of menu items.
+   * @task UI
+   */
   public function buildMainMenuItems(
     PhabricatorUser $user,
-    PhabricatorController $controller) {
+    PhabricatorController $controller = null) {
     return array();
   }
 

src/applications/people/application/PhabricatorApplicationPeople.php

@@ -46,7 +46,7 @@ final class PhabricatorApplicationPeople extends PhabricatorApplication {
 
   public function buildMainMenuItems(
     PhabricatorUser $user,
-    PhabricatorController $controller) {
+    PhabricatorController $controller = null) {
 
     $items = array();
 

src/applications/people/application/PhabricatorApplicationSettings.php

@@ -40,7 +40,7 @@ final class PhabricatorApplicationSettings extends PhabricatorApplication {
 
   public function buildMainMenuItems(
     PhabricatorUser $user,
-    PhabricatorController $controller) {
+    PhabricatorController $controller = null) {
 
     $items = array();