From ebb7807bb41e2aeb502ecfcc9e775e3eaaaf8ce7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 6 Aug 2012 12:46:51 -0700 Subject: [PATCH] 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 --- .../AphrontApplicationConfiguration.php | 156 ++++++++++++------ .../PhabricatorApplicationAuth.php | 2 +- .../base/PhabricatorApplication.php | 15 +- .../PhabricatorApplicationPeople.php | 2 +- .../PhabricatorApplicationSettings.php | 2 +- 5 files changed, 120 insertions(+), 57 deletions(-) diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php index 863a26070a..5d54f6f827 100644 --- a/src/aphront/configuration/AphrontApplicationConfiguration.php +++ b/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 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 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); + } } diff --git a/src/applications/auth/application/PhabricatorApplicationAuth.php b/src/applications/auth/application/PhabricatorApplicationAuth.php index e4c32594e8..f6f2ba430f 100644 --- a/src/applications/auth/application/PhabricatorApplicationAuth.php +++ b/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(); diff --git a/src/applications/base/PhabricatorApplication.php b/src/applications/base/PhabricatorApplication.php index 1419161e47..d91f15f3f9 100644 --- a/src/applications/base/PhabricatorApplication.php +++ b/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 List of menu items. + * @task UI + */ public function buildMainMenuItems( PhabricatorUser $user, - PhabricatorController $controller) { + PhabricatorController $controller = null) { return array(); } diff --git a/src/applications/people/application/PhabricatorApplicationPeople.php b/src/applications/people/application/PhabricatorApplicationPeople.php index 7812da491c..7889c9b6a7 100644 --- a/src/applications/people/application/PhabricatorApplicationPeople.php +++ b/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(); diff --git a/src/applications/people/application/PhabricatorApplicationSettings.php b/src/applications/people/application/PhabricatorApplicationSettings.php index 20fc3f55f6..963b526e25 100644 --- a/src/applications/people/application/PhabricatorApplicationSettings.php +++ b/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();