diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a56b3c3a3d..cb5645fc94 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -340,6 +340,7 @@ phutil_register_library_map(array( 'DiffusionSvnRequest' => 'applications/diffusion/request/svn', 'DiffusionSymbolController' => 'applications/diffusion/controller/symbol', 'DiffusionSymbolQuery' => 'applications/diffusion/query/symbol', + 'DiffusionURITestCase' => 'applications/diffusion/request/base/__tests__', 'DiffusionView' => 'applications/diffusion/view/base', 'DrydockAllocator' => 'applications/drydock/allocator/resource', 'DrydockAllocatorWorker' => 'applications/drydock/allocator/worker', @@ -1188,6 +1189,7 @@ phutil_register_library_map(array( 'DiffusionSvnLastModifiedQuery' => 'DiffusionLastModifiedQuery', 'DiffusionSvnRequest' => 'DiffusionRequest', 'DiffusionSymbolController' => 'DiffusionController', + 'DiffusionURITestCase' => 'ArcanistPhutilTestCase', 'DiffusionView' => 'AphrontView', 'DrydockAllocatorWorker' => 'PhabricatorWorker', 'DrydockCommandInterface' => 'DrydockInterface', diff --git a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php index 25b2e03211..113f9b824f 100644 --- a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php @@ -247,30 +247,13 @@ class AphrontDefaultApplicationConfiguration '' => 'DiffusionHomeController', '(?P[A-Z]+)/' => array( '' => 'DiffusionRepositoryController', - 'repository/'. - '(?P[^/]+)/' - => 'DiffusionRepositoryController', - 'change/'. - '(?P.*?)'. - '(?:[;](?P[a-z0-9]+))?' - => 'DiffusionChangeController', - 'history/'. - '(?P.*?)'. - '(?:[;](?P[a-z0-9]+))?' - => 'DiffusionHistoryController', - 'browse/'. - '(?P.*?)'. - '(?:[;](?P[a-z0-9]+))?'. - '(?:[$](?P\d+(?:-\d+)?))?' - => 'DiffusionBrowseController', - 'diff/'. - '(?P.*?)'. - '(?:[;](?P[a-z0-9]+))?' - => 'DiffusionDiffController', - 'lastmodified/'. - '(?P.*?)'. - '(?:[;](?P[a-z0-9]+))?' - => 'DiffusionLastModifiedController', + + 'repository/(?P.*)' => 'DiffusionRepositoryController', + 'change/(?P.*)' => 'DiffusionChangeController', + 'history/(?P.*)' => 'DiffusionHistoryController', + 'browse/(?P.*)' => 'DiffusionBrowseController', + 'lastmodified/(?P.*)' => 'DiffusionLastModifiedController', + 'diff/' => 'DiffusionDiffController', ), 'inline/(?P[^/]+)/' => 'DiffusionInlineCommentController', 'services/' => array( diff --git a/src/applications/conduit/method/diffusion/getrecentcommitsbypath/ConduitAPI_diffusion_getrecentcommitsbypath_Method.php b/src/applications/conduit/method/diffusion/getrecentcommitsbypath/ConduitAPI_diffusion_getrecentcommitsbypath_Method.php index b409c0f2c3..c46c49ef74 100644 --- a/src/applications/conduit/method/diffusion/getrecentcommitsbypath/ConduitAPI_diffusion_getrecentcommitsbypath_Method.php +++ b/src/applications/conduit/method/diffusion/getrecentcommitsbypath/ConduitAPI_diffusion_getrecentcommitsbypath_Method.php @@ -45,13 +45,13 @@ final class ConduitAPI_diffusion_getrecentcommitsbypath_Method } protected function execute(ConduitAPIRequest $request) { - $results = array(); + $drequest = DiffusionRequest::newFromDictionary( + array( + 'callsign' => $request->getValue('callsign'), + 'path' => $request->getValue('path'), + )); - $history = DiffusionHistoryQuery::newFromDiffusionRequest( - DiffusionRequest::newFromAphrontRequestDictionary( - $request->getAllParameters() - ) - ) + $history = DiffusionHistoryQuery::newFromDiffusionRequest($drequest) ->setLimit(self::RESULT_LIMIT) ->needDirectChanges(true) ->needChildChanges(true) diff --git a/src/applications/diffusion/controller/base/DiffusionController.php b/src/applications/diffusion/controller/base/DiffusionController.php index eee14ec0c6..b128f52dd8 100644 --- a/src/applications/diffusion/controller/base/DiffusionController.php +++ b/src/applications/diffusion/controller/base/DiffusionController.php @@ -21,8 +21,10 @@ abstract class DiffusionController extends PhabricatorController { protected $diffusionRequest; public function willProcessRequest(array $data) { - $this->diffusionRequest = DiffusionRequest::newFromAphrontRequestDictionary( - $data); + if (isset($data['callsign'])) { + $drequest = DiffusionRequest::newFromAphrontRequestDictionary($data); + $this->diffusionRequest = $drequest; + } } public function setDiffusionRequest(DiffusionRequest $request) { @@ -31,6 +33,9 @@ abstract class DiffusionController extends PhabricatorController { } protected function getDiffusionRequest() { + if (!$this->diffusionRequest) { + throw new Exception("No Diffusion request object!"); + } return $this->diffusionRequest; } @@ -73,37 +78,34 @@ abstract class DiffusionController extends PhabricatorController { } $drequest = $this->getDiffusionRequest(); - $repository = $drequest->getRepository(); - $callsign = $repository->getCallsign(); - $branch_uri = $drequest->getBranchURIComponent($drequest->getBranch()); - $path_uri = $branch_uri.$drequest->getPath(); + foreach ($navs as $action => $name) { + $href = $drequest->generateURI( + array( + 'action' => $action, + )); - $commit_uri = null; - $raw_commit = $drequest->getRawCommit(); - if ($raw_commit) { - $commit_uri = ';'.$drequest->getCommitURIComponent($raw_commit); - } - - foreach ($navs as $uri => $name) { $nav->addNavItem( phutil_render_tag( 'a', array( - 'href' => "/diffusion/{$callsign}/{$uri}/{$path_uri}{$commit_uri}", + 'href' => $href, 'class' => - ($uri == $selected + ($action == $selected ? 'aphront-side-nav-selected' : null), ), $name)); } + + // TODO: URI encoding might need to be sorted out for this link. + $nav->addNavItem( phutil_render_tag( 'a', array( 'href' => '/owners/view/search/'. - '?repository='.phutil_escape_uri($callsign). + '?repository='.phutil_escape_uri($drequest->getCallsign()). '&path='.phutil_escape_uri('/'.$drequest->getPath()), ), 'Search Owners')); @@ -158,11 +160,18 @@ abstract class DiffusionController extends PhabricatorController { } private function buildCrumbList(array $spec = array()) { - $drequest = $this->getDiffusionRequest(); $crumb_list = array(); - $repository = $drequest->getRepository(); + // On the home page, we don't have a DiffusionRequest. + if ($this->diffusionRequest) { + $drequest = $this->getDiffusionRequest(); + $repository = $drequest->getRepository(); + } else { + $drequest = null; + $repository = null; + } + if ($repository) { $crumb_list[] = phutil_render_tag( 'a', @@ -231,14 +240,9 @@ abstract class DiffusionController extends PhabricatorController { return $crumb_list; } - $branch_uri = $drequest->getBranchURIComponent($drequest->getBranch()); - $view_root_uri = "/diffusion/{$callsign}/{$view}/{$branch_uri}"; - $jump_href = $view_root_uri; - - $view_tail_uri = null; - if ($raw_commit) { - $view_tail_uri = ';'.$drequest->getCommitURIComponent($raw_commit); - } + $uri_params = array( + 'action' => $view, + ); if (!strlen($path)) { $crumb_list[] = $view_name; @@ -247,7 +251,10 @@ abstract class DiffusionController extends PhabricatorController { $crumb_list[] = phutil_render_tag( 'a', array( - 'href' => $view_root_uri.$view_tail_uri, + 'href' => $drequest->generateURI( + array( + 'path' => null, + ) + $uri_params), ), $view_name); @@ -263,7 +270,10 @@ abstract class DiffusionController extends PhabricatorController { $path_sections[] = phutil_render_tag( 'a', array( - 'href' => $view_root_uri.$thus_far.$view_tail_uri, + 'href' => $drequest->generateURI( + array( + 'path' => $thus_far, + ) + $uri_params), ), phutil_escape_html($path_part)); } @@ -271,8 +281,6 @@ abstract class DiffusionController extends PhabricatorController { $path_sections[] = phutil_escape_html($last); $path_sections = '/'.implode('/', $path_sections); - $jump_href = $view_root_uri.$thus_far.$last; - $crumb_list[] = $path_sections; } @@ -282,7 +290,10 @@ abstract class DiffusionController extends PhabricatorController { $jump_link = phutil_render_tag( 'a', array( - 'href' => $jump_href, + 'href' => $drequest->generateURI( + array( + 'commit' => null, + ) + $uri_params), ), 'Jump to HEAD'); $last_crumb .= " @ {$commit_link} ({$jump_link})"; diff --git a/src/applications/diffusion/controller/commit/DiffusionCommitController.php b/src/applications/diffusion/controller/commit/DiffusionCommitController.php index de35431294..8e48354004 100644 --- a/src/applications/diffusion/controller/commit/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/commit/DiffusionCommitController.php @@ -20,6 +20,13 @@ final class DiffusionCommitController extends DiffusionController { const CHANGES_LIMIT = 100; + public function willProcessRequest(array $data) { + // This controller doesn't use blob/path stuff, just pass the dictionary + // in directly instead of using the AphrontRequest parsing mechanism. + $drequest = DiffusionRequest::newFromDictionary($data); + $this->diffusionRequest = $drequest; + } + public function processRequest() { $drequest = $this->getDiffusionRequest(); $request = $this->getRequest(); @@ -192,11 +199,11 @@ final class DiffusionCommitController extends DiffusionController { } } - $branch = $drequest->getBranchURIComponent( - $drequest->getBranch()); - $filename = $changeset->getFilename(); - $reference = "{$branch}{$filename};".$drequest->getCommit(); - $references[$key] = $reference; + $references[$key] = $drequest->generateURI( + array( + 'action' => 'rendering-ref', + 'path' => $changeset->getFilename(), + )); } // TOOD: Some parts of the views still rely on properties of the diff --git a/src/applications/diffusion/controller/commit/__init__.php b/src/applications/diffusion/controller/commit/__init__.php index 1b04ed438a..ebcda3060d 100644 --- a/src/applications/diffusion/controller/commit/__init__.php +++ b/src/applications/diffusion/controller/commit/__init__.php @@ -19,6 +19,7 @@ phutil_require_module('phabricator', 'applications/diffusion/data/pathchange'); phutil_require_module('phabricator', 'applications/diffusion/query/contains/base'); phutil_require_module('phabricator', 'applications/diffusion/query/pathchange/base'); phutil_require_module('phabricator', 'applications/diffusion/query/pathid/base'); +phutil_require_module('phabricator', 'applications/diffusion/request/base'); phutil_require_module('phabricator', 'applications/diffusion/view/commentlist'); phutil_require_module('phabricator', 'applications/diffusion/view/commitchangetable'); phutil_require_module('phabricator', 'applications/draft/storage/draft'); diff --git a/src/applications/diffusion/controller/diff/DiffusionDiffController.php b/src/applications/diffusion/controller/diff/DiffusionDiffController.php index 606cf79c82..5da607e117 100644 --- a/src/applications/diffusion/controller/diff/DiffusionDiffController.php +++ b/src/applications/diffusion/controller/diff/DiffusionDiffController.php @@ -19,15 +19,12 @@ final class DiffusionDiffController extends DiffusionController { public function willProcessRequest(array $data) { - $request = $this->getRequest(); - if ($request->getStr('ref')) { - $parts = explode(';', $request->getStr('ref')); - $data['path'] = idx($parts, 0); - $data['commit'] = idx($parts, 1); - } + $data = $data + array( + 'dblob' => $this->getRequest()->getStr('ref'), + ); + $drequest = DiffusionRequest::newFromAphrontRequestDictionary($data); - $this->diffusionRequest = DiffusionRequest::newFromAphrontRequestDictionary( - $data); + $this->diffusionRequest = $drequest; } public function processRequest() { diff --git a/src/applications/diffusion/controller/file/DiffusionBrowseFileController.php b/src/applications/diffusion/controller/file/DiffusionBrowseFileController.php index b8d1dfc4c9..bf559965c4 100644 --- a/src/applications/diffusion/controller/file/DiffusionBrowseFileController.php +++ b/src/applications/diffusion/controller/file/DiffusionBrowseFileController.php @@ -403,22 +403,27 @@ final class DiffusionBrowseFileController extends DiffusionController { $targ = null; } - // Create the row display. - $uri_path = $drequest->getUriPath(); - $uri_rev = $drequest->getStableCommitName(); - $uri_view = $view - ? '?view='.$view - : null; + $href = $drequest->generateURI( + array( + 'action' => 'browse', + 'stable' => true, + )); + $href = (string)$href; - $l = phutil_render_tag( + $query_params = null; + if ($view) { + $query_params = '?view='.$view; + } + + $link = phutil_render_tag( 'a', array( - 'href' => $uri_path.';'.$uri_rev.'$'.$n.$uri_view, + 'href' => $href.'$'.$n.$query_params, ), $n); $rows[] = $tr.$blame_info. - ''.$l.''. + ''.$link.''. ''.$targ.$line.''; ++$n; } diff --git a/src/applications/diffusion/controller/pathcomplete/DiffusionPathCompleteController.php b/src/applications/diffusion/controller/pathcomplete/DiffusionPathCompleteController.php index 2efcbdd60f..69ed227b71 100644 --- a/src/applications/diffusion/controller/pathcomplete/DiffusionPathCompleteController.php +++ b/src/applications/diffusion/controller/pathcomplete/DiffusionPathCompleteController.php @@ -44,10 +44,10 @@ final class DiffusionPathCompleteController extends DiffusionController { } } - $drequest = DiffusionRequest::newFromAphrontRequestDictionary( + $drequest = DiffusionRequest::newFromDictionary( array( - 'callsign' => $repository->getCallsign(), - 'path' => ':/'.$query_dir, + 'repository' => $repository, + 'path' => $query_dir, )); $browse_query = DiffusionBrowseQuery::newFromDiffusionRequest($drequest); diff --git a/src/applications/diffusion/controller/pathvalidate/DiffusionPathValidateController.php b/src/applications/diffusion/controller/pathvalidate/DiffusionPathValidateController.php index d539c4db39..6a74f03055 100644 --- a/src/applications/diffusion/controller/pathvalidate/DiffusionPathValidateController.php +++ b/src/applications/diffusion/controller/pathvalidate/DiffusionPathValidateController.php @@ -36,10 +36,10 @@ final class DiffusionPathValidateController extends DiffusionController { $path = $request->getStr('path'); $path = ltrim($path, '/'); - $drequest = DiffusionRequest::newFromAphrontRequestDictionary( + $drequest = DiffusionRequest::newFromDictionary( array( - 'callsign' => $repository->getCallsign(), - 'path' => ':/'.$path, + 'repository' => $repository, + 'path' => $path, )); $browse_query = DiffusionBrowseQuery::newFromDiffusionRequest($drequest); diff --git a/src/applications/diffusion/query/diff/git/DiffusionGitDiffQuery.php b/src/applications/diffusion/query/diff/git/DiffusionGitDiffQuery.php index e2ce756485..b7a34f4d83 100644 --- a/src/applications/diffusion/query/diff/git/DiffusionGitDiffQuery.php +++ b/src/applications/diffusion/query/diff/git/DiffusionGitDiffQuery.php @@ -1,7 +1,7 @@ getRequest(); $repository = $drequest->getRepository(); - if (!$drequest->getRawCommit()) { - $effective_commit = $this->getEffectiveCommit(); - if (!$effective_commit) { - return null; - } - // TODO: This side effect is kind of skethcy. - $drequest->setCommit($effective_commit); - } else { - $effective_commit = $drequest->getCommit(); + $effective_commit = $this->getEffectiveCommit(); + if (!$effective_commit) { + return null; } + // TODO: This side effect is kind of skethcy. + $drequest->setCommit($effective_commit); $options = array( '-M', @@ -72,6 +68,10 @@ final class DiffusionGitDiffQuery extends DiffusionDiffQuery { } } + if (!$raw_diff) { + return null; + } + $parser = new ArcanistDiffParser(); $try_encoding = $repository->getDetail('encoding'); @@ -86,10 +86,10 @@ final class DiffusionGitDiffQuery extends DiffusionDiffQuery { $changesets = $diff->getChangesets(); $changeset = reset($changesets); - $this->renderingReference = - $drequest->getBranchURIComponent($drequest->getBranch()). - $drequest->getPath().';'. - $drequest->getCommit(); + $this->renderingReference = $drequest->generateURI( + array( + 'action' => 'rendering-ref', + )); return $changeset; } diff --git a/src/applications/diffusion/query/diff/mercurial/DiffusionMercurialDiffQuery.php b/src/applications/diffusion/query/diff/mercurial/DiffusionMercurialDiffQuery.php index 1df0c97a00..7cfe68e988 100644 --- a/src/applications/diffusion/query/diff/mercurial/DiffusionMercurialDiffQuery.php +++ b/src/applications/diffusion/query/diff/mercurial/DiffusionMercurialDiffQuery.php @@ -1,7 +1,7 @@ getChangesets(); $changeset = reset($changesets); - $this->renderingReference = - $drequest->getBranchURIComponent($drequest->getBranch()). - $drequest->getPath().';'. - $drequest->getCommit(); + $this->renderingReference = $drequest->generateURI( + array( + 'action' => 'rendering-ref', + )); return $changeset; } diff --git a/src/applications/diffusion/request/base/DiffusionRequest.php b/src/applications/diffusion/request/base/DiffusionRequest.php index 12d2dd3361..ec0ff3b289 100644 --- a/src/applications/diffusion/request/base/DiffusionRequest.php +++ b/src/applications/diffusion/request/base/DiffusionRequest.php @@ -17,10 +17,16 @@ */ /** - * TODO: This might need to be concrete-extensible, but straighten out the - * class hierarchy here. + * Contains logic to parse Diffusion requests, which have a complicated URI + * structure. + * + * + * @task new Creating Requests + * @task uri Managing Diffusion URIs + * + * @group diffusion */ -class DiffusionRequest { +abstract class DiffusionRequest { protected $callsign; protected $path; @@ -33,60 +39,152 @@ class DiffusionRequest { protected $repositoryCommitData; protected $stableCommitName; + abstract protected function getSupportsBranches(); + abstract protected function didInitialize(); + + +/* -( Creating Requests )-------------------------------------------------- */ + + + /** + * Create a new synthetic request from a parameter dictionary. If you need + * a @{class:DiffusionRequest} object in order to issue a DiffusionQuery, you + * can use this method to build one. + * + * Parameters are: + * + * - `callsign` Repository callsign. Provide this or `repository`. + * - `repository` Repository object. Provide this or `callsign`. + * - `branch` Optional, branch name. + * - `path` Optional, file path. + * - `commit` Optional, commit identifier. + * - `line` Optional, line range. + * + * @param map See documentation. + * @return DiffusionRequest New request object. + * @task new + */ + final public static function newFromDictionary(array $data) { + if (isset($data['repository']) && isset($data['callsign'])) { + throw new Exception( + "Specify 'repository' or 'callsign', but not both."); + } else if (!isset($data['repository']) && !isset($data['callsign'])) { + throw new Exception( + "One of 'repository' and 'callsign' is required."); + } + + if (isset($data['repository'])) { + $object = self::newFromRepository($data['repository']); + } else { + $object = self::newFromCallsign($data['callsign']); + } + $object->initializeFromDictionary($data); + return $object; + } + + + /** + * Create a new request from an Aphront request dictionary. This is an + * internal method that you generally should not call directly; instead, + * call @{method:newFromDictionary}. + * + * @param map Map of Aphront request data. + * @return DiffusionRequest New request object. + * @task new + */ + final public static function newFromAphrontRequestDictionary(array $data) { + $callsign = phutil_unescape_uri_path_component(idx($data, 'callsign')); + $object = self::newFromCallsign($callsign); + + $use_branches = $object->getSupportsBranches(); + $parsed = self::parseRequestBlob(idx($data, 'dblob'), $use_branches); + + $object->initializeFromDictionary($parsed); + return $object; + } + + + /** + * Internal. + * + * @task new + */ final private function __construct() { // } - final public static function newFromAphrontRequestDictionary(array $data) { - $vcs = null; - $repository = null; - $callsign = idx($data, 'callsign'); - if ($callsign) { - $repository = id(new PhabricatorRepository())->loadOneWhere( - 'callsign = %s', - $callsign); - if (!$repository) { - throw new Exception("No such repository '{$callsign}'."); - } - $vcs = $repository->getVersionControlSystem(); + /** + * Internal. Use @{method:newFromDictionary}, not this method. + * + * @param string Repository callsign. + * @return DiffusionRequest New request object. + * @task new + */ + final private static function newFromCallsign($callsign) { + $repository = id(new PhabricatorRepository())->loadOneWhere( + 'callsign = %s', + $callsign); + + if (!$repository) { + throw new Exception("No such repository '{$callsign}'."); } - switch ($vcs) { - case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: - $class = 'DiffusionGitRequest'; - break; - case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: - $class = 'DiffusionSvnRequest'; - break; - case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: - $class = 'DiffusionMercurialRequest'; - break; - default: - $class = 'DiffusionRequest'; - break; + return self::newFromRepository($repository); + } + + + /** + * Internal. Use @{method:newFromDictionary}, not this method. + * + * @param PhabricatorRepository Repository object. + * @return DiffusionRequest New request object. + * @task new + */ + final private static function newFromRepository( + PhabricatorRepository $repository) { + + $map = array( + PhabricatorRepositoryType::REPOSITORY_TYPE_GIT => 'DiffusionGitRequest', + PhabricatorRepositoryType::REPOSITORY_TYPE_SVN => 'DiffusionSvnRequest', + PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL => + 'DiffusionMercurialRequest', + ); + + $class = idx($map, $repository->getVersionControlSystem()); + + if (!$class) { + throw new Exception("Unknown version control system!"); } $object = new $class(); - $object->callsign = $callsign; $object->repository = $repository; - $object->line = idx($data, 'line'); - $object->commit = idx($data, 'commit'); - $object->path = idx($data, 'path'); - - $object->initializeFromAphrontRequestDictionary($data); + $object->callsign = $repository->getCallsign(); return $object; } - protected function initializeFromAphrontRequestDictionary(array $data) { + /** + * Internal. Use @{method:newFromDictionary}, not this method. + * + * @param map Map of parsed data. + * @return void + * @task new + */ + final private function initializeFromDictionary(array $data) { + $this->path = idx($data, 'path'); + $this->commit = idx($data, 'commit'); + $this->line = idx($data, 'line'); + + if ($this->getSupportsBranches()) { + $this->branch = idx($data, 'branch'); + } + + $this->didInitialize(); } - protected function parsePath($path) { - $this->path = $path; - } public function getRepository() { return $this->repository; @@ -100,10 +198,6 @@ class DiffusionRequest { return $this->path; } - public function getUriPath() { - return '/diffusion/'.$this->getCallsign().'/browse/'.$this->path; - } - public function getLine() { return $this->line; } @@ -166,12 +260,204 @@ class DiffusionRequest { return $this; } - public function getCommitURIComponent($commit) { - return $commit; +/* -( Managing Diffusion URIs )-------------------------------------------- */ + + + /** + * Generate a Diffusion URI using this request to provide defaults. See + * @{method:generateDiffusionURI} for details. This method is the same, but + * preserves the request parameters if they are not overridden. + * + * @param map See @{method:generateDiffusionURI}. + * @return PhutilURI Generated URI. + * @task uri + */ + public function generateURI(array $params) { + if (empty($params['stable'])) { + $default_commit = $this->getRawCommit(); + } else { + $default_commit = $this->getStableCommitName(); + } + + $params += array( + 'callsign' => $this->getCallsign(), + 'path' => $this->getPath(), + 'branch' => $this->getBranch(), + 'commit' => $default_commit, + ); + return self::generateDiffusionURI($params); } - public function getBranchURIComponent($branch) { - return $branch; + + /** + * Generate a Diffusion URI from a parameter map. Applies the correct encoding + * and formatting to the URI. Parameters are: + * + * - `action` One of `history`, `browse`, `change`, `lastmodified`, + * `branch` or `revision-ref`. The action specified by the URI. + * - `callsign` Repository callsign. + * - `branch` Optional if action is not `branch`, branch name. + * - `path` Optional, path to file. + * - `commit` Optional, commit identifier. + * - `line` Optional, line range. + * - `params` Optional, query parameters. + * + * The function generates the specified URI and returns it. + * + * @param map See documentation. + * @return PhutilURI Generated URI. + * @task uri + */ + public static function generateDiffusionURI(array $params) { + $action = idx($params, 'action'); + + $callsign = idx($params, 'callsign'); + $path = idx($params, 'path'); + $branch = idx($params, 'branch'); + $commit = idx($params, 'commit'); + $line = idx($params, 'line'); + + if (strlen($callsign)) { + $callsign = phutil_escape_uri_path_component($callsign).'/'; + } + + if (strlen($branch)) { + $branch = phutil_escape_uri_path_component($branch).'/'; + } + + if (strlen($path)) { + $path = str_replace(array(';', '$'), array(';;', '$$'), $path); + $path = phutil_escape_uri($path); + } + + $path = "{$branch}{$path}"; + + if (strlen($commit)) { + $commit = ';'.phutil_escape_uri($commit); + } + + if (strlen($line)) { + $line = '$'.phutil_escape_uri($line); + } + + $req_callsign = false; + $req_branch = false; + + switch ($action) { + case 'history': + case 'browse': + case 'change': + case 'lastmodified': + $req_callsign = true; + break; + case 'branch': + $req_callsign = true; + $req_branch = true; + break; + } + + if ($req_callsign && !strlen($callsign)) { + throw new Exception( + "Diffusion URI action '{$action}' requires callsign!"); + } + + if ($req_branch && !strlen($branch)) { + throw new Exception( + "Diffusion URI action '{$action}' requires brnach!"); + } + + switch ($action) { + case 'change': + case 'history': + case 'browse': + case 'lastmodified': + $uri = "/diffusion/{$callsign}{$action}/{$path}{$commit}{$line}"; + break; + case 'branch': + $uri = "/diffusion/{$callsign}repository/{$path}"; + break; + case 'rendering-ref': + // This isn't a real URI per se, it's passed as a query parameter to + // the ajax changeset stuff but then we parse it back out as though + // it came from a URI. + $uri = "{$path}{$commit}"; + break; + default: + throw new Exception("Unknown Diffusion URI action '{$action}'!"); + } + + if ($action == 'rendering-ref') { + return $uri; + } + + $uri = new PhutilURI($uri); + if (idx($params, 'params')) { + $uri->setQueryParams($params['params']); + } + + return $uri; + } + + + /** + * Internal. Public only for unit tests. + * + * Parse the request URI into components. + * + * @param string URI blob. + * @param bool True if this VCS supports branches. + * @return map Parsed URI. + * + * @task uri + */ + public static function parseRequestBlob($blob, $supports_branches) { + $result = array( + 'branch' => null, + 'path' => null, + 'commit' => null, + 'line' => null, + ); + + $matches = null; + + if ($supports_branches) { + // Consume the front part of the URI, up to the first "/". This is the + // path-component encoded branch name. + if (preg_match('@^([^/]+)/@', $blob, $matches)) { + $result['branch'] = phutil_unescape_uri_path_component($matches[1]); + $blob = substr($blob, strlen($matches[1]) + 1); + } + } + + // Consume the back part of the URI, up to the first "$". Use a negative + // lookbehind to prevent matching '$$'. We double the '$' symbol when + // encoding so that files with names like "money/$100" will survive. + if (preg_match('@(? array( + 'branch' => 'branch', + 'path' => 'path.ext', + 'commit' => 'abc', + 'line' => '3', + ), + 'branch/path.ext$3' => array( + 'branch' => 'branch', + 'path' => 'path.ext', + 'line' => '3', + ), + 'branch/money;;/$$100' => array( + 'branch' => 'branch', + 'path' => 'money;/$100', + ), + 'a%252Fb/' => array( + 'branch' => 'a/b', + ), + ); + + foreach ($map as $input => $expect) { + + // Simulate decode effect of the webserver. + $input = rawurldecode($input); + + $expect = $expect + array( + 'branch' => null, + 'path' => null, + 'commit' => null, + 'line' => null, + ); + $expect = array_select_keys( + $expect, + array('branch', 'path', 'commit', 'line')); + + $actual = $this->parseBlob($input); + + $this->assertEqual( + $expect, + $actual, + "Parsing '{$input}'"); + } + } + + public function testBlobDecodeFail() { + $this->tryTestCaseMap( + array( + 'branch/path/../../../secrets/secrets.key' => false, + ), + array($this, 'parseBlob')); + } + + public function parseBlob($blob) { + return DiffusionRequest::parseRequestBlob( + $blob, + $supports_branches = true); + } + + public function testURIGeneration() { + $map = array( + '/diffusion/A/browse/branch/path.ext;abc$1' => array( + 'action' => 'browse', + 'callsign' => 'A', + 'branch' => 'branch', + 'path' => 'path.ext', + 'commit' => 'abc', + 'line' => '1', + ), + '/diffusion/A/browse/a%252Fb/path.ext' => array( + 'action' => 'browse', + 'callsign' => 'A', + 'branch' => 'a/b', + 'path' => 'path.ext', + ), + '/diffusion/A/browse/%2B/%20%21' => array( + 'action' => 'browse', + 'callsign' => 'A', + 'path' => '+/ !', + ), + '/diffusion/A/browse/money/%24%24100$2' => array( + 'action' => 'browse', + 'callsign' => 'A', + 'path' => 'money/$100', + 'line' => '2', + ), + '/diffusion/A/browse/path/to/file.ext?view=things' => array( + 'action' => 'browse', + 'callsign' => 'A', + 'path' => 'path/to/file.ext', + 'params' => array( + 'view' => 'things', + ), + ), + '/diffusion/A/repository/master/' => array( + 'action' => 'branch', + 'callsign' => 'A', + 'branch' => 'master', + ), + 'path/to/file.ext;abc' => array( + 'action' => 'rendering-ref', + 'path' => 'path/to/file.ext', + 'commit' => 'abc', + ), + ); + + foreach ($map as $expect => $input) { + $actual = DiffusionRequest::generateDiffusionURI($input); + $this->assertEqual( + $expect, + (string)$actual); + } + } + +} diff --git a/src/applications/diffusion/request/base/__tests__/__init__.php b/src/applications/diffusion/request/base/__tests__/__init__.php new file mode 100644 index 0000000000..7ce064365b --- /dev/null +++ b/src/applications/diffusion/request/base/__tests__/__init__.php @@ -0,0 +1,16 @@ +path; - $parts = explode('/', $path); - - $branch = array_shift($parts); - if ($branch != ':') { - $this->branch = $this->decodeBranchName($branch); - } - - foreach ($parts as $key => $part) { - // Prevent any hyjinx since we're ultimately shipping this to the - // filesystem under a lot of git workflows. - if ($part == '..') { - unset($parts[$key]); - } - } - - $this->path = implode('/', $parts); + protected function getSupportsBranches() { + return true; + } + protected function didInitialize() { if ($this->repository) { $repository = $this->repository; @@ -109,11 +96,6 @@ final class DiffusionGitRequest extends DiffusionRequest { throw new Exception("Unable to determine branch!"); } - public function getUriPath() { - return '/diffusion/'.$this->getCallsign().'/browse/'. - $this->getBranchURIComponent($this->branch).$this->path; - } - public function getCommit() { if ($this->commit) { return $this->commit; @@ -126,26 +108,4 @@ final class DiffusionGitRequest extends DiffusionRequest { return substr($this->stableCommitName, 0, 16); } - public function getBranchURIComponent($branch) { - return $this->encodeBranchName($branch).'/'; - } - - private function decodeBranchName($branch) { - $branch = str_replace(':', '/', $branch); - - // Backward compatibility for older-style URIs which had an explicit - // "origin" remote in the branch name. If a remote is specified, strip it - // away. - if (strpos($branch, '/') !== false) { - $parts = explode('/', $branch); - $branch = end($parts); - } - - return $branch; - } - - private function encodeBranchName($branch) { - return str_replace('/', ':', $branch); - } - } diff --git a/src/applications/diffusion/request/mercurial/DiffusionMercurialRequest.php b/src/applications/diffusion/request/mercurial/DiffusionMercurialRequest.php index 77e9334bf2..59f1833007 100644 --- a/src/applications/diffusion/request/mercurial/DiffusionMercurialRequest.php +++ b/src/applications/diffusion/request/mercurial/DiffusionMercurialRequest.php @@ -16,28 +16,17 @@ * limitations under the License. */ -// TODO: This has some minor code duplication vs the Git request that could be -// shared. +/** + * @group diffusion + */ final class DiffusionMercurialRequest extends DiffusionRequest { - protected function initializeFromAphrontRequestDictionary(array $data) { - parent::initializeFromAphrontRequestDictionary($data); + protected function getSupportsBranches() { + return true; + } - $path = $this->path; - $parts = explode('/', $path); - - $branch = array_shift($parts); - if ($branch != ':') { - $this->branch = $this->decodeBranchName($branch); - } - - foreach ($parts as $key => $part) { - if ($part == '..') { - unset($parts[$key]); - } - } - - $this->path = implode('/', $parts); + protected function didInitialize() { + return; } public function getBranch() { @@ -50,11 +39,6 @@ final class DiffusionMercurialRequest extends DiffusionRequest { throw new Exception("Unable to determine branch!"); } - public function getUriPath() { - return '/diffusion/'.$this->getCallsign().'/browse/'. - $this->getBranchURIComponent($this->branch).$this->path; - } - public function getCommit() { if ($this->commit) { return $this->commit; @@ -66,16 +50,4 @@ final class DiffusionMercurialRequest extends DiffusionRequest { return substr($this->stableCommitName, 0, 16); } - public function getBranchURIComponent($branch) { - return $this->encodeBranchName($branch).'/'; - } - - private function decodeBranchName($branch) { - return str_replace(':', '/', $branch); - } - - private function encodeBranchName($branch) { - return str_replace('/', ':', $branch); - } - } diff --git a/src/applications/diffusion/request/svn/DiffusionSvnRequest.php b/src/applications/diffusion/request/svn/DiffusionSvnRequest.php index eb1c19c210..315c2f1a0b 100644 --- a/src/applications/diffusion/request/svn/DiffusionSvnRequest.php +++ b/src/applications/diffusion/request/svn/DiffusionSvnRequest.php @@ -16,22 +16,22 @@ * limitations under the License. */ +/** + * @group diffusion + */ final class DiffusionSvnRequest extends DiffusionRequest { - protected function initializeFromAphrontRequestDictionary(array $data) { - parent::initializeFromAphrontRequestDictionary($data); + protected function getSupportsBranches() { + return false; + } + protected function didInitialize() { if ($this->path === null) { $subpath = $this->repository->getDetail('svn-subpath'); if ($subpath) { $this->path = $subpath; } } - - if (!strncmp($this->path, ':', 1)) { - $this->path = substr($this->path, 1); - $this->path = ltrim($this->path, '/'); - } } public function getStableCommitName() { diff --git a/src/applications/diffusion/view/base/DiffusionView.php b/src/applications/diffusion/view/base/DiffusionView.php index 53973f0389..223844c563 100644 --- a/src/applications/diffusion/view/base/DiffusionView.php +++ b/src/applications/diffusion/view/base/DiffusionView.php @@ -41,70 +41,43 @@ abstract class DiffusionView extends AphrontView { return $text; } - $drequest = $this->getDiffusionRequest(); - - if ($commit_identifier) { - $commit = ';'.$commit_identifier; - } else if ($drequest->getRawCommit()) { - $commit = ';'.$drequest->getCommitURIComponent($drequest->getRawCommit()); - } else { - $commit = null; - } - - $repository = $drequest->getRepository(); - $callsign = $repository->getCallsign(); - - $branch = $drequest->getBranchURIComponent($drequest->getBranch()); - $path = $branch.($path ? $path : $drequest->getPath()); + $href = $this->getDiffusionRequest()->generateURI( + array( + 'action' => 'change', + 'path' => $path, + 'commit' => $commit_identifier, + )); return phutil_render_tag( 'a', array( - 'href' => "/diffusion/{$callsign}/change/{$path}{$commit}", + 'href' => $href, ), $text); } final public function linkHistory($path) { - $drequest = $this->getDiffusionRequest(); - - if ($drequest->getRawCommit()) { - $commit = ';'.$drequest->getCommitURIComponent($drequest->getRawCommit()); - } else { - $commit = null; - } - - $repository = $drequest->getRepository(); - $callsign = $repository->getCallsign(); - - $branch = $drequest->getBranchURIComponent($drequest->getBranch()); - $path = $branch.$path; - - $text = 'History'; + $href = $this->getDiffusionRequest()->generateURI( + array( + 'action' => 'history', + 'path' => $path, + )); return phutil_render_tag( 'a', array( - 'href' => "/diffusion/{$callsign}/history/{$path}{$commit}", + 'href' => $href, ), - $text); + 'History'); } final public function linkBrowse($path, array $details = array()) { - $drequest = $this->getDiffusionRequest(); - $raw_commit = idx($details, 'commit', $drequest->getRawCommit()); - if ($raw_commit) { - $commit = ';'.$drequest->getCommitURIComponent($raw_commit); - } else { - $commit = null; - } - - $repository = $drequest->getRepository(); - $callsign = $repository->getCallsign(); - - $branch = $drequest->getBranchURIComponent($drequest->getBranch()); - $path = $branch.$path; + $href = $this->getDiffusionRequest()->generateURI( + array( + 'action' => 'browse', + 'path' => $path, + )); if (isset($details['text'])) { $text = phutil_escape_html($details['text']); @@ -115,7 +88,7 @@ abstract class DiffusionView extends AphrontView { return phutil_render_tag( 'a', array( - 'href' => "/diffusion/{$callsign}/browse/{$path}{$commit}", + 'href' => $href, ), $text); } diff --git a/src/applications/diffusion/view/base/__init__.php b/src/applications/diffusion/view/base/__init__.php index 8db5411a0f..0e55875327 100644 --- a/src/applications/diffusion/view/base/__init__.php +++ b/src/applications/diffusion/view/base/__init__.php @@ -11,7 +11,6 @@ phutil_require_module('phabricator', 'applications/repository/constants/reposito phutil_require_module('phabricator', 'view/base'); phutil_require_module('phutil', 'markup'); -phutil_require_module('phutil', 'utils'); phutil_require_source('DiffusionView.php'); diff --git a/src/applications/diffusion/view/branchtable/DiffusionBranchTableView.php b/src/applications/diffusion/view/branchtable/DiffusionBranchTableView.php index c178454253..c998e0b089 100644 --- a/src/applications/diffusion/view/branchtable/DiffusionBranchTableView.php +++ b/src/applications/diffusion/view/branchtable/DiffusionBranchTableView.php @@ -1,7 +1,7 @@ getDiffusionRequest(); $current_branch = $drequest->getBranch(); - $callsign = $drequest->getRepository()->getCallsign(); - $rows = array(); $rowc = array(); foreach ($this->branches as $branch) { - $branch_uri = $drequest->getBranchURIComponent($branch->getName()); - $rows[] = array( phutil_render_tag( 'a', array( - 'href' => "/diffusion/{$callsign}/repository/{$branch_uri}", + 'href' => $drequest->generateURI( + array( + 'action' => 'branch', + 'branch' => $branch->getName(), + )), ), phutil_escape_html($branch->getName())), self::linkCommit( diff --git a/src/applications/diffusion/view/browsetable/DiffusionBrowseTableView.php b/src/applications/diffusion/view/browsetable/DiffusionBrowseTableView.php index 84bddf952b..1ec8f279f5 100644 --- a/src/applications/diffusion/view/browsetable/DiffusionBrowseTableView.php +++ b/src/applications/diffusion/view/browsetable/DiffusionBrowseTableView.php @@ -1,7 +1,7 @@ celerity_generate_unique_node_id(), 'details' => celerity_generate_unique_node_id(), ); - $uri = - '/diffusion/'.$repository->getCallsign().'/lastmodified/'. - $request->getBranchURIComponent($request->getBranch()). - $base_path.$path->getPath(); - if ($request->getRawCommit()) { - $uri .= ';'.$request->getRawCommit(); - } + + $uri = (string)$request->generateURI( + array( + 'action' => 'lastmodified', + 'path' => $base_path.$path->getPath(), + )); + $need_pull[$uri] = $dict; foreach ($dict as $k => $uniq) { $dict[$k] = ''; diff --git a/src/applications/owners/query/path/PhabricatorOwnerPathQuery.php b/src/applications/owners/query/path/PhabricatorOwnerPathQuery.php index 7551f800c6..7bcf74c612 100644 --- a/src/applications/owners/query/path/PhabricatorOwnerPathQuery.php +++ b/src/applications/owners/query/path/PhabricatorOwnerPathQuery.php @@ -22,7 +22,12 @@ final class PhabricatorOwnerPathQuery { PhabricatorRepository $repository, PhabricatorRepositoryCommit $commit) { - $drequest = self::buildDiffusionRequest($repository, $commit); + $drequest = DiffusionRequest::newFromAphrontRequestDictionary( + array( + 'repository' => $repository, + 'commit' => $commit->getCommitIdentifier(), + )); + $path_query = DiffusionPathChangeQuery::newFromDiffusionRequest( $drequest); $paths = $path_query->loadChanges(); @@ -38,15 +43,4 @@ final class PhabricatorOwnerPathQuery { return $result; } - private static function buildDiffusionRequest( - PhabricatorRepository $repository, - PhabricatorRepositoryCommit $commit) { - - return DiffusionRequest::newFromAphrontRequestDictionary( - array( - 'callsign' => $repository->getCallsign(), - 'commit' => $commit->getCommitIdentifier(), - )); - } - } diff --git a/src/applications/owners/storage/package/PhabricatorOwnersPackage.php b/src/applications/owners/storage/package/PhabricatorOwnersPackage.php index 17d1941b49..209a367966 100644 --- a/src/applications/owners/storage/package/PhabricatorOwnersPackage.php +++ b/src/applications/owners/storage/package/PhabricatorOwnersPackage.php @@ -188,10 +188,10 @@ final class PhabricatorOwnersPackage extends PhabricatorOwnersDAO { foreach ($paths as $path => $ignored) { $path = ltrim($path, '/'); // build query to validate path - $drequest = DiffusionRequest::newFromAphrontRequestDictionary( + $drequest = DiffusionRequest::newFromDictionary( array( - 'callsign' => $repository->getCallsign(), - 'path' => ':/'.$path, + 'repository' => $repository, + 'path' => $path, )); $query = DiffusionBrowseQuery::newFromDiffusionRequest($drequest); $query->needValidityOnly(true); diff --git a/src/applications/repository/storage/symbol/PhabricatorRepositorySymbol.php b/src/applications/repository/storage/symbol/PhabricatorRepositorySymbol.php index 776810417c..4b5da1b355 100644 --- a/src/applications/repository/storage/symbol/PhabricatorRepositorySymbol.php +++ b/src/applications/repository/storage/symbol/PhabricatorRepositorySymbol.php @@ -45,18 +45,13 @@ final class PhabricatorRepositorySymbol extends PhabricatorRepositoryDAO { } public function getURI() { - $repo = $this->getRepository(); - $file = $this->getPath(); - $line = $this->getLineNumber(); - - $drequest = DiffusionRequest::newFromAphrontRequestDictionary( + return DiffusionRequest::generateDiffusionURI( array( - 'callsign' => $repo->getCallsign(), + 'action' => 'browse', + 'callsign' => $this->getRepository()->getCallsign(), + 'path' => $this->getPath(), + 'line' => $this->getLineNumber(), )); - $branch = $drequest->getBranchURIComponent($drequest->getBranch()); - $file = $branch.ltrim($file, '/'); - - return '/diffusion/'.$repo->getCallsign().'/browse/'.$file.'$'.$line; } public function getPath() {