From 0e8ed0c61684441ceafe2791b9a05970b58babac Mon Sep 17 00:00:00 2001 From: tycho Date: Sun, 18 Oct 2015 04:22:05 -0700 Subject: [PATCH 01/17] Desactivate subtask when logged out. Summary: Fixes T9592. Test Plan: Log out ! Navigates to a task. See the add button grey-ed out ! Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin Maniphest Tasks: T9592 Differential Revision: https://secure.phabricator.com/D14299 --- .../maniphest/controller/ManiphestTaskDetailController.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php index 8619987201..8876a3386f 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php @@ -381,7 +381,6 @@ final class ManiphestTaskDetailController extends ManiphestController { private function buildActionView(ManiphestTask $task) { $viewer = $this->getRequest()->getUser(); - $viewer_phid = $viewer->getPHID(); $id = $task->getID(); $phid = $task->getPHID(); @@ -391,6 +390,8 @@ final class ManiphestTaskDetailController extends ManiphestController { $task, PhabricatorPolicyCapability::CAN_EDIT); + $can_create = $viewer->isLoggedIn(); + $view = id(new PhabricatorActionListView()) ->setUser($viewer) ->setObject($task) @@ -417,7 +418,9 @@ final class ManiphestTaskDetailController extends ManiphestController { id(new PhabricatorActionView()) ->setName(pht('Create Subtask')) ->setHref($this->getApplicationURI("/task/create/?parent={$id}")) - ->setIcon('fa-level-down')); + ->setIcon('fa-level-down')) + ->setDisabled(!$can_create) + ->setWorkflow(!$can_create); $view->addAction( id(new PhabricatorActionView()) From 478249147006292292cc4b503b7ce764841dc314 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Sun, 18 Oct 2015 14:43:29 -0700 Subject: [PATCH 02/17] Fix fatal in Maniphest Summary: Fixes T9596. Was unable to navigate to a task in Maniphest. Test Plan: navigate to that task. Reviewers: #blessed_reviewers, epriestley, avivey, tycho.tatitscheff Reviewed By: avivey, tycho.tatitscheff Subscribers: tycho.tatitscheff, avivey, Korvin Maniphest Tasks: T9596 Differential Revision: https://secure.phabricator.com/D14300 --- .../maniphest/controller/ManiphestTaskDetailController.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php index 8876a3386f..090bdb9bb7 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php @@ -418,9 +418,9 @@ final class ManiphestTaskDetailController extends ManiphestController { id(new PhabricatorActionView()) ->setName(pht('Create Subtask')) ->setHref($this->getApplicationURI("/task/create/?parent={$id}")) - ->setIcon('fa-level-down')) + ->setIcon('fa-level-down') ->setDisabled(!$can_create) - ->setWorkflow(!$can_create); + ->setWorkflow(!$can_create)); $view->addAction( id(new PhabricatorActionView()) From a8e9da4a5624ecc2b0147376f5e10f17ca241f9a Mon Sep 17 00:00:00 2001 From: Chad Little Date: Sun, 18 Oct 2015 16:07:07 -0700 Subject: [PATCH 03/17] Update Conduit for handleRequest Summary: Ref T8628. Updates Conduit for handleRequest Test Plan: Use Conduit, test list, method calls, try a query, post this diff. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T8628 Differential Revision: https://secure.phabricator.com/D14265 --- .../PhabricatorConduitAPIController.php | 20 ++++++------------- .../PhabricatorConduitListController.php | 10 ++-------- .../PhabricatorConduitLogController.php | 5 ++--- .../PhabricatorConduitTokenController.php | 12 +++++------ .../PhabricatorConduitTokenEditController.php | 2 +- ...ricatorConduitTokenTerminateController.php | 2 +- 6 files changed, 18 insertions(+), 33 deletions(-) diff --git a/src/applications/conduit/controller/PhabricatorConduitAPIController.php b/src/applications/conduit/controller/PhabricatorConduitAPIController.php index 4bbe167c39..367caf394b 100644 --- a/src/applications/conduit/controller/PhabricatorConduitAPIController.php +++ b/src/applications/conduit/controller/PhabricatorConduitAPIController.php @@ -7,18 +7,9 @@ final class PhabricatorConduitAPIController return false; } - private $method; - - public function willProcessRequest(array $data) { - $this->method = $data['method']; - return $this; - } - - public function processRequest() { + public function handleRequest(AphrontRequest $request) { + $method = $request->getURIData('method'); $time_start = microtime(true); - $request = $this->getRequest(); - - $method = $this->method; $api_request = null; $method_implementation = null; @@ -55,7 +46,7 @@ final class PhabricatorConduitAPIController $conduit_username = '-'; if ($call->shouldRequireAuthentication()) { $metadata['scope'] = $call->getRequiredScope(); - $auth_error = $this->authenticateUser($api_request, $metadata); + $auth_error = $this->authenticateUser($api_request, $metadata, $method); // If we've explicitly authenticated the user here and either done // CSRF validation or are using a non-web authentication mechanism. $allow_unguarded_writes = true; @@ -169,7 +160,8 @@ final class PhabricatorConduitAPIController */ private function authenticateUser( ConduitAPIRequest $api_request, - array $metadata) { + array $metadata, + $method) { $request = $this->getRequest(); @@ -207,7 +199,7 @@ final class PhabricatorConduitAPIController unset($protocol_data['scope']); ConduitClient::verifySignature( - $this->method, + $method, $api_request->getAllParameters(), $protocol_data, $ssl_public_key); diff --git a/src/applications/conduit/controller/PhabricatorConduitListController.php b/src/applications/conduit/controller/PhabricatorConduitListController.php index 17ea5a970a..76650f22c9 100644 --- a/src/applications/conduit/controller/PhabricatorConduitListController.php +++ b/src/applications/conduit/controller/PhabricatorConduitListController.php @@ -3,19 +3,13 @@ final class PhabricatorConduitListController extends PhabricatorConduitController { - private $queryKey; - public function shouldAllowPublic() { return true; } - public function willProcessRequest(array $data) { - $this->queryKey = idx($data, 'queryKey'); - } - - public function processRequest() { + public function handleRequest(AphrontRequest $request) { $controller = id(new PhabricatorApplicationSearchController()) - ->setQueryKey($this->queryKey) + ->setQueryKey($request->getURIData('queryKey')) ->setSearchEngine(new PhabricatorConduitSearchEngine()) ->setNavigation($this->buildSideNavView()); return $this->delegateToController($controller); diff --git a/src/applications/conduit/controller/PhabricatorConduitLogController.php b/src/applications/conduit/controller/PhabricatorConduitLogController.php index 0c408982f8..c40a9f3ffe 100644 --- a/src/applications/conduit/controller/PhabricatorConduitLogController.php +++ b/src/applications/conduit/controller/PhabricatorConduitLogController.php @@ -3,9 +3,8 @@ final class PhabricatorConduitLogController extends PhabricatorConduitController { - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $request->getViewer(); $conn_table = new PhabricatorConduitConnectionLog(); $call_table = new PhabricatorConduitMethodCallLog(); diff --git a/src/applications/conduit/controller/PhabricatorConduitTokenController.php b/src/applications/conduit/controller/PhabricatorConduitTokenController.php index c0d230f1bb..b5501a2805 100644 --- a/src/applications/conduit/controller/PhabricatorConduitTokenController.php +++ b/src/applications/conduit/controller/PhabricatorConduitTokenController.php @@ -3,11 +3,11 @@ final class PhabricatorConduitTokenController extends PhabricatorConduitController { - public function processRequest() { - $user = $this->getRequest()->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $request->getViewer(); id(new PhabricatorAuthSessionEngine())->requireHighSecuritySession( - $user, + $viewer, $this->getRequest(), '/'); @@ -19,13 +19,13 @@ final class PhabricatorConduitTokenController $old_token = id(new PhabricatorConduitCertificateToken()) ->loadOneWhere( 'userPHID = %s', - $user->getPHID()); + $viewer->getPHID()); if ($old_token) { $old_token->delete(); } $token = id(new PhabricatorConduitCertificateToken()) - ->setUserPHID($user->getPHID()) + ->setUserPHID($viewer->getPHID()) ->setToken(Filesystem::readRandomCharacters(40)) ->save(); @@ -42,7 +42,7 @@ final class PhabricatorConduitTokenController Javelin::initBehavior('select-on-click'); $form = id(new AphrontFormView()) - ->setUser($user) + ->setUser($viewer) ->appendRemarkupInstructions($pre_instructions) ->appendChild( id(new AphrontFormTextAreaControl()) diff --git a/src/applications/conduit/controller/PhabricatorConduitTokenEditController.php b/src/applications/conduit/controller/PhabricatorConduitTokenEditController.php index ff06173f50..503456d010 100644 --- a/src/applications/conduit/controller/PhabricatorConduitTokenEditController.php +++ b/src/applications/conduit/controller/PhabricatorConduitTokenEditController.php @@ -5,8 +5,8 @@ final class PhabricatorConduitTokenEditController public function handleRequest(AphrontRequest $request) { $viewer = $request->getViewer(); - $id = $request->getURIData('id'); + if ($id) { $token = id(new PhabricatorConduitTokenQuery()) ->setViewer($viewer) diff --git a/src/applications/conduit/controller/PhabricatorConduitTokenTerminateController.php b/src/applications/conduit/controller/PhabricatorConduitTokenTerminateController.php index f41b417c87..466089ebeb 100644 --- a/src/applications/conduit/controller/PhabricatorConduitTokenTerminateController.php +++ b/src/applications/conduit/controller/PhabricatorConduitTokenTerminateController.php @@ -5,9 +5,9 @@ final class PhabricatorConduitTokenTerminateController public function handleRequest(AphrontRequest $request) { $viewer = $request->getViewer(); - $object_phid = $request->getStr('objectPHID'); $id = $request->getURIData('id'); + if ($id) { $token = id(new PhabricatorConduitTokenQuery()) ->setViewer($viewer) From 057d62d570d4474745b8074132bece9cf563eb73 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Sun, 18 Oct 2015 16:08:20 -0700 Subject: [PATCH 04/17] Update Phlux for handleRequest Summary: Ref T8628. Updates Phlux Test Plan: New var, list vars, edit vars Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T8628 Differential Revision: https://secure.phabricator.com/D14267 --- .../phlux/controller/PhluxEditController.php | 28 ++++++++----------- .../phlux/controller/PhluxListController.php | 9 +++--- .../phlux/controller/PhluxViewController.php | 24 ++++++---------- 3 files changed, 24 insertions(+), 37 deletions(-) diff --git a/src/applications/phlux/controller/PhluxEditController.php b/src/applications/phlux/controller/PhluxEditController.php index ac64ffcaa2..775737c9b7 100644 --- a/src/applications/phlux/controller/PhluxEditController.php +++ b/src/applications/phlux/controller/PhluxEditController.php @@ -2,35 +2,29 @@ final class PhluxEditController extends PhluxController { - private $key; + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); + $key = $request->getURIData('key'); - public function willProcessRequest(array $data) { - $this->key = idx($data, 'key'); - } - - public function processRequest() { - $request = $this->getRequest(); - $user = $request->getUser(); - - $is_new = ($this->key === null); + $is_new = ($key === null); if ($is_new) { $var = new PhluxVariable(); $var->setViewPolicy(PhabricatorPolicies::POLICY_USER); $var->setEditPolicy(PhabricatorPolicies::POLICY_USER); } else { $var = id(new PhluxVariableQuery()) - ->setViewer($user) + ->setViewer($viewer) ->requireCapabilities( array( PhabricatorPolicyCapability::CAN_VIEW, PhabricatorPolicyCapability::CAN_EDIT, )) - ->withKeys(array($this->key)) + ->withKeys(array($key)) ->executeOne(); if (!$var) { return new Aphront404Response(); } - $view_uri = $this->getApplicationURI('/view/'.$this->key.'/'); + $view_uri = $this->getApplicationURI('/view/'.$key.'/'); } $e_key = ($is_new ? true : null); @@ -67,7 +61,7 @@ final class PhluxEditController extends PhluxController { if (!$errors) { $editor = id(new PhluxVariableEditor()) - ->setActor($user) + ->setActor($viewer) ->setContinueOnNoEffect(true) ->setContentSourceFromRequest($request); @@ -110,12 +104,12 @@ final class PhluxEditController extends PhluxController { } $policies = id(new PhabricatorPolicyQuery()) - ->setViewer($user) + ->setViewer($viewer) ->setObject($var) ->execute(); $form = id(new AphrontFormView()) - ->setUser($user) + ->setUser($viewer) ->appendChild( id(new AphrontFormTextControl()) ->setValue($var->getVariableKey()) @@ -161,7 +155,7 @@ final class PhluxEditController extends PhluxController { $title = pht('Create Variable'); $crumbs->addTextCrumb($title, $request->getRequestURI()); } else { - $title = pht('Edit %s', $this->key); + $title = pht('Edit %s', $key); $crumbs->addTextCrumb($title, $request->getRequestURI()); } diff --git a/src/applications/phlux/controller/PhluxListController.php b/src/applications/phlux/controller/PhluxListController.php index 69521ea62f..550f571a6a 100644 --- a/src/applications/phlux/controller/PhluxListController.php +++ b/src/applications/phlux/controller/PhluxListController.php @@ -2,14 +2,13 @@ final class PhluxListController extends PhluxController { - public function processRequest() { - $request = $this->getRequest(); - $user = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); $pager = new AphrontCursorPagerView(); $pager->readFromRequest($request); $query = id(new PhluxVariableQuery()) - ->setViewer($user); + ->setViewer($viewer); $vars = $query->executeWithCursorPager($pager); @@ -22,7 +21,7 @@ final class PhluxListController extends PhluxController { $item->setHref($this->getApplicationURI('/view/'.$key.'/')); $item->addIcon( 'none', - phabricator_datetime($var->getDateModified(), $user)); + phabricator_datetime($var->getDateModified(), $viewer)); $view->addItem($item); } diff --git a/src/applications/phlux/controller/PhluxViewController.php b/src/applications/phlux/controller/PhluxViewController.php index be19e7bd28..f45154a158 100644 --- a/src/applications/phlux/controller/PhluxViewController.php +++ b/src/applications/phlux/controller/PhluxViewController.php @@ -2,19 +2,13 @@ final class PhluxViewController extends PhluxController { - private $key; - - public function willProcessRequest(array $data) { - $this->key = $data['key']; - } - - public function processRequest() { - $request = $this->getRequest(); - $user = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); + $key = $request->getURIData('key'); $var = id(new PhluxVariableQuery()) - ->setViewer($user) - ->withKeys(array($this->key)) + ->setViewer($viewer) + ->withKeys(array($key)) ->executeOne(); if (!$var) { @@ -29,16 +23,16 @@ final class PhluxViewController extends PhluxController { $header = id(new PHUIHeaderView()) ->setHeader($title) - ->setUser($user) + ->setUser($viewer) ->setPolicyObject($var); $actions = id(new PhabricatorActionListView()) - ->setUser($user) + ->setUser($viewer) ->setObjectURI($request->getRequestURI()) ->setObject($var); $can_edit = PhabricatorPolicyFilter::hasCapability( - $user, + $viewer, $var, PhabricatorPolicyCapability::CAN_EDIT); @@ -53,7 +47,7 @@ final class PhluxViewController extends PhluxController { $display_value = json_encode($var->getVariableValue()); $properties = id(new PHUIPropertyListView()) - ->setUser($user) + ->setUser($viewer) ->setObject($var) ->setActionList($actions) ->addProperty(pht('Value'), $display_value); From d784bf1ea8139cabe8aba31263d4961ed5bb41f6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 19 Oct 2015 11:14:46 -0700 Subject: [PATCH 05/17] Make disk-based setup caches more correct (but slower) Summary: Fixes T9599. When APC/APCu are not available, we fall back to a disk-based cache. We try to share this cache across webserver processes like APC/APCu would be shared in order to improve performance, but are just kind of guessing how to coordinate it. From T9599, it sounds like we don't always get this right in every configuration. Since this is complicated and error prone, just stop trying to do this. This cache has bad performance anyway (no production install should be using it), and we have much better APC/APCu setup instructions now than we did when I wrote this. Just using the PID is simpler and more correct. Test Plan: - Artificially disabled APC. - Reloaded the page, saw all the setup stuff run. - Reloaded the page, saw no setup stuff run (i.e., cache was hit). - Restarted the webserver. - Reloaded the page, saw all the setup stuff run. - Reloaded again, got a cache hit. I don't really know how to reproduce the exact problem with the parent PID not working, but from T9599 it sounds like this fixed the issue and from my test plan we still appear to get correct behavior in the standard/common case. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9599 Differential Revision: https://secure.phabricator.com/D14302 --- src/applications/cache/PhabricatorCaches.php | 36 +++---------------- .../cache/spec/PhabricatorDataCacheSpec.php | 4 ++- 2 files changed, 7 insertions(+), 33 deletions(-) diff --git a/src/applications/cache/PhabricatorCaches.php b/src/applications/cache/PhabricatorCaches.php index 5824c6458c..1127a7312b 100644 --- a/src/applications/cache/PhabricatorCaches.php +++ b/src/applications/cache/PhabricatorCaches.php @@ -206,40 +206,12 @@ final class PhabricatorCaches extends Phobject { // otherwise (we desire this property to give the cache the best hit rate // we can). - // In some setups, the parent PID is more stable and longer-lived that the - // PID (e.g., under apache, our PID will be a worker while the ppid will - // be the main httpd process). If we're confident we're running under such - // a setup, we can try to use the PPID as the basis for our cache instead - // of our own PID. - $use_ppid = false; - - switch (php_sapi_name()) { - case 'cli-server': - // This is the PHP5.4+ built-in webserver. We should use the pid - // (the server), not the ppid (probably a shell or something). - $use_ppid = false; - break; - case 'fpm-fcgi': - // We should be safe to use PPID here. - $use_ppid = true; - break; - case 'apache2handler': - // We're definitely safe to use the PPID. - $use_ppid = true; - break; - } + // Unfortunately, we don't have a very good strategy for minimizing the + // churn rate of the cache. We previously tried to use the parent process + // PID in some cases, but this was not reliable. See T9599 for one case of + // this. $pid_basis = getmypid(); - if ($use_ppid) { - if (function_exists('posix_getppid')) { - $parent_pid = posix_getppid(); - // On most systems, normal processes can never have PIDs lower than 100, - // so something likely went wrong if we we get one of these. - if ($parent_pid > 100) { - $pid_basis = $parent_pid; - } - } - } // If possible, we also want to know when the process launched, so we can // drop the cache if a process restarts but gets the same PID an earlier diff --git a/src/applications/cache/spec/PhabricatorDataCacheSpec.php b/src/applications/cache/spec/PhabricatorDataCacheSpec.php index f5fe1c6277..eae5d75790 100644 --- a/src/applications/cache/spec/PhabricatorDataCacheSpec.php +++ b/src/applications/cache/spec/PhabricatorDataCacheSpec.php @@ -63,7 +63,9 @@ final class PhabricatorDataCacheSpec extends PhabricatorCacheSpec { private function initNoneSpec() { if (version_compare(phpversion(), '5.5', '>=')) { $message = pht( - 'Installing the "APCu" PHP extension will improve performance.'); + 'Installing the "APCu" PHP extension will improve performance. '. + 'This extension is strongly recommended. Without it, Phabricator '. + 'must rely on a very inefficient disk-based cache.'); $this ->newIssue('extension.apcu') From 267e718dfe2f0c62b49328f11908f8fe0e174717 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Mon, 19 Oct 2015 12:12:52 -0700 Subject: [PATCH 06/17] Don't allow logged out users to initialize a Workboard Summary: Right now logged out users can enable a workboard on a project. Test Plan: Log out, view a public project, click on Workboard, get not set up dialog. Click Cancel, return to project details. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D14304 --- .../PhabricatorProjectBoardViewController.php | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index ffe3ea2e73..332736998e 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -67,6 +67,9 @@ final class PhabricatorProjectBoardViewController // TODO: Expand the checks here if we add the ability // to hide the Backlog column if (!$columns) { + if (!$viewer->isLoggedIn()) { + return $this->noAccessDialog($project); + } switch ($request->getStr('initialize-type')) { case 'backlog-only': $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); @@ -713,6 +716,20 @@ final class PhabricatorProjectBoardViewController ->setDialog($dialog); } + private function noAccessDialog(PhabricatorProject $project) { + + $instructions = pht('This workboard has not been setup yet.'); + + $dialog = id(new AphrontDialogView()) + ->setUser($this->getRequest()->getUser()) + ->setTitle(pht('No Workboard')) + ->addCancelButton($this->getApplicationURI('view/'.$project->getID().'/')) + ->appendParagraph($instructions); + + return id(new AphrontDialogResponse()) + ->setDialog($dialog); + } + /** * Add current state parameters (like order and the visibility of hidden From ec485de8f977898d165decbb70caff7f02141a83 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Mon, 19 Oct 2015 13:22:13 -0700 Subject: [PATCH 07/17] Restrict Workboard initialization to CAN_EDIT Summary: Make Workboard initialization more restrictive. Test Plan: Log out, see "No Workboard", Log in with permissions, see "New Workboard", Log in with notchad, see "No Workboard". Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T7410 Differential Revision: https://secure.phabricator.com/D14306 --- .../controller/PhabricatorProjectBoardViewController.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index 332736998e..12699c8cbc 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -67,7 +67,11 @@ final class PhabricatorProjectBoardViewController // TODO: Expand the checks here if we add the ability // to hide the Backlog column if (!$columns) { - if (!$viewer->isLoggedIn()) { + $can_edit = PhabricatorPolicyFilter::hasCapability( + $viewer, + $project, + PhabricatorPolicyCapability::CAN_EDIT); + if (!$can_edit) { return $this->noAccessDialog($project); } switch ($request->getStr('initialize-type')) { From fbd365d57114d2edd410b09ac42fe1343e9a6bd6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 19 Oct 2015 13:27:47 -0700 Subject: [PATCH 08/17] Remove scattered links to "Support" document Summary: I'm going to do some version of D13941. Clean up extra links to the old document first. These were just randomly links from various places that we no longer really want feedback on and/or are now better covered by other documents. Test Plan: - `grep` - Reviewed Config/Welcome screen. - Reviewed `uri.allowed-editor-protocols`. Reviewers: chad Reviewed By: chad Differential Revision: https://secure.phabricator.com/D14303 --- .../PhabricatorConfigWelcomeController.php | 14 ++------------ .../option/PhabricatorSecurityConfigOptions.php | 9 ++------- src/docs/user/userguide/arcanist.diviner | 5 ----- src/docs/user/userguide/audit.diviner | 6 +----- src/docs/user/userguide/differential.diviner | 3 +-- src/docs/user/userguide/phame.diviner | 8 ++------ src/docs/user/userguide/phriction.diviner | 6 ++---- 7 files changed, 10 insertions(+), 41 deletions(-) diff --git a/src/applications/config/controller/PhabricatorConfigWelcomeController.php b/src/applications/config/controller/PhabricatorConfigWelcomeController.php index 37442c2375..a3ef29bbc8 100644 --- a/src/applications/config/controller/PhabricatorConfigWelcomeController.php +++ b/src/applications/config/controller/PhabricatorConfigWelcomeController.php @@ -225,18 +225,8 @@ final class PhabricatorConfigWelcomeController 'fa-globe', $content); - $support_href = PhabricatorEnv::getDoclink('Give Feedback! Get Support!'); - $content = pht( - "=== Need Help with Setup? ===\n\n". - 'Having trouble getting something set up? See '. - '**[[ %s | Give Feedback! Get Support! ]]** for ways to get in touch '. - 'to get answers to questions, report bugs, and request features.', - $support_href); - - $explore[] = $this->newItem( - $request, - 'fa-life-ring', - $content); + // TODO: Restore some sort of "Support" link here, but just nuke it for + // now as we figure stuff out. $differential_uri = PhabricatorEnv::getURI('/differential/'); $differential_create_uri = PhabricatorEnv::getURI( diff --git a/src/applications/config/option/PhabricatorSecurityConfigOptions.php b/src/applications/config/option/PhabricatorSecurityConfigOptions.php index b8ff3ccc48..8baea2e664 100644 --- a/src/applications/config/option/PhabricatorSecurityConfigOptions.php +++ b/src/applications/config/option/PhabricatorSecurityConfigOptions.php @@ -20,8 +20,6 @@ final class PhabricatorSecurityConfigOptions } public function getOptions() { - $support_href = PhabricatorEnv::getDoclink('Give Feedback! Get Support!'); - $doc_href = PhabricatorEnv::getDoclink('Configuring a File Domain'); $doc_name = pht('Configuration Guide: Configuring a File Domain'); @@ -197,11 +195,8 @@ final class PhabricatorSecurityConfigOptions ->setSummary(pht('Whitelists editor protocols for "Open in Editor".')) ->setDescription( pht( - "Users can configure a URI pattern to open files in a text ". - "editor. The URI must use a protocol on this whitelist.\n\n". - "(If you use an editor which defines a protocol not on this ". - "list, [[ %s | let us know ]] and we'll update the defaults.)", - $support_href)) + 'Users can configure a URI pattern to open files in a text '. + 'editor. The URI must use a protocol on this whitelist.')) ->setLocked(true), $this->newOption( 'celerity.resource-hash', diff --git a/src/docs/user/userguide/arcanist.diviner b/src/docs/user/userguide/arcanist.diviner index 41613db968..f1c4545956 100644 --- a/src/docs/user/userguide/arcanist.diviner +++ b/src/docs/user/userguide/arcanist.diviner @@ -129,11 +129,6 @@ able to use: - Another common approach is to write an install script as an action into existing build scripts, so users can run `make install-arc` or `ant install-arc` or similar. - - In general, if this sucks and is causing you pain, let us know (see - @{article:Give Feedback! Get Support!}). We're planning to improve this at - some point, but it's somewhat complicated to get right. While it can take a - little time to set up, we aren't getting feedback that it's a persistent - pain point, so it hasn't been a priority. == Installing Tab Completion == diff --git a/src/docs/user/userguide/audit.diviner b/src/docs/user/userguide/audit.diviner index cff9b73e1b..9c2f752b08 100644 --- a/src/docs/user/userguide/audit.diviner +++ b/src/docs/user/userguide/audit.diviner @@ -12,9 +12,6 @@ Phabricator supports two code review workflows, "review" (pre-push) and This document summarizes the post-push "audit" workflow implemented by the creatively-named //Audit// tool. -NOTE: The audit workflow is new, give us feedback about it! See -@{article:Give Feedback! Get Support!}. - = How Audit Works = Using auditing allows you to push and deploy code without waiting for code @@ -104,5 +101,4 @@ only changes that are relevant to them. = Next Steps = - - Learn more about Herald at @{article:Herald User Guide}; or - - give us feedback at @{article:Give Feedback! Get Support!}. + - Learn more about Herald at @{article:Herald User Guide}. diff --git a/src/docs/user/userguide/differential.diviner b/src/docs/user/userguide/differential.diviner index 92f93a3015..edf992f720 100644 --- a/src/docs/user/userguide/differential.diviner +++ b/src/docs/user/userguide/differential.diviner @@ -65,5 +65,4 @@ files affected, etc.) - learn about handling large changesets at @{article:Differential User Guide: Large Changes}; or - learn about test plans at @{article:Differential User Guide: Test Plans}; or - - learn more about Herald at @{article:Herald User Guide}; or - - give us feedback at @{article:Give Feedback! Get Support!}. + - learn more about Herald at @{article:Herald User Guide}. diff --git a/src/docs/user/userguide/phame.diviner b/src/docs/user/userguide/phame.diviner index 83cd91f2cf..0de225c49f 100644 --- a/src/docs/user/userguide/phame.diviner +++ b/src/docs/user/userguide/phame.diviner @@ -5,6 +5,8 @@ Journal about your thoughts and feelings. Share with others. Profit. = Overview = +IMPORTANT: Phame is a prototype application. + Phame is a simple blogging platform. You can write drafts which only you can see. Later, you can publish these drafts as posts which anyone who can access the Phabricator instance can see. You can also add posts to blogs to increase @@ -55,9 +57,3 @@ this functionality. A given comment widget is tied 1:1 with a given post. This means the same instance of a given comment widget will appear for a given post regardless of whether that post is being viewed in the context of a blog. - -= Next Steps = - - - Phame is extremely new and very basic for now. Give us feedback on - what you'd like to see improve! See @{article:Give Feedback! Get - Support!}. diff --git a/src/docs/user/userguide/phriction.diviner b/src/docs/user/userguide/phriction.diviner index f3eaa4dffb..60ad6357fe 100644 --- a/src/docs/user/userguide/phriction.diviner +++ b/src/docs/user/userguide/phriction.diviner @@ -3,10 +3,8 @@ Construct a detailed written history of your civilization. -= Overview = +Overview +======== Phriction is a simple wiki. You can edit pages, and the text you write will stay there. Other people can see it later. - -NOTE: Phriction is extremely new and very basic for now. Give us feedback on -what you'd like to see improve! See @{article:Give Feedback! Get Support!}. From 4cb2ec11202e2ffb71b5255bd36bafb27c627262 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 19 Oct 2015 13:29:24 -0700 Subject: [PATCH 09/17] Update support documentation for modern times Summary: Basically similar to D13941 but a little more extreme: - Really strongly emphasize reproducibility for bug reports, and set users up for rejection if they don't satisfy this. - Really strongly emphasize problem descriptions for feature requests, and set users up for rejection. - Get rid of various "please give us feedback"; we get plenty of feedback these days. - Some modernization tweaks. - Split the support document into: - Stuff we actually support for free (security / good bug reports / feature requests). - Stuff you can pay us for (hosting / consulting / prioritization). - A nebulous "community" section, with appropriate (low) expectations that better reflects reality. My overall goals here are: - Set expectations better, so users don't show up in IRC expecting it to be a "great place to get amazing support" or whatever the docs said in 2011. - Possibly move the needle slightly on bug reports / feature request quality, maybe. Test Plan: Read changes carefully. Reviewers: chad Reviewed By: chad Differential Revision: https://secure.phabricator.com/D14305 --- src/docs/book/user.book | 3 + src/docs/contributor/bug_reports.diviner | 34 +++-- src/docs/contributor/contrib_intro.diviner | 23 ++-- src/docs/contributor/feature_requests.diviner | 29 ++-- src/docs/user/feedback.diviner | 43 +----- src/docs/user/reporting_security.diviner | 27 ++-- src/docs/user/support.diviner | 125 ++++++++++++++++++ 7 files changed, 193 insertions(+), 91 deletions(-) create mode 100644 src/docs/user/support.diviner diff --git a/src/docs/book/user.book b/src/docs/book/user.book index 464ec994a0..2312bf65cd 100644 --- a/src/docs/book/user.book +++ b/src/docs/book/user.book @@ -31,6 +31,9 @@ }, "fieldmanual": { "name": "Field Manuals" + }, + "cellar": { + "name": "Musty Cellar" } } } diff --git a/src/docs/contributor/bug_reports.diviner b/src/docs/contributor/bug_reports.diviner index 74bf0a2fcb..0ad3635d87 100644 --- a/src/docs/contributor/bug_reports.diviner +++ b/src/docs/contributor/bug_reports.diviner @@ -3,6 +3,14 @@ Describes how to file an effective Phabricator bug report. +Include Reproduction Steps! +=========================== + +IMPORTANT: When filing a bug report, you **MUST** include reproduction +instructions. We can not help fix bugs we can not reproduce, and will not +accept reports which omit reproduction instructions. See below for details. + + Overview ======== @@ -28,6 +36,7 @@ If you have a feature request (not a bug report), see For general information on contributing to Phabricator, see @{article:Contributor Introduction}. + Common Fixes ============ @@ -73,6 +82,9 @@ Supported Issues Before filing a bug, make sure you're filing an issue against something we support. +**We can NOT help you with issues we can not reproduce.** It is critical that +you explain how to reproduce the issue when filing a report. + **We do NOT support prototype applications.** If you're running into an issue with a prototype application, you're on your own. For more information about prototype applications, see @{article:User Guide: Prototype Applications}. @@ -101,6 +113,7 @@ Otherwise, if you're having an issue with a supported first-party application and followed the upstream install instructions on a normal computer, we're happy to try to help. + Getting More Information ======================== @@ -121,6 +134,7 @@ help us figure out and resolve an issue. troubleshooting. Adjusting settings or enabling debugging modes may give you more information about the issue. + Reproducibility =============== @@ -135,7 +149,9 @@ trouble narrowing something down or want to check if updating might fix an issue. It is nearly impossible for us to resolve many issues if we can not reproduce -them. +them. We will not accept reports which do not contain the information required +to reproduce problems. + Unreproducible Problems ======================= @@ -161,6 +177,7 @@ We will make a reasonable effort to reproduce problems, but can not help with issues which we can't reproduce. You can make sure we're able to help resolve your issue by generating clear reproduction steps. + Create a Task in Maniphest ========================== @@ -168,11 +185,10 @@ If you're up to date, supported, have collected information about the problem, and have the best reproduction instructions you can come up with, you're ready to file an issue. -We'll look at any issue report we can find (we monitor IRC, email, -StackOverflow, Quora, Facebook and Twitter), but the upstream Maniphest is the -authoritative bug tracker and the best place to file: +It is **particularly critical** that you include reproduction steps. We will +not accept reports which describe issues we can not reproduce. -https://secure.phabricator.com/maniphest/task/create/ +(NOTE) https://secure.phabricator.com/maniphest/task/create/ If you don't want to file there (or, for example, your bug relates to being unable to log in or unable to file an issue in Maniphest) you can file on any @@ -182,12 +198,9 @@ filed against the upstream than if they're filed somewhere else. | Effectiveness | Filing Method | |---|---| | Best | Upstream Maniphest | -| Ehhh | Quora, StackOverflow, Facebook, email, etc. | +| Ehhh | Quora, StackOverflow, Facebook, Jelly, email, etc. | | What | Passive-aggressive tweet | -If you have a quick question or want to discuss something before filing an -issue, IRC is a great way to get a sanity check first. You can find information -about IRC in @{article: Give Feedback! Get Support!}. Next Steps ========== @@ -195,6 +208,5 @@ Next Steps Continue by: - learning about @{article: Contributing Feature Requests}; or - - reading general support information in - @{article: Give Feedback! Get Support!}; or + - reading general support information in @{article:Support Resources}; or - returning to the @{article:Contributor Introduction}. diff --git a/src/docs/contributor/contrib_intro.diviner b/src/docs/contributor/contrib_intro.diviner index d9b1f42ed4..a93c8530dd 100644 --- a/src/docs/contributor/contrib_intro.diviner +++ b/src/docs/contributor/contrib_intro.diviner @@ -23,8 +23,7 @@ contribute to Phabricator: - Send us an email or drop by IRC just to say "thanks". A big part of the reason we build this software is to help people solve problems, and knowing - that our efforts are appreciated is really rewarding. You can find ways to - get in touch in @{article:Give Feedback! Get Support!} + that our efforts are appreciated is really rewarding. - Recommend Phabricator to people who you think might find it useful. Our most powerful growth channel is word of mouth, and mentioning or tweeting about Phabricator helps the project grow. If writing a tweet sounds like @@ -40,7 +39,6 @@ contribute to Phabricator: - Report bugs and request features. We may not always be able to fix or build things right away, but knowing about issues users are encountering or features they'd like to see improves our ability to plan and prioritize. - For ways to do this, see @{article:Give Feedback! Get Support!} - For details on reporting bugs, see @{article:Contributing Bug Reports}. - For details on requesting features, see @{article:Contributing Feature Requests}. @@ -48,16 +46,11 @@ contribute to Phabricator: 6-12 months currently exists on the [[ Roadmap ]] or in Maniphest. Telling us about use cases you have can help us build better products when the time comes to write the code. - - Hang out in IRC, and maybe answer a question or two. IRC is a completely - legit place for serious hackers to hang out anyway, but while you're there - you might see someone ask a question that you know the answer to. Helping - them out (or pointing them to the right documentation) is a big help to us. - You can find details about the IRC channel in - @{article:Give Feedback! Get Support!} -If all of this sounds nice but you really just want to write some code, that's -awesome too. To get started with contributing code, see -@{article:Contributing Code}. +If all of this sounds nice but you really just want to write some code, be +aware that this project often presents a high barrier to entry for new +contributors. To continue, see @{article:Contributing Code}. + Next Steps ========== @@ -65,6 +58,6 @@ Next Steps Continue by: - learning about bug reports in @{article:Contributing Bug Reports}; - - learning about feature requests in @{article:Contributing Feature Requests}; - - learning about code contributions in @{article:Contributing Code}; or - - getting in touch with @{article:Give Feedback! Get Support!} + - learning about feature requests in + @{article:Contributing Feature Requests}; or + - learning about code contributions in @{article:Contributing Code}. diff --git a/src/docs/contributor/feature_requests.diviner b/src/docs/contributor/feature_requests.diviner index 9a1fd40c45..a5f8b610c0 100644 --- a/src/docs/contributor/feature_requests.diviner +++ b/src/docs/contributor/feature_requests.diviner @@ -3,6 +3,13 @@ Describes how to file an effective Phabricator feature request. +Describe Your Problem! +====================== + +IMPORTANT: When filing a feature request, you **MUST** describe the root +problem you are facing. We will not accept feature requests which do not +include a problem description. See below for details. + Overview ======== @@ -138,8 +145,9 @@ upstream. Describe Problems ================= -When you file a feature request, it is really helpful to describe the problem -you're facing first, not just your desired solution. +When you file a feature request, we need you to describe the problem you're +facing first, not just your desired solution. Describing the problem you are +facing is the **most important part** of a feature request. Often, your problem may have a lot in common with other similar problems. If we understand your use case we can compare it to other use cases and sometimes find @@ -159,6 +167,10 @@ which has approximately the same effect and does fit into the product direction. If you only describe the solution and not the problem, we can't generalize, contextualize, merge, reframe, or offer alternative solutions or workarounds. +You must describe the problem you are facing when filing a feature request. We +will not accept feature requests which do not contextualize the request by +describing the root problem. + Hypotheticals ============= @@ -206,13 +218,13 @@ Create a Task in Maniphest If you think your feature might be a good fit for the upstream, have reasonable expectations about it, and have a good description of the problem you're trying -to solve, you're ready to file a feature request: +to solve, you're ready to file a feature request. -https://secure.phabricator.com/maniphest/task/create/ +It is **particularly critical** that you describe the problem you are facing, +not just the feature you want. We will not accept feature requests which do +not describe the root problem the feature is intended to resolve. -If you have a quick question or want to discuss something before filing a -request, IRC is the best way to get a quick answer. You can find information -about IRC and other support channels in @{article: Give Feedback! Get Support!}. +(NOTE) https://secure.phabricator.com/maniphest/task/create/ Next Steps @@ -221,6 +233,5 @@ Next Steps Continue by: - learning about @{article: Contributing Bug Reports}; or - - reading general support information in - @{article: Give Feedback! Get Support!}; or + - reading general support information in @{article:Support Resources}; or - returning to the @{article:Contributor Introduction}. diff --git a/src/docs/user/feedback.diviner b/src/docs/user/feedback.diviner index 6fc4ca1fcb..0998002b10 100644 --- a/src/docs/user/feedback.diviner +++ b/src/docs/user/feedback.diviner @@ -1,44 +1,7 @@ @title Give Feedback! Get Support! @short Feedback/Support -@group intro +@group cellar -How to give us feedback, report bugs, and request features, and get support for -problems with Phabricator. +Deprecated. -Overview -======== - -We'd love to hear your feedback about Phabricator, whether it's good or bad. The -best ways to provide feedback and get support are: - - - For bug reports and feature requests, file a task in our task tracker, - Maniphest. - - For questions and real-time chat, join the IRC channel. - - If you just want to provide some quick feedback, you can tweet at - us ([[ http://twitter.com/phabricator | @phabricator ]]). - -Bugs and Requests -===================== - -The best way to report bugs and request features is through -[[http://secure.phabricator.com/maniphest/task/create/ | Maniphest]]. For -information on filing good bug reports and feature requests, see: - - - @{article:Contributing Bug Reports} - - @{article:Contributing Feature Requests} - -IRC -========== - -We're active in `#phabricator` on FreeNode, and it's the best place to ask -questions and get support. - -Next Steps -========== - -Continue by: - - - learning more about bug reports with @{article:Contributing Bug Reports}; or - - learning more about feature requests with - @{article:Contributing Feature Requests}; or - - contributing to Phabricator with @{article:Contributor Introduction}. +This article has moved to @{article:Support Resources}. diff --git a/src/docs/user/reporting_security.diviner b/src/docs/user/reporting_security.diviner index 445313c970..c74fc40e1a 100644 --- a/src/docs/user/reporting_security.diviner +++ b/src/docs/user/reporting_security.diviner @@ -3,7 +3,8 @@ Describes how to report security vulnerabilities in Phabricator. -= Overview = +Overview +======== Phabricator runs a disclosure and award program through [[ https://www.hackerone.com/ | HackerOne ]]. This program is the best way to @@ -19,23 +20,17 @@ how to participate. We have a 24 hour response timeline, and are usually able to respond to (and, very often, fix) issues more quickly than that. -= Other Channels = -You can also contact us on another channel if you prefer. See -@{article:Give Feedback! Get Support!} for a list of ways to get in touch -with us. +Other Channels +============== -= Getting Notified = +If you aren't sure if something qualifies or don't want to report via +HackerOne, you can submit the issue as a normal bug report. For instructions, +see @{article:Contributing Bug Reports}. -When we fix significant security vulnerabilities, we currently publish -information: - - on our [[ https://www.facebook.com/phabricator | Facebook Page ]]; - - on our [[ https://twitter.com/phabricator | Twitter Feed ]]; - - and on IRC (`#phabricator` on FreeNode). +Get Updated +=========== -If you'd prefer to receive information on other channels, let us know. - -General information about security is reported monthly in the -[[ http://phabricator.org/changelog/ | Changelog ]]. This includes low impact -issues, reports we did not act on, and other details. +General information about security changes is reported weekly in the +[[ https://secure.phabricator.com/w/changelog/ | Changelog ]]. diff --git a/src/docs/user/support.diviner b/src/docs/user/support.diviner new file mode 100644 index 0000000000..3402c099e7 --- /dev/null +++ b/src/docs/user/support.diviner @@ -0,0 +1,125 @@ +@title Support Resources +@short Support +@group intro + +Resources for reporting bugs, requesting features, and getting support. + +Overview +======== + +This document describes available support resources. + +The upstream provides active, free support for a narrow range of problems +(primarily, security issues and reproducible bugs). + +The upstream does not provide free support for general problems with installing +or configuring Phabricator. You may be able to get some help with these +kinds of issues from the community. + + +Reporting Security Vulnerabilities +================================== + +The upstream accepts, fixes, and awards bounties for reports of material +security issues with the software. + +To report security issues, see @{article:Reporting Security Vulnerabilities}. + + +Reporting Bugs +============== + +The upstream will accept **reproducible** bug reports in modern, first-party +production code running in reasonable environments. + +To report bugs, see @{article:Contributing Bug Reports}. + + +Requesting Features +=================== + +The upstream accepts feature requests which **describe problems** you would +like Phabricator to be able to solve. + +To request features, see @{article:Contributing Feature Requests}. + + +Contributing +============ + +Phabricator is a very difficult project to contribute to. New contributors +will face a high barrier to entry. + +If you'd like to contribute to Phabricator, start with +@{article:Contributor Introduction}. + + + +Installation and Setup Help +=========================== + +Installation and setup help is available from the upstream at consulting rates. +See [[ https://secure.phabricator.com/w/consulting/ | Consulting ]] for details. + +Helping individual installs navigate unique setup problems takes our time +away from developing Phabricator, so we can not offer this service for free. + +You may be able to get free help with these issues from the community. See +below for details. + + +Hosting +========= + +The upstream offers Phabricator as a hosted service at +[[ https://phacility.com | Phacility ]]. This simplifies setting up and +operating a Phabricator instance, and gives you access to a broader range +of upstream support services. + +Running this service gives us a strong financial incentive to make installing +and operating Phabricator as difficult as possible. Blinded by greed, we toil +endlessly to make installation a perplexing nightmare that none other than +ourselves can hope to navigate. + + +Prioritization +============== + +The upstream offers prioritization, a service which allows you to control +our roadmap and get features you're interested in built sooner at reasonable +rates. See +[[ https://secure.phabricator.com/w/prioritization/ | Prioritization ]] for +details. + + +Consulting +========== + +The upstream offers general-purpose consulting services. See +[[ https://secure.phabricator.com/w/consulting/ | Consulting ]] for details. + + +Community +========= + +These resources are not provided by the upstream. They are not official support +channels and you may not receive support here, or you may receive help which is +misleading or wrong. + +You may be able to get answers to questions on sites like +[[ http://stackoverflow.com | Stack Overflow ]], +[[ https://www.quora.com | Quora ]], +[[ https://jelly.co | Jelly ]], or +[[ https://twitter.com | Twitter ]]. The upstream occasionally participates on +these sites but these are not official support channels and you should not +expect to receive a response. + +There is a +[[ https://secure.phabricator.com/conpherence/1336/ | General Chat ]] +Conpherence room on this install, and you can ask questions in +[[ https://secure.phabricator.com/ponder/ | Ponder ]]. These +are not upstream support channels and you may not receive a response to +questions, but someone in the community may be able to point you in the right +direction. + +There is also a community IRC channel in `#phabricator` on FreeNode. From bbbda23678ddbee4a8e473fa7966c7ad15e0a60e Mon Sep 17 00:00:00 2001 From: Chad Little Date: Mon, 19 Oct 2015 20:08:30 -0700 Subject: [PATCH 10/17] In PHUIInfoView, only show list UI if more than 1 item Summary: We often just setError as an array even if it's only one error. This just makes the UI a little cleaner in these cases. Test Plan: Remove all reviewers from a diff, see status error without list styling. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D14308 --- src/view/form/PHUIInfoView.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/view/form/PHUIInfoView.php b/src/view/form/PHUIInfoView.php index 2ebbbe44e7..3a8f259f52 100644 --- a/src/view/form/PHUIInfoView.php +++ b/src/view/form/PHUIInfoView.php @@ -50,7 +50,7 @@ final class PHUIInfoView extends AphrontView { require_celerity_resource('phui-info-view-css'); $errors = $this->errors; - if ($errors) { + if (count($errors) > 1) { $list = array(); foreach ($errors as $error) { $list[] = phutil_tag( @@ -64,6 +64,8 @@ final class PHUIInfoView extends AphrontView { 'class' => 'phui-info-view-list', ), $list); + } else if (count($errors) == 1) { + $list = $this->errors[0]; } else { $list = null; } From 22b9b76079bebcd0bd7d3702ac5af576f9e95195 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 20 Oct 2015 06:56:11 -0700 Subject: [PATCH 11/17] Fix control state for custom application policies with template types Summary: Fixes T9118. When populating some policy controls like "Default Can View" for repositories, we do some special logic to add object policies which are valid for the target object type. For example, it's OK to set the default policy for an object which has subscribers to "Subscribers". However, this logic incorrectly //removed// custom policies, so the form input ended up blank. Instead, provide both object policies and custom policies. Test Plan: - Set default view policy to a custom policy. - Hit "Edit" again, saw control correctly reflect custom policy after change. - Set default edit policy to a different custom policy. - Saved, edited, verified both policies stuck. - Set both policies back. - Checked some other object types to make sure object policies still work properly. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9118 Differential Revision: https://secure.phabricator.com/D14310 --- .../controller/PhabricatorApplicationEditController.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/applications/meta/controller/PhabricatorApplicationEditController.php b/src/applications/meta/controller/PhabricatorApplicationEditController.php index ba82b30053..d9d7b3c15c 100644 --- a/src/applications/meta/controller/PhabricatorApplicationEditController.php +++ b/src/applications/meta/controller/PhabricatorApplicationEditController.php @@ -145,7 +145,12 @@ final class PhabricatorApplicationEditController ->setViewer($user) ->setObject($template_object) ->execute(); - $control->setPolicies($template_policies); + + // NOTE: We want to expose both any object template policies + // (like "Subscribers") and any custom policy. + $all_policies = $template_policies + $policies; + + $control->setPolicies($all_policies); $control->setTemplateObject($template_object); } } From 09ab82faefaa5f8c78de2fb73ea66e2f41d08675 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Tue, 20 Oct 2015 09:02:55 -0700 Subject: [PATCH 12/17] Update Search for handleRequest Summary: Ref T8628. Updates Search. Test Plan: Did various searches, saved new queries, reordered, ran new queries. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T8628 Differential Revision: https://secure.phabricator.com/D14268 --- .../PhabricatorSearchController.php | 15 +++------ .../PhabricatorSearchDeleteController.php | 31 +++++++------------ .../PhabricatorSearchEditController.php | 25 ++++++--------- .../PhabricatorSearchHovercardController.php | 1 - .../PhabricatorSearchOrderController.php | 18 ++++------- 5 files changed, 30 insertions(+), 60 deletions(-) diff --git a/src/applications/search/controller/PhabricatorSearchController.php b/src/applications/search/controller/PhabricatorSearchController.php index cc05e9c5f4..d6fbf6356b 100644 --- a/src/applications/search/controller/PhabricatorSearchController.php +++ b/src/applications/search/controller/PhabricatorSearchController.php @@ -5,19 +5,12 @@ final class PhabricatorSearchController const SCOPE_CURRENT_APPLICATION = 'application'; - private $queryKey; - public function shouldAllowPublic() { return true; } - public function willProcessRequest(array $data) { - $this->queryKey = idx($data, 'queryKey'); - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); if ($request->getStr('jump') != 'no') { $pref_jump = PhabricatorUserPreferences::PREFERENCE_SEARCHBAR_JUMP; @@ -97,7 +90,7 @@ final class PhabricatorSearchController } $controller = id(new PhabricatorApplicationSearchController()) - ->setQueryKey($this->queryKey) + ->setQueryKey($request->getURIData('queryKey')) ->setSearchEngine($engine) ->setNavigation($this->buildSideNavView()); @@ -105,7 +98,7 @@ final class PhabricatorSearchController } public function buildSideNavView($for_app = false) { - $viewer = $this->getRequest()->getUser(); + $viewer = $this->getViewer(); $nav = new AphrontSideNavFilterView(); $nav->setBaseURI(new PhutilURI($this->getApplicationURI())); diff --git a/src/applications/search/controller/PhabricatorSearchDeleteController.php b/src/applications/search/controller/PhabricatorSearchDeleteController.php index d743fb39ba..f72c283519 100644 --- a/src/applications/search/controller/PhabricatorSearchDeleteController.php +++ b/src/applications/search/controller/PhabricatorSearchDeleteController.php @@ -3,33 +3,24 @@ final class PhabricatorSearchDeleteController extends PhabricatorSearchBaseController { - private $queryKey; - private $engineClass; - - public function willProcessRequest(array $data) { - $this->queryKey = idx($data, 'queryKey'); - $this->engineClass = idx($data, 'engine'); - } - - public function processRequest() { - $request = $this->getRequest(); - $user = $request->getUser(); - - $key = $this->queryKey; + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); + $key = $request->getURIData('queryKey'); + $engine_class = $request->getURIData('engine'); $base_class = 'PhabricatorApplicationSearchEngine'; - if (!is_subclass_of($this->engineClass, $base_class)) { + if (!is_subclass_of($engine_class, $base_class)) { return new Aphront400Response(); } - $engine = newv($this->engineClass, array()); - $engine->setViewer($user); + $engine = newv($engine_class, array()); + $engine->setViewer($viewer); $named_query = id(new PhabricatorNamedQueryQuery()) - ->setViewer($user) - ->withEngineClassNames(array($this->engineClass)) + ->setViewer($viewer) + ->withEngineClassNames(array($engine_class)) ->withQueryKeys(array($key)) - ->withUserPHIDs(array($user->getPHID())) + ->withUserPHIDs(array($viewer->getPHID())) ->executeOne(); if (!$named_query && $engine->isBuiltinQuery($key)) { @@ -84,7 +75,7 @@ final class PhabricatorSearchDeleteController } $dialog = id(new AphrontDialogView()) - ->setUser($user) + ->setUser($viewer) ->setTitle($title) ->appendChild($desc) ->addCancelButton($return_uri) diff --git a/src/applications/search/controller/PhabricatorSearchEditController.php b/src/applications/search/controller/PhabricatorSearchEditController.php index 3f089c5edf..da89919611 100644 --- a/src/applications/search/controller/PhabricatorSearchEditController.php +++ b/src/applications/search/controller/PhabricatorSearchEditController.php @@ -3,38 +3,31 @@ final class PhabricatorSearchEditController extends PhabricatorSearchBaseController { - private $queryKey; - - public function willProcessRequest(array $data) { - $this->queryKey = idx($data, 'queryKey'); - } - - public function processRequest() { - $request = $this->getRequest(); - $user = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); $saved_query = id(new PhabricatorSavedQueryQuery()) - ->setViewer($user) - ->withQueryKeys(array($this->queryKey)) + ->setViewer($viewer) + ->withQueryKeys(array($request->getURIData('queryKey'))) ->executeOne(); if (!$saved_query) { return new Aphront404Response(); } - $engine = $saved_query->newEngine()->setViewer($user); + $engine = $saved_query->newEngine()->setViewer($viewer); $complete_uri = $engine->getQueryManagementURI(); $cancel_uri = $complete_uri; $named_query = id(new PhabricatorNamedQueryQuery()) - ->setViewer($user) + ->setViewer($viewer) ->withQueryKeys(array($saved_query->getQueryKey())) - ->withUserPHIDs(array($user->getPHID())) + ->withUserPHIDs(array($viewer->getPHID())) ->executeOne(); if (!$named_query) { $named_query = id(new PhabricatorNamedQuery()) - ->setUserPHID($user->getPHID()) + ->setUserPHID($viewer->getPHID()) ->setQueryKey($saved_query->getQueryKey()) ->setEngineClassName($saved_query->getEngineClassName()); @@ -64,7 +57,7 @@ final class PhabricatorSearchEditController } $form = id(new AphrontFormView()) - ->setUser($user); + ->setUser($viewer); $form->appendChild( id(new AphrontFormTextControl()) diff --git a/src/applications/search/controller/PhabricatorSearchHovercardController.php b/src/applications/search/controller/PhabricatorSearchHovercardController.php index a05a2392b5..98d9aca68b 100644 --- a/src/applications/search/controller/PhabricatorSearchHovercardController.php +++ b/src/applications/search/controller/PhabricatorSearchHovercardController.php @@ -9,7 +9,6 @@ final class PhabricatorSearchHovercardController public function handleRequest(AphrontRequest $request) { $viewer = $this->getViewer(); - $phids = $request->getArr('phids'); $handles = id(new PhabricatorHandleQuery()) diff --git a/src/applications/search/controller/PhabricatorSearchOrderController.php b/src/applications/search/controller/PhabricatorSearchOrderController.php index 244caf83b0..e5f7887473 100644 --- a/src/applications/search/controller/PhabricatorSearchOrderController.php +++ b/src/applications/search/controller/PhabricatorSearchOrderController.php @@ -3,25 +3,19 @@ final class PhabricatorSearchOrderController extends PhabricatorSearchBaseController { - private $engineClass; - - public function willProcessRequest(array $data) { - $this->engineClass = idx($data, 'engine'); - } - - public function processRequest() { - $request = $this->getRequest(); - $user = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); + $engine_class = $request->getURIData('engine'); $request->validateCSRF(); $base_class = 'PhabricatorApplicationSearchEngine'; - if (!is_subclass_of($this->engineClass, $base_class)) { + if (!is_subclass_of($engine_class, $base_class)) { return new Aphront400Response(); } - $engine = newv($this->engineClass, array()); - $engine->setViewer($user); + $engine = newv($engine_class, array()); + $engine->setViewer($viewer); $queries = $engine->loadAllNamedQueries(); $queries = mpull($queries, null, 'getQueryKey'); From 4c1463eb569aac1ea825d0e3edfda2331b1f6335 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 20 Oct 2015 09:03:47 -0700 Subject: [PATCH 13/17] Probably fix bad URI construction for Diffusion symbols Summary: Ref T9532. Test Plan: I don't have this configured locally but this seems very likely to be the correct fix. This list should be a list of PHIDs, but is a list of PHIDs followed by one PhabricatorRepository object. Reviewers: avivey, chad Reviewed By: chad Maniphest Tasks: T9532 Differential Revision: https://secure.phabricator.com/D14311 --- .../diffusion/controller/DiffusionBrowseFileController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/diffusion/controller/DiffusionBrowseFileController.php b/src/applications/diffusion/controller/DiffusionBrowseFileController.php index fbd2785725..eb935208e5 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseFileController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseFileController.php @@ -288,7 +288,7 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController { $repo = $drequest->getRepository(); $symbol_repos = nonempty($repo->getSymbolSources(), array()); - $symbol_repos[] = $repo; + $symbol_repos[] = $repo->getPHID(); $lang = last(explode('.', $drequest->getPath())); $repo_languages = $repo->getSymbolLanguages(); From 421c2453e5e4bcdc5de97b0f4fd0f72ead89d8a3 Mon Sep 17 00:00:00 2001 From: Giedrius Dubinskas Date: Tue, 20 Oct 2015 19:07:04 +0000 Subject: [PATCH 14/17] Truncate long source lines in Paste search result list snippets Summary: An attempt to resolve T9600. - `PhabricatorPasteQuery` builds truncated snippet when requested using `needSnippet()`. - `PhabricatorPasteSearchEngine` uses Paste snippet istead of content. - `PhabricatorSourceCodeView` accepts truncated source and type instead of line limit. Test Plan: Generated some content for Paste application and also added huge JSON oneliner. Checked Paste application pages in browser. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Maniphest Tasks: T9600 Differential Revision: https://secure.phabricator.com/D14313 --- src/__phutil_library_map__.php | 2 + .../controller/PhabricatorPasteController.php | 2 - .../PhabricatorPasteViewController.php | 5 +- .../paste/query/PhabricatorPasteQuery.php | 138 ++++++++++++++++-- .../query/PhabricatorPasteSearchEngine.php | 10 +- .../paste/snippet/PhabricatorPasteSnippet.php | 24 +++ .../paste/storage/PhabricatorPaste.php | 10 ++ src/view/layout/PhabricatorSourceCodeView.php | 55 ++++--- 8 files changed, 203 insertions(+), 43 deletions(-) create mode 100644 src/applications/paste/snippet/PhabricatorPasteSnippet.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 89f65a8db0..5c1c071c44 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2552,6 +2552,7 @@ phutil_register_library_map(array( 'PhabricatorPasteRemarkupRule' => 'applications/paste/remarkup/PhabricatorPasteRemarkupRule.php', 'PhabricatorPasteSchemaSpec' => 'applications/paste/storage/PhabricatorPasteSchemaSpec.php', 'PhabricatorPasteSearchEngine' => 'applications/paste/query/PhabricatorPasteSearchEngine.php', + 'PhabricatorPasteSnippet' => 'applications/paste/snippet/PhabricatorPasteSnippet.php', 'PhabricatorPasteTestDataGenerator' => 'applications/paste/lipsum/PhabricatorPasteTestDataGenerator.php', 'PhabricatorPasteTransaction' => 'applications/paste/storage/PhabricatorPasteTransaction.php', 'PhabricatorPasteTransactionComment' => 'applications/paste/storage/PhabricatorPasteTransactionComment.php', @@ -6633,6 +6634,7 @@ phutil_register_library_map(array( 'PhabricatorPasteRemarkupRule' => 'PhabricatorObjectRemarkupRule', 'PhabricatorPasteSchemaSpec' => 'PhabricatorConfigSchemaSpec', 'PhabricatorPasteSearchEngine' => 'PhabricatorApplicationSearchEngine', + 'PhabricatorPasteSnippet' => 'Phobject', 'PhabricatorPasteTestDataGenerator' => 'PhabricatorTestDataGenerator', 'PhabricatorPasteTransaction' => 'PhabricatorApplicationTransaction', 'PhabricatorPasteTransactionComment' => 'PhabricatorApplicationTransactionComment', diff --git a/src/applications/paste/controller/PhabricatorPasteController.php b/src/applications/paste/controller/PhabricatorPasteController.php index 4956e89c28..1f543ac240 100644 --- a/src/applications/paste/controller/PhabricatorPasteController.php +++ b/src/applications/paste/controller/PhabricatorPasteController.php @@ -39,13 +39,11 @@ abstract class PhabricatorPasteController extends PhabricatorController { public function buildSourceCodeView( PhabricatorPaste $paste, - $max_lines = null, $highlights = array()) { $lines = phutil_split_lines($paste->getContent()); return id(new PhabricatorSourceCodeView()) - ->setLimit($max_lines) ->setLines($lines) ->setHighlights($highlights) ->setURI(new PhutilURI($paste->getURI())); diff --git a/src/applications/paste/controller/PhabricatorPasteViewController.php b/src/applications/paste/controller/PhabricatorPasteViewController.php index f8176892bc..46239e7fca 100644 --- a/src/applications/paste/controller/PhabricatorPasteViewController.php +++ b/src/applications/paste/controller/PhabricatorPasteViewController.php @@ -56,10 +56,7 @@ final class PhabricatorPasteViewController extends PhabricatorPasteController { ->setHeader($header) ->addPropertyList($properties); - $source_code = $this->buildSourceCodeView( - $paste, - null, - $this->highlightMap); + $source_code = $this->buildSourceCodeView($paste, $this->highlightMap); require_celerity_resource('paste-css'); $source_code = phutil_tag( diff --git a/src/applications/paste/query/PhabricatorPasteQuery.php b/src/applications/paste/query/PhabricatorPasteQuery.php index 768f0cf20b..2bc9b35168 100644 --- a/src/applications/paste/query/PhabricatorPasteQuery.php +++ b/src/applications/paste/query/PhabricatorPasteQuery.php @@ -10,6 +10,7 @@ final class PhabricatorPasteQuery private $needContent; private $needRawContent; + private $needSnippets; private $languages; private $includeNoLanguage; private $dateCreatedAfter; @@ -47,6 +48,11 @@ final class PhabricatorPasteQuery return $this; } + public function needSnippets($need_snippets) { + $this->needSnippets = $need_snippets; + return $this; + } + public function withLanguages(array $languages) { $this->includeNoLanguage = false; foreach ($languages as $key => $language) { @@ -91,6 +97,10 @@ final class PhabricatorPasteQuery $pastes = $this->loadContent($pastes); } + if ($this->needSnippets) { + $pastes = $this->loadSnippets($pastes); + } + return $pastes; } @@ -166,6 +176,17 @@ final class PhabricatorPasteQuery )); } + private function getSnippetCacheKey(PhabricatorPaste $paste) { + return implode( + ':', + array( + 'P'.$paste->getID(), + $paste->getFilePHID(), + $paste->getLanguage(), + 'snippet', + )); + } + private function loadRawContent(array $pastes) { $file_phids = mpull($pastes, 'getFilePHID'); $files = id(new PhabricatorFileQuery()) @@ -250,19 +271,114 @@ final class PhabricatorPasteQuery return $pastes; } - private function buildContent(PhabricatorPaste $paste) { - $language = $paste->getLanguage(); - $source = $paste->getRawContent(); + private function loadSnippets(array $pastes) { + $cache = new PhabricatorKeyValueDatabaseCache(); - if (empty($language)) { - return PhabricatorSyntaxHighlighter::highlightWithFilename( - $paste->getTitle(), - $source); - } else { - return PhabricatorSyntaxHighlighter::highlightWithLanguage( - $language, - $source); + $cache = new PhutilKeyValueCacheProfiler($cache); + $cache->setProfiler(PhutilServiceProfiler::getInstance()); + + $keys = array(); + foreach ($pastes as $paste) { + $keys[] = $this->getSnippetCacheKey($paste); } + + $caches = $cache->getKeys($keys); + + $need_raw = array(); + $have_cache = array(); + foreach ($pastes as $paste) { + $key = $this->getSnippetCacheKey($paste); + if (isset($caches[$key])) { + $snippet_data = phutil_json_decode($caches[$key], true); + $snippet = new PhabricatorPasteSnippet( + phutil_safe_html($snippet_data['content']), + $snippet_data['type']); + $paste->attachSnippet($snippet); + $have_cache[$paste->getPHID()] = true; + } else { + $need_raw[$key] = $paste; + } + } + + if (!$need_raw) { + return $pastes; + } + + $write_data = array(); + + $have_raw = $this->loadRawContent($need_raw); + $have_raw = mpull($have_raw, null, 'getPHID'); + foreach ($pastes as $key => $paste) { + $paste_phid = $paste->getPHID(); + if (isset($have_cache[$paste_phid])) { + continue; + } + + if (empty($have_raw[$paste_phid])) { + unset($pastes[$key]); + continue; + } + + $snippet = $this->buildSnippet($paste); + $paste->attachSnippet($snippet); + $snippet_data = array( + 'content' => (string)$snippet->getContent(), + 'type' => (string)$snippet->getType(), + ); + $write_data[$this->getSnippetCacheKey($paste)] = phutil_json_encode( + $snippet_data); + } + + if ($write_data) { + $cache->setKeys($write_data); + } + + return $pastes; + } + + private function buildContent(PhabricatorPaste $paste) { + return $this->highlightSource( + $paste->getRawContent(), + $paste->getTitle(), + $paste->getLanguage()); + } + + private function buildSnippet(PhabricatorPaste $paste) { + $snippet_type = PhabricatorPasteSnippet::FULL; + $snippet = $paste->getRawContent(); + + if (strlen($snippet) > 1024) { + $snippet_type = PhabricatorPasteSnippet::FIRST_BYTES; + $snippet = id(new PhutilUTF8StringTruncator()) + ->setMaximumBytes(1024) + ->setTerminator('') + ->truncateString($snippet); + } + + $lines = phutil_split_lines($snippet); + if (count($lines) > 5) { + $snippet_type = PhabricatorPasteSnippet::FIRST_LINES; + $snippet = implode('', array_slice($lines, 0, 5)); + } + + return new PhabricatorPasteSnippet( + $this->highlightSource( + $snippet, + $paste->getTitle(), + $paste->getLanguage()), + $snippet_type); + } + + private function highlightSource($source, $title, $language) { + if (empty($language)) { + return PhabricatorSyntaxHighlighter::highlightWithFilename( + $title, + $source); + } else { + return PhabricatorSyntaxHighlighter::highlightWithLanguage( + $language, + $source); + } } public function getQueryApplicationClass() { diff --git a/src/applications/paste/query/PhabricatorPasteSearchEngine.php b/src/applications/paste/query/PhabricatorPasteSearchEngine.php index e27445d078..0b4e47b497 100644 --- a/src/applications/paste/query/PhabricatorPasteSearchEngine.php +++ b/src/applications/paste/query/PhabricatorPasteSearchEngine.php @@ -13,7 +13,7 @@ final class PhabricatorPasteSearchEngine public function newQuery() { return id(new PhabricatorPasteQuery()) - ->needContent(true); + ->needSnippets(true); } protected function buildQueryFromParameters(array $map) { @@ -136,11 +136,15 @@ final class PhabricatorPasteSearchEngine $created = phabricator_date($paste->getDateCreated(), $viewer); $author = $handles[$paste->getAuthorPHID()]->renderLink(); - $lines = phutil_split_lines($paste->getContent()); + $snippet_type = $paste->getSnippet()->getType(); + $lines = phutil_split_lines($paste->getSnippet()->getContent()); $preview = id(new PhabricatorSourceCodeView()) - ->setLimit(5) ->setLines($lines) + ->setTruncatedFirstBytes( + $snippet_type == PhabricatorPasteSnippet::FIRST_BYTES) + ->setTruncatedFirstLines( + $snippet_type == PhabricatorPasteSnippet::FIRST_LINES) ->setURI(new PhutilURI($paste->getURI())); $source_code = phutil_tag( diff --git a/src/applications/paste/snippet/PhabricatorPasteSnippet.php b/src/applications/paste/snippet/PhabricatorPasteSnippet.php new file mode 100644 index 0000000000..4a001e5955 --- /dev/null +++ b/src/applications/paste/snippet/PhabricatorPasteSnippet.php @@ -0,0 +1,24 @@ +content = $content; + $this->type = $type; + } + + public function getContent() { + return $this->content; + } + + public function getType() { + return $this->type; + } +} diff --git a/src/applications/paste/storage/PhabricatorPaste.php b/src/applications/paste/storage/PhabricatorPaste.php index 0fab56b724..8aeda09aae 100644 --- a/src/applications/paste/storage/PhabricatorPaste.php +++ b/src/applications/paste/storage/PhabricatorPaste.php @@ -28,6 +28,7 @@ final class PhabricatorPaste extends PhabricatorPasteDAO private $content = self::ATTACHABLE; private $rawContent = self::ATTACHABLE; + private $snippet = self::ATTACHABLE; public static function initializeNewPaste(PhabricatorUser $actor) { $app = id(new PhabricatorApplicationQuery()) @@ -135,6 +136,15 @@ final class PhabricatorPaste extends PhabricatorPasteDAO return $this; } + public function getSnippet() { + return $this->assertAttached($this->snippet); + } + + public function attachSnippet(PhabricatorPasteSnippet $snippet) { + $this->snippet = $snippet; + return $this; + } + /* -( PhabricatorSubscribableInterface )----------------------------------- */ diff --git a/src/view/layout/PhabricatorSourceCodeView.php b/src/view/layout/PhabricatorSourceCodeView.php index 7fb3f2ca66..eb34925b3f 100644 --- a/src/view/layout/PhabricatorSourceCodeView.php +++ b/src/view/layout/PhabricatorSourceCodeView.php @@ -3,15 +3,11 @@ final class PhabricatorSourceCodeView extends AphrontView { private $lines; - private $limit; private $uri; private $highlights = array(); private $canClickHighlight = true; - - public function setLimit($limit) { - $this->limit = $limit; - return $this; - } + private $truncatedFirstBytes = false; + private $truncatedFirstLines = false; public function setLines(array $lines) { $this->lines = $lines; @@ -33,6 +29,16 @@ final class PhabricatorSourceCodeView extends AphrontView { return $this; } + public function setTruncatedFirstBytes($truncated_first_bytes) { + $this->truncatedFirstBytes = $truncated_first_bytes; + return $this; + } + + public function setTruncatedFirstLines($truncated_first_lines) { + $this->truncatedFirstLines = $truncated_first_lines; + return $this; + } + public function render() { require_celerity_resource('phabricator-source-code-view-css'); require_celerity_resource('syntax-highlighting-css'); @@ -46,24 +52,31 @@ final class PhabricatorSourceCodeView extends AphrontView { $rows = array(); - foreach ($this->lines as $line) { - $hit_limit = $this->limit && - ($line_number == $this->limit) && - (count($this->lines) != $this->limit); - - if ($hit_limit) { - $content_number = ''; - $content_line = phutil_tag( + $lines = $this->lines; + if ($this->truncatedFirstLines) { + $lines[] = phutil_tag( 'span', array( 'class' => 'c', ), pht('...')); - } else { - $content_number = $line_number; - // NOTE: See phabricator-oncopy behavior. - $content_line = hsprintf("\xE2\x80\x8B%s", $line); - } + } else if ($this->truncatedFirstBytes) { + $last_key = last_key($lines); + $lines[$last_key] = hsprintf( + '%s%s', + $lines[$last_key], + phutil_tag( + 'span', + array( + 'class' => 'c', + ), + pht('...'))); + } + + foreach ($lines as $line) { + + // NOTE: See phabricator-oncopy behavior. + $content_line = hsprintf("\xE2\x80\x8B%s", $line); $row_attributes = array(); if (isset($this->highlights[$line_number])) { @@ -106,10 +119,6 @@ final class PhabricatorSourceCodeView extends AphrontView { $content_line), )); - if ($hit_limit) { - break; - } - $line_number++; } From 5b619862cb06c4215ae61b4943eb6b5da7ce34a1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 21 Oct 2015 11:28:26 -0700 Subject: [PATCH 15/17] Show a more reasonable status element for pull requests Summary: Ref T182. Replace the total mess we had before with a sort-of-reasonable element. This automatically updates using "javascript". Test Plan: {F901983} {F901984} Used "Land Revision", saw the land status go from "Waiting" -> "Working" -> "Landed" without having to mash reload over and over again. Reviewers: chad Reviewed By: chad Maniphest Tasks: T182 Differential Revision: https://secure.phabricator.com/D14314 --- resources/celerity/map.php | 7 ++ src/__phutil_library_map__.php | 4 + .../DifferentialRevisionViewController.php | 29 ++----- .../PhabricatorDrydockApplication.php | 1 + ...ockRepositoryOperationStatusController.php | 59 +++++++++++++ .../DrydockLandRepositoryOperation.php | 24 ++++++ .../DrydockRepositoryOperationType.php | 4 + .../storage/DrydockRepositoryOperation.php | 16 ++++ .../DrydockRepositoryOperationStatusView.php | 84 +++++++++++++++++++ .../drydock/drydock-live-operation-status.js | 32 +++++++ 10 files changed, 237 insertions(+), 23 deletions(-) create mode 100644 src/applications/drydock/controller/DrydockRepositoryOperationStatusController.php create mode 100644 src/applications/drydock/view/DrydockRepositoryOperationStatusView.php create mode 100644 webroot/rsrc/js/application/drydock/drydock-live-operation-status.js diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 778c419638..176bf6509c 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -373,6 +373,7 @@ return array( 'rsrc/js/application/diffusion/behavior-locate-file.js' => '6d3e1947', 'rsrc/js/application/diffusion/behavior-pull-lastmodified.js' => 'f01586dc', 'rsrc/js/application/doorkeeper/behavior-doorkeeper-tag.js' => 'e5822781', + 'rsrc/js/application/drydock/drydock-live-operation-status.js' => '901935ef', 'rsrc/js/application/files/behavior-icon-composer.js' => '8ef9ab58', 'rsrc/js/application/files/behavior-launch-icon-composer.js' => '48086888', 'rsrc/js/application/herald/HeraldRuleEditor.js' => '91a6031b', @@ -576,6 +577,7 @@ return array( 'javelin-behavior-diffusion-locate-file' => '6d3e1947', 'javelin-behavior-diffusion-pull-lastmodified' => 'f01586dc', 'javelin-behavior-doorkeeper-tag' => 'e5822781', + 'javelin-behavior-drydock-live-operation-status' => '901935ef', 'javelin-behavior-durable-column' => 'c72aa091', 'javelin-behavior-error-log' => '6882e80a', 'javelin-behavior-event-all-day' => '38dcf3c8', @@ -1518,6 +1520,11 @@ return array( 'javelin-dom', 'javelin-stratcom', ), + '901935ef' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-request', + ), '91a6031b' => array( 'multirow-row-manager', 'javelin-install', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 5c1c071c44..d00b3a377d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -885,6 +885,8 @@ phutil_register_library_map(array( 'DrydockRepositoryOperationPHIDType' => 'applications/drydock/phid/DrydockRepositoryOperationPHIDType.php', 'DrydockRepositoryOperationQuery' => 'applications/drydock/query/DrydockRepositoryOperationQuery.php', 'DrydockRepositoryOperationSearchEngine' => 'applications/drydock/query/DrydockRepositoryOperationSearchEngine.php', + 'DrydockRepositoryOperationStatusController' => 'applications/drydock/controller/DrydockRepositoryOperationStatusController.php', + 'DrydockRepositoryOperationStatusView' => 'applications/drydock/view/DrydockRepositoryOperationStatusView.php', 'DrydockRepositoryOperationType' => 'applications/drydock/operation/DrydockRepositoryOperationType.php', 'DrydockRepositoryOperationUpdateWorker' => 'applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php', 'DrydockRepositoryOperationViewController' => 'applications/drydock/controller/DrydockRepositoryOperationViewController.php', @@ -4672,6 +4674,8 @@ phutil_register_library_map(array( 'DrydockRepositoryOperationPHIDType' => 'PhabricatorPHIDType', 'DrydockRepositoryOperationQuery' => 'DrydockQuery', 'DrydockRepositoryOperationSearchEngine' => 'PhabricatorApplicationSearchEngine', + 'DrydockRepositoryOperationStatusController' => 'DrydockController', + 'DrydockRepositoryOperationStatusView' => 'AphrontView', 'DrydockRepositoryOperationType' => 'Phobject', 'DrydockRepositoryOperationUpdateWorker' => 'DrydockWorker', 'DrydockRepositoryOperationViewController' => 'DrydockController', diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 2e1cb4c931..f6c90362df 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -1060,30 +1060,13 @@ final class DifferentialRevisionViewController extends DifferentialController { $operation = head(msort($operations, 'getID')); - // TODO: This is completely made up for now, give it useful information and - // a sweet progress bar. + $box_view = id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Active Operations')); - switch ($operation->getOperationState()) { - case DrydockRepositoryOperation::STATE_WAIT: - case DrydockRepositoryOperation::STATE_WORK: - $severity = PHUIInfoView::SEVERITY_NOTICE; - $text = pht( - 'Some sort of repository operation is currently running.'); - break; - default: - $severity = PHUIInfoView::SEVERITY_ERROR; - $text = pht( - 'Some sort of repository operation failed.'); - break; - } - - $info_view = id(new PHUIInfoView()) - ->setSeverity($severity) - ->appendChild($text); - - return id(new PHUIObjectBoxView()) - ->setHeaderText(pht('Active Operations (EXPERIMENTAL!)')) - ->setInfoView($info_view); + return id(new DrydockRepositoryOperationStatusView()) + ->setUser($viewer) + ->setBoxView($box_view) + ->setOperation($operation); } } diff --git a/src/applications/drydock/application/PhabricatorDrydockApplication.php b/src/applications/drydock/application/PhabricatorDrydockApplication.php index 237e4afd9a..0b109e68da 100644 --- a/src/applications/drydock/application/PhabricatorDrydockApplication.php +++ b/src/applications/drydock/application/PhabricatorDrydockApplication.php @@ -95,6 +95,7 @@ final class PhabricatorDrydockApplication extends PhabricatorApplication { => 'DrydockRepositoryOperationListController', '(?P[1-9]\d*)/' => array( '' => 'DrydockRepositoryOperationViewController', + 'status/' => 'DrydockRepositoryOperationStatusController', ), ), ), diff --git a/src/applications/drydock/controller/DrydockRepositoryOperationStatusController.php b/src/applications/drydock/controller/DrydockRepositoryOperationStatusController.php new file mode 100644 index 0000000000..6503886481 --- /dev/null +++ b/src/applications/drydock/controller/DrydockRepositoryOperationStatusController.php @@ -0,0 +1,59 @@ +getViewer(); + $id = $request->getURIData('id'); + + $operation = id(new DrydockRepositoryOperationQuery()) + ->setViewer($viewer) + ->withIDs(array($id)) + ->executeOne(); + if (!$operation) { + return new Aphront404Response(); + } + + $id = $operation->getID(); + + $status_view = id(new DrydockRepositoryOperationStatusView()) + ->setUser($viewer) + ->setOperation($operation); + + if ($request->isAjax()) { + $payload = array( + 'markup' => $status_view->renderUnderwayState(), + 'isUnderway' => $operation->isUnderway(), + ); + + return id(new AphrontAjaxResponse()) + ->setContent($payload); + } + + $title = pht('Repository Operation %d', $id); + + $crumbs = $this->buildApplicationCrumbs(); + $crumbs->addTextCrumb( + pht('Operations'), + $this->getApplicationURI('operation/')); + $crumbs->addTextCrumb($title); + + return $this->buildApplicationPage( + array( + $crumbs, + $status_view, + ), + array( + 'title' => array( + $title, + pht('Status'), + ), + )); + } + +} diff --git a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php index 050aaeeb36..df888f3f6f 100644 --- a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php +++ b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php @@ -11,6 +11,30 @@ final class DrydockLandRepositoryOperation return pht('Land Revision'); } + public function getOperationCurrentStatus( + DrydockRepositoryOperation $operation, + PhabricatorUser $viewer) { + + $target = $operation->getRepositoryTarget(); + $repository = $operation->getRepository(); + switch ($operation->getOperationState()) { + case DrydockRepositoryOperation::STATE_WAIT: + return pht( + 'Waiting to land revision into %s on %s...', + $repository->getMonogram(), + $target); + case DrydockRepositoryOperation::STATE_WORK: + return pht( + 'Landing revision into %s on %s...', + $repository->getMonogram(), + $target); + case DrydockRepositoryOperation::STATE_DONE: + return pht( + 'Revision landed into %s.', + $repository->getMonogram()); + } + } + public function applyOperation( DrydockRepositoryOperation $operation, DrydockInterface $interface) { diff --git a/src/applications/drydock/operation/DrydockRepositoryOperationType.php b/src/applications/drydock/operation/DrydockRepositoryOperationType.php index a78ed3173e..1ae6d6e4f8 100644 --- a/src/applications/drydock/operation/DrydockRepositoryOperationType.php +++ b/src/applications/drydock/operation/DrydockRepositoryOperationType.php @@ -12,6 +12,10 @@ abstract class DrydockRepositoryOperationType extends Phobject { DrydockRepositoryOperation $operation, PhabricatorUser $viewer); + abstract public function getOperationCurrentStatus( + DrydockRepositoryOperation $operation, + PhabricatorUser $viewer); + final public function setViewer(PhabricatorUser $viewer) { $this->viewer = $viewer; return $this; diff --git a/src/applications/drydock/storage/DrydockRepositoryOperation.php b/src/applications/drydock/storage/DrydockRepositoryOperation.php index 7a8e35ea68..2ee09f3227 100644 --- a/src/applications/drydock/storage/DrydockRepositoryOperation.php +++ b/src/applications/drydock/storage/DrydockRepositoryOperation.php @@ -142,6 +142,22 @@ final class DrydockRepositoryOperation extends DrydockDAO $viewer); } + public function getOperationCurrentStatus(PhabricatorUser $viewer) { + return $this->getImplementation()->getOperationCurrentStatus( + $this, + $viewer); + } + + public function isUnderway() { + switch ($this->getOperationState()) { + case self::STATE_WAIT: + case self::STATE_WORK: + return true; + } + + return false; + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/drydock/view/DrydockRepositoryOperationStatusView.php b/src/applications/drydock/view/DrydockRepositoryOperationStatusView.php new file mode 100644 index 0000000000..3bdb529c20 --- /dev/null +++ b/src/applications/drydock/view/DrydockRepositoryOperationStatusView.php @@ -0,0 +1,84 @@ +operation = $operation; + return $this; + } + + public function getOperation() { + return $this->operation; + } + + public function setBoxView(PHUIObjectBoxView $box_view) { + $this->boxView = $box_view; + return $this; + } + + public function getBoxView() { + return $this->boxView; + } + + public function render() { + $viewer = $this->getUser(); + $operation = $this->getOperation(); + + $list = $this->renderUnderwayState(); + + // If the operation is currently underway, refresh the status view. + if ($operation->isUnderway()) { + $status_id = celerity_generate_unique_node_id(); + $id = $operation->getID(); + + $list->setID($status_id); + + Javelin::initBehavior( + 'drydock-live-operation-status', + array( + 'statusID' => $status_id, + 'updateURI' => "/drydock/operation/{$id}/status/", + )); + } + + $box_view = $this->getBoxView(); + if (!$box_view) { + $box_view = id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Operation Status')); + } + $box_view->setObjectList($list); + + return $box_view; + } + + public function renderUnderwayState() { + $viewer = $this->getUser(); + $operation = $this->getOperation(); + + $id = $operation->getID(); + + $state = $operation->getOperationState(); + $icon = DrydockRepositoryOperation::getOperationStateIcon($state); + $name = DrydockRepositoryOperation::getOperationStateName($state); + + $item = id(new PHUIObjectItemView()) + ->setHref("/drydock/operation/{$id}/") + ->setHeader($operation->getOperationDescription($viewer)) + ->setStatusIcon($icon, $name); + + if ($state != DrydockRepositoryOperation::STATE_FAIL) { + $item->addAttribute($operation->getOperationCurrentStatus($viewer)); + } else { + // TODO: Make this more useful. + $item->addAttribute(pht('Operation encountered an error.')); + } + + return id(new PHUIObjectItemListView()) + ->addItem($item); + } + +} diff --git a/webroot/rsrc/js/application/drydock/drydock-live-operation-status.js b/webroot/rsrc/js/application/drydock/drydock-live-operation-status.js new file mode 100644 index 0000000000..dec4e81796 --- /dev/null +++ b/webroot/rsrc/js/application/drydock/drydock-live-operation-status.js @@ -0,0 +1,32 @@ +/** + * @provides javelin-behavior-drydock-live-operation-status + * @requires javelin-behavior + * javelin-dom + * javelin-request + * @javelin + */ + +JX.behavior('drydock-live-operation-status', function(config) { + var node = JX.$(config.statusID); + + function update() { + new JX.Request(config.updateURI, onresponse) + .send(); + } + + function onresponse(r) { + var new_node = JX.$H(r.markup).getNode(); + JX.DOM.replace(node, new_node); + node = new_node; + + if (r.isUnderway) { + poll(); + } + } + + function poll() { + setTimeout(update, 1000); + } + + poll(); +}); From 4afeebe8348962eccecc5488e012f9fa61de5475 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 21 Oct 2015 12:37:37 -0700 Subject: [PATCH 16/17] Don't store IP addresses in content sources Summary: We don't use these for anything, we're inconsistent about recording them, and there's some mild interaction with privacy concerns and data retention. Every other log we store any kind of information in can be given a custom retention policy after recent GC changes. If we did put this back eventually it would probably be better to store a session identifier anyway, since that's more granular and more detailed. You can fetch this info out of access logs anyway, too. Test Plan: Left a couple of comments. Reviewers: chad Reviewed By: chad Differential Revision: https://secure.phabricator.com/D14315 --- .../metamta/contentsource/PhabricatorContentSource.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/applications/metamta/contentsource/PhabricatorContentSource.php b/src/applications/metamta/contentsource/PhabricatorContentSource.php index 6d6a8ee896..e2320eb85f 100644 --- a/src/applications/metamta/contentsource/PhabricatorContentSource.php +++ b/src/applications/metamta/contentsource/PhabricatorContentSource.php @@ -54,9 +54,7 @@ final class PhabricatorContentSource extends Phobject { public static function newFromRequest(AphrontRequest $request) { return self::newForSource( self::SOURCE_WEB, - array( - 'ip' => $request->getRemoteAddr(), - )); + array()); } public static function newFromConduitRequest(ConduitAPIRequest $request) { From b038041dc600448c81c06c7b0f64c0d009245ca8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 24 Oct 2015 04:50:36 -0700 Subject: [PATCH 17/17] Prevent duplicate account links from being created by swapping logins and then refreshing the link Summary: Fixes T6707. Users can currently do this: - Log in to a service (like Facebook or Google) with account "A". - Link their Phabricator account to that account. - Log out of Facebook, log back in with account "B". - Refresh the account link from {nav Settings > External Accounts}. When they do this, we write a second account link (between their Phabricator account and account "B"). However, the rest of the codebase assumes accounts are singly-linked, so this breaks down elsewhere. For now, decline to link the second account. We'll permit this some day, but need to do more work to allow it, and the need is very rare. Test Plan: - Followed the steps above, hit the new error. - Logged back in to the proper account and did a link refresh (which worked). {F905562} Reviewers: chad Reviewed By: chad Maniphest Tasks: T6707 Differential Revision: https://secure.phabricator.com/D14319 --- .../PhabricatorAuthLoginController.php | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/applications/auth/controller/PhabricatorAuthLoginController.php b/src/applications/auth/controller/PhabricatorAuthLoginController.php index 65d462cb8e..be610e223a 100644 --- a/src/applications/auth/controller/PhabricatorAuthLoginController.php +++ b/src/applications/auth/controller/PhabricatorAuthLoginController.php @@ -113,6 +113,27 @@ final class PhabricatorAuthLoginController $provider->getProviderName())); } } else { + + // If the user already has a linked account of this type, prevent them + // from linking a second account. This can happen if they swap logins + // and then refresh the account link. See T6707. We will eventually + // allow this after T2549. + $existing_accounts = id(new PhabricatorExternalAccountQuery()) + ->setViewer($viewer) + ->withUserPHIDs(array($viewer->getPHID())) + ->withAccountTypes(array($account->getAccountType())) + ->execute(); + if ($existing_accounts) { + return $this->renderError( + pht( + 'Your Phabricator account is already connected to an external '. + 'account on this provider ("%s"), but you are currently logged '. + 'in to the provider with a different account. Log out of the '. + 'external service, then log back in with the correct account '. + 'before refreshing the account link.', + $provider->getProviderName())); + } + if ($provider->shouldAllowAccountLink()) { return $this->processLinkUser($account); } else {