From fb3b4ee532acc7bf1ec22c5568318d5bab51495e Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 5 Jan 2016 04:42:07 -0800 Subject: [PATCH] Make CommitController more flexible about handling URIs Summary: Ref T4245. This adds support for both ID-based and callsign-based routes, although the ID-based routes don't occur anywhere. Also moves toward simplifying the DiffusionRequest stuff. Test Plan: Visited normal callsign-based commit pages; visited new ID-based commit pages. Reviewers: chad Reviewed By: chad Maniphest Tasks: T4245 Differential Revision: https://secure.phabricator.com/D14940 --- .../PhabricatorDiffusionApplication.php | 7 ++- .../controller/DiffusionCommitController.php | 18 +++----- .../controller/DiffusionController.php | 45 ++++++++++++++++++- .../diffusion/request/DiffusionRequest.php | 6 ++- .../query/PhabricatorRepositoryQuery.php | 2 +- .../storage/PhabricatorRepositoryCommit.php | 8 +++- 6 files changed, 68 insertions(+), 18 deletions(-) diff --git a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php index cbbfff8763..db967bad63 100644 --- a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php +++ b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php @@ -47,8 +47,13 @@ final class PhabricatorDiffusionApplication extends PhabricatorApplication { public function getRoutes() { return array( - '/r(?P[A-Z]+)(?P[a-z0-9]+)' + '/(?:'. + 'r(?P[A-Z]+)'. + '|'. + 'R(?P[1-9]\d*):'. + ')(?P[a-f0-9]+)' => 'DiffusionCommitController', + '/diffusion/' => array( '(?:query/(?P[^/]+)/)?' => 'DiffusionRepositoryListController', diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index 42a64f416f..2b0e589687 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -17,20 +17,16 @@ final class DiffusionCommitController extends DiffusionController { return true; } - protected function shouldLoadDiffusionRequest() { - return false; - } + public function handleRequest(AphrontRequest $request) { + $response = $this->loadDiffusionContext(); + if ($response) { + return $response; + } + + $drequest = $this->getDiffusionRequest(); - protected function processDiffusionRequest(AphrontRequest $request) { $user = $request->getUser(); - // This controller doesn't use blob/path stuff, just pass the dictionary - // in directly instead of using the AphrontRequest parsing mechanism. - $data = $request->getURIMap(); - $data['user'] = $user; - $drequest = DiffusionRequest::newFromDictionary($data); - $this->diffusionRequest = $drequest; - if ($request->getStr('diff')) { return $this->buildRawDiffResponse($drequest); } diff --git a/src/applications/diffusion/controller/DiffusionController.php b/src/applications/diffusion/controller/DiffusionController.php index 89446e132f..daff943438 100644 --- a/src/applications/diffusion/controller/DiffusionController.php +++ b/src/applications/diffusion/controller/DiffusionController.php @@ -35,7 +35,7 @@ abstract class DiffusionController extends PhabricatorController { return true; } - final public function handleRequest(AphrontRequest $request) { + public function handleRequest(AphrontRequest $request) { if ($request->getURIData('callsign') && $this->shouldLoadDiffusionRequest()) { try { @@ -48,10 +48,51 @@ abstract class DiffusionController extends PhabricatorController { } $this->setDiffusionRequest($drequest); } + return $this->processDiffusionRequest($request); } - abstract protected function processDiffusionRequest(AphrontRequest $request); + protected function loadDiffusionContext() { + $request = $this->getRequest(); + $viewer = $this->getViewer(); + + $identifier = $request->getURIData('repositoryCallsign'); + if (!strlen($identifier)) { + $identifier = (int)$request->getURIData('repositoryID'); + } + + $blob = $request->getURIData('dblob'); + if (strlen($blob)) { + $parsed = DiffusionRequest::parseRequestBlob($blob); + } else { + $parsed = array( + 'commit' => $request->getURIData('commit'), + 'path' => $request->getURIData('path'), + 'line' => $request->getURIData('line'), + 'branch' => $request->getURIData('branch'), + 'lint' => $request->getStr('lint'), + ); + } + + $params = array( + 'repository' => $identifier, + 'user' => $viewer, + ) + $parsed; + + $drequest = DiffusionRequest::newFromDictionary($params); + + if (!$drequest) { + return new Aphront404Response(); + } + + $this->diffusionRequest = $drequest; + + return null; + } + + protected function processDiffusionRequest(AphrontRequest $request) { + throw new PhutilMethodNotImplementedException(); + } public function buildCrumbs(array $spec = array()) { $crumbs = $this->buildApplicationCrumbs(); diff --git a/src/applications/diffusion/request/DiffusionRequest.php b/src/applications/diffusion/request/DiffusionRequest.php index 5f10617105..a9a062167b 100644 --- a/src/applications/diffusion/request/DiffusionRequest.php +++ b/src/applications/diffusion/request/DiffusionRequest.php @@ -105,6 +105,10 @@ abstract class DiffusionRequest extends Phobject { $object = self::newFromRepository($repository); } + if (!$object) { + return null; + } + $object->initializeFromDictionary($data); return $object; @@ -175,7 +179,7 @@ abstract class DiffusionRequest extends Phobject { ->executeOne(); if (!$repository) { - throw new Exception(pht("No such repository '%s'.", $identifier)); + return null; } return self::newFromRepository($repository); diff --git a/src/applications/repository/query/PhabricatorRepositoryQuery.php b/src/applications/repository/query/PhabricatorRepositoryQuery.php index 9bb15d1804..38fa5bf56c 100644 --- a/src/applications/repository/query/PhabricatorRepositoryQuery.php +++ b/src/applications/repository/query/PhabricatorRepositoryQuery.php @@ -55,7 +55,7 @@ final class PhabricatorRepositoryQuery $monograms = array(); foreach ($identifiers as $identifier) { - if (ctype_digit($identifier)) { + if (ctype_digit((string)$identifier)) { $ids[$identifier] = $identifier; } else if (preg_match('/^(r[A-Z]+)|(R[1-9]\d*)\z/', $identifier)) { $monograms[$identifier] = $identifier; diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index 1db4b71d5c..987c42492a 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -252,8 +252,12 @@ final class PhabricatorRepositoryCommit $repository = $this->getRepository(); $callsign = $repository->getCallsign(); $identifier = $this->getCommitIdentifier(); - - return "r{$callsign}{$identifier}"; + if ($callsign !== null) { + return "r{$callsign}{$identifier}"; + } else { + $id = $repository->getID(); + return "R{$id}:{$identifier}"; + } } public function getDisplayName() {