From ff6ccd5f68ac553445b11e20f6a4fa46de08da6b Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Sat, 29 Aug 2015 18:51:26 +1000 Subject: [PATCH 01/35] Remove leftover code for "postponed" lint and unit status Summary: Ref T9134. It looks like this functionality was removed in D13848. Test Plan: Submitted a diff successfully. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin Maniphest Tasks: T9134 Differential Revision: https://secure.phabricator.com/D13869 --- .../DifferentialCreateDiffConduitAPIMethod.php | 7 ------- .../constants/DifferentialLintStatus.php | 13 ++++++------- .../constants/DifferentialUnitStatus.php | 13 ++++++------- .../constants/DifferentialUnitTestResult.php | 11 +++++------ .../customfield/DifferentialLintField.php | 4 ---- .../customfield/DifferentialUnitField.php | 4 ---- .../view/DifferentialRevisionUpdateHistoryView.php | 6 ------ .../view/HarbormasterUnitPropertyView.php | 1 - 8 files changed, 17 insertions(+), 42 deletions(-) diff --git a/src/applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php index 41f5f34770..bfa82893b3 100644 --- a/src/applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php @@ -26,7 +26,6 @@ final class DifferentialCreateDiffConduitAPIMethod 'okay', 'warn', 'fail', - 'postponed', )); return array( @@ -95,9 +94,6 @@ final class DifferentialCreateDiffConduitAPIMethod case 'fail': $lint_status = DifferentialLintStatus::LINT_FAIL; break; - case 'postponed': - $lint_status = DifferentialLintStatus::LINT_POSTPONED; - break; case 'none': default: $lint_status = DifferentialLintStatus::LINT_NONE; @@ -117,9 +113,6 @@ final class DifferentialCreateDiffConduitAPIMethod case 'fail': $unit_status = DifferentialUnitStatus::UNIT_FAIL; break; - case 'postponed': - $unit_status = DifferentialUnitStatus::UNIT_POSTPONED; - break; case 'none': default: $unit_status = DifferentialUnitStatus::UNIT_NONE; diff --git a/src/applications/differential/constants/DifferentialLintStatus.php b/src/applications/differential/constants/DifferentialLintStatus.php index 4b491db9c1..72709b8eef 100644 --- a/src/applications/differential/constants/DifferentialLintStatus.php +++ b/src/applications/differential/constants/DifferentialLintStatus.php @@ -2,12 +2,11 @@ final class DifferentialLintStatus extends Phobject { - const LINT_NONE = 0; - const LINT_OKAY = 1; - const LINT_WARN = 2; - const LINT_FAIL = 3; - const LINT_SKIP = 4; - const LINT_POSTPONED = 5; - const LINT_AUTO_SKIP = 6; + const LINT_NONE = 0; + const LINT_OKAY = 1; + const LINT_WARN = 2; + const LINT_FAIL = 3; + const LINT_SKIP = 4; + const LINT_AUTO_SKIP = 6; } diff --git a/src/applications/differential/constants/DifferentialUnitStatus.php b/src/applications/differential/constants/DifferentialUnitStatus.php index 65275b82b3..2b69853dfb 100644 --- a/src/applications/differential/constants/DifferentialUnitStatus.php +++ b/src/applications/differential/constants/DifferentialUnitStatus.php @@ -2,12 +2,11 @@ final class DifferentialUnitStatus extends Phobject { - const UNIT_NONE = 0; - const UNIT_OKAY = 1; - const UNIT_WARN = 2; - const UNIT_FAIL = 3; - const UNIT_SKIP = 4; - const UNIT_POSTPONED = 5; - const UNIT_AUTO_SKIP = 6; + const UNIT_NONE = 0; + const UNIT_OKAY = 1; + const UNIT_WARN = 2; + const UNIT_FAIL = 3; + const UNIT_SKIP = 4; + const UNIT_AUTO_SKIP = 6; } diff --git a/src/applications/differential/constants/DifferentialUnitTestResult.php b/src/applications/differential/constants/DifferentialUnitTestResult.php index 6ae919cf24..293544a0af 100644 --- a/src/applications/differential/constants/DifferentialUnitTestResult.php +++ b/src/applications/differential/constants/DifferentialUnitTestResult.php @@ -2,11 +2,10 @@ final class DifferentialUnitTestResult extends Phobject { - const RESULT_PASS = 'pass'; - const RESULT_FAIL = 'fail'; - const RESULT_SKIP = 'skip'; - const RESULT_BROKEN = 'broken'; - const RESULT_UNSOUND = 'unsound'; - const RESULT_POSTPONED = 'postponed'; + const RESULT_PASS = 'pass'; + const RESULT_FAIL = 'fail'; + const RESULT_SKIP = 'skip'; + const RESULT_BROKEN = 'broken'; + const RESULT_UNSOUND = 'unsound'; } diff --git a/src/applications/differential/customfield/DifferentialLintField.php b/src/applications/differential/customfield/DifferentialLintField.php index 52be0e393c..62b94c51eb 100644 --- a/src/applications/differential/customfield/DifferentialLintField.php +++ b/src/applications/differential/customfield/DifferentialLintField.php @@ -73,9 +73,6 @@ final class DifferentialLintField if ($status == DifferentialLintStatus::LINT_SKIP) { $warnings[] = pht( 'Lint was skipped when generating these changes.'); - } else if ($status == DifferentialLintStatus::LINT_POSTPONED) { - $warnings[] = pht( - 'Background linting has not finished executing on these changes.'); } else { $warnings[] = pht('These changes have lint problems.'); } @@ -94,7 +91,6 @@ final class DifferentialLintField DifferentialLintStatus::LINT_FAIL => 'red', DifferentialLintStatus::LINT_SKIP => 'blue', DifferentialLintStatus::LINT_AUTO_SKIP => 'blue', - DifferentialLintStatus::LINT_POSTPONED => 'blue', ); $icon_color = idx($colors, $diff->getLintStatus(), 'grey'); diff --git a/src/applications/differential/customfield/DifferentialUnitField.php b/src/applications/differential/customfield/DifferentialUnitField.php index b5de9fce53..3883baba86 100644 --- a/src/applications/differential/customfield/DifferentialUnitField.php +++ b/src/applications/differential/customfield/DifferentialUnitField.php @@ -84,9 +84,6 @@ final class DifferentialUnitField // Don't show any warnings. } else if ($status == DifferentialUnitStatus::UNIT_AUTO_SKIP) { // Don't show any warnings. - } else if ($status == DifferentialUnitStatus::UNIT_POSTPONED) { - $warnings[] = pht( - 'Background tests have not finished executing on these changes.'); } else if ($status == DifferentialUnitStatus::UNIT_SKIP) { $warnings[] = pht( 'Unit tests were skipped when generating these changes.'); @@ -108,7 +105,6 @@ final class DifferentialUnitField DifferentialUnitStatus::UNIT_FAIL => 'red', DifferentialUnitStatus::UNIT_SKIP => 'blue', DifferentialUnitStatus::UNIT_AUTO_SKIP => 'blue', - DifferentialUnitStatus::UNIT_POSTPONED => 'blue', ); $icon_color = idx($colors, $diff->getUnitStatus(), 'grey'); diff --git a/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php b/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php index 27d071aa68..082344a492 100644 --- a/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php +++ b/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php @@ -323,7 +323,6 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { DifferentialLintStatus::LINT_FAIL => self::STAR_FAIL, DifferentialLintStatus::LINT_SKIP => self::STAR_SKIP, DifferentialLintStatus::LINT_AUTO_SKIP => self::STAR_SKIP, - DifferentialLintStatus::LINT_POSTPONED => self::STAR_SKIP, ); $star = idx($map, $diff->getLintStatus(), self::STAR_FAIL); @@ -339,7 +338,6 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { DifferentialUnitStatus::UNIT_FAIL => self::STAR_FAIL, DifferentialUnitStatus::UNIT_SKIP => self::STAR_SKIP, DifferentialUnitStatus::UNIT_AUTO_SKIP => self::STAR_SKIP, - DifferentialUnitStatus::UNIT_POSTPONED => self::STAR_SKIP, ); $star = idx($map, $diff->getUnitStatus(), self::STAR_FAIL); @@ -361,8 +359,6 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { return pht('Lint Skipped'); case DifferentialLintStatus::LINT_AUTO_SKIP: return pht('Automatic diff as part of commit; lint not applicable.'); - case DifferentialLintStatus::LINT_POSTPONED: - return pht('Lint Postponed'); } return pht('Unknown'); } @@ -382,8 +378,6 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { case DifferentialUnitStatus::UNIT_AUTO_SKIP: return pht( 'Automatic diff as part of commit; unit tests not applicable.'); - case DifferentialUnitStatus::UNIT_POSTPONED: - return pht('Unit Tests Postponed'); } return pht('Unknown'); } diff --git a/src/applications/harbormaster/view/HarbormasterUnitPropertyView.php b/src/applications/harbormaster/view/HarbormasterUnitPropertyView.php index 107272246d..2614c25743 100644 --- a/src/applications/harbormaster/view/HarbormasterUnitPropertyView.php +++ b/src/applications/harbormaster/view/HarbormasterUnitPropertyView.php @@ -88,7 +88,6 @@ final class HarbormasterUnitPropertyView extends AphrontView { ArcanistUnitTestResult::RESULT_FAIL => pht('Failed'), ArcanistUnitTestResult::RESULT_UNSOUND => pht('Unsound'), ArcanistUnitTestResult::RESULT_SKIP => pht('Skipped'), - ArcanistUnitTestResult::RESULT_POSTPONED => pht('Postponed'), ArcanistUnitTestResult::RESULT_PASS => pht('Passed'), ); $result = idx($names, $result, $result); From 7904d4c29ceece355b19cf97a9aaac65e7e16dda Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Sat, 29 Aug 2015 22:37:07 +1000 Subject: [PATCH 02/35] Add some missing translations Summary: Fixes T8862. Test Plan: Eyeballed it. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Maniphest Tasks: T8862 Differential Revision: https://secure.phabricator.com/D14005 --- .../PhabricatorUSEnglishTranslation.php | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php index d9397d80c8..7a5f8b78dd 100644 --- a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php +++ b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php @@ -386,6 +386,13 @@ final class PhabricatorUSEnglishTranslation ), ), + '%s added %s reviewer(s) for %s: %s.' => array( + array( + '%s added a reviewer for %3$s: %4$s.', + '%s added reviewers for %3$s: %4$s.', + ), + ), + '%s removed %s reviewer(s): %s.' => array( array( '%s removed a reviewer: %3$s.', @@ -393,6 +400,13 @@ final class PhabricatorUSEnglishTranslation ), ), + '%s removed %s reviewer(s) for %s: %s.' => array( + array( + '%s removed a reviewer for %3$s: %4$s.', + '%s removed reviewers for %3$s: %4$s.', + ), + ), + '%d other(s)' => array( '1 other', '%d others', From d718415868544b1fc6b0e45487cbbeda5447ecce Mon Sep 17 00:00:00 2001 From: Chad Little Date: Sat, 29 Aug 2015 08:29:23 -0700 Subject: [PATCH 03/35] Swap duplicate close status on Ponder for invalid Summary: Until we have a proper close as duplicate workflow for Ponder, remove the option with something more sensible. Test Plan: Closed a question as invalid, saw it closed and in feed. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D14007 --- .../autopatches/20150829.ponder.dupe.1.sql | 2 ++ .../ponder/constants/PonderQuestionStatus.php | 20 +++++++++---------- .../storage/PonderQuestionTransaction.php | 8 ++++---- 3 files changed, 16 insertions(+), 14 deletions(-) create mode 100644 resources/sql/autopatches/20150829.ponder.dupe.1.sql diff --git a/resources/sql/autopatches/20150829.ponder.dupe.1.sql b/resources/sql/autopatches/20150829.ponder.dupe.1.sql new file mode 100644 index 0000000000..b5ba7f6538 --- /dev/null +++ b/resources/sql/autopatches/20150829.ponder.dupe.1.sql @@ -0,0 +1,2 @@ +UPDATE {$NAMESPACE}_ponder.ponder_question + SET status = 'invalid' WHERE status = 'duplicate'; diff --git a/src/applications/ponder/constants/PonderQuestionStatus.php b/src/applications/ponder/constants/PonderQuestionStatus.php index 61a446353e..55e433bf25 100644 --- a/src/applications/ponder/constants/PonderQuestionStatus.php +++ b/src/applications/ponder/constants/PonderQuestionStatus.php @@ -5,14 +5,14 @@ final class PonderQuestionStatus extends PonderConstants { const STATUS_OPEN = 'open'; const STATUS_CLOSED_RESOLVED = 'resolved'; const STATUS_CLOSED_OBSOLETE = 'obsolete'; - const STATUS_CLOSED_DUPLICATE = 'duplicate'; + const STATUS_CLOSED_INVALID = 'invalid'; public static function getQuestionStatusMap() { return array( self::STATUS_OPEN => pht('Open'), self::STATUS_CLOSED_RESOLVED => pht('Closed, Resolved'), self::STATUS_CLOSED_OBSOLETE => pht('Closed, Obsolete'), - self::STATUS_CLOSED_DUPLICATE => pht('Closed, Duplicate'), + self::STATUS_CLOSED_INVALID => pht('Closed, Invalid'), ); } @@ -21,7 +21,7 @@ final class PonderQuestionStatus extends PonderConstants { self::STATUS_OPEN => pht('Open'), self::STATUS_CLOSED_RESOLVED => pht('Closed, Resolved'), self::STATUS_CLOSED_OBSOLETE => pht('Closed, Obsolete'), - self::STATUS_CLOSED_DUPLICATE => pht('Closed, Duplicate'), + self::STATUS_CLOSED_INVALID => pht('Closed, Invalid'), ); return idx($map, $status, pht('Unknown')); } @@ -31,7 +31,7 @@ final class PonderQuestionStatus extends PonderConstants { self::STATUS_OPEN => pht('Open'), self::STATUS_CLOSED_RESOLVED => pht('Resolved'), self::STATUS_CLOSED_OBSOLETE => pht('Obsolete'), - self::STATUS_CLOSED_DUPLICATE => pht('Duplicate'), + self::STATUS_CLOSED_INVALID => pht('Invalid'), ); return idx($map, $status, pht('Unknown')); } @@ -43,9 +43,9 @@ final class PonderQuestionStatus extends PonderConstants { self::STATUS_CLOSED_RESOLVED => pht('This question has been answered or resolved.'), self::STATUS_CLOSED_OBSOLETE => - pht('This question is no longer valid or out of date.'), - self::STATUS_CLOSED_DUPLICATE => - pht('This question is a duplicate of another question.'), + pht('This question is out of date.'), + self::STATUS_CLOSED_INVALID => + pht('This question is invalid.'), ); return idx($map, $status, pht('Unknown')); } @@ -55,7 +55,7 @@ final class PonderQuestionStatus extends PonderConstants { self::STATUS_OPEN => PHUITagView::COLOR_BLUE, self::STATUS_CLOSED_RESOLVED => PHUITagView::COLOR_BLACK, self::STATUS_CLOSED_OBSOLETE => PHUITagView::COLOR_BLACK, - self::STATUS_CLOSED_DUPLICATE => PHUITagView::COLOR_BLACK, + self::STATUS_CLOSED_INVALID => PHUITagView::COLOR_BLACK, ); return idx($map, $status); @@ -66,7 +66,7 @@ final class PonderQuestionStatus extends PonderConstants { self::STATUS_OPEN => 'fa-question-circle', self::STATUS_CLOSED_RESOLVED => 'fa-check', self::STATUS_CLOSED_OBSOLETE => 'fa-ban', - self::STATUS_CLOSED_DUPLICATE => 'fa-clone', + self::STATUS_CLOSED_INVALID => 'fa-ban', ); return idx($map, $status); @@ -82,7 +82,7 @@ final class PonderQuestionStatus extends PonderConstants { return array( self::STATUS_CLOSED_RESOLVED, self::STATUS_CLOSED_OBSOLETE, - self::STATUS_CLOSED_DUPLICATE, + self::STATUS_CLOSED_INVALID, ); } diff --git a/src/applications/ponder/storage/PonderQuestionTransaction.php b/src/applications/ponder/storage/PonderQuestionTransaction.php index 9fb4edbf05..7b7188b448 100644 --- a/src/applications/ponder/storage/PonderQuestionTransaction.php +++ b/src/applications/ponder/storage/PonderQuestionTransaction.php @@ -100,9 +100,9 @@ final class PonderQuestionTransaction return pht( '%s closed this question as obsolete.', $this->renderHandleLink($author_phid)); - case PonderQuestionStatus::STATUS_CLOSED_DUPLICATE: + case PonderQuestionStatus::STATUS_CLOSED_INVALID: return pht( - '%s closed this question as a duplicate.', + '%s closed this question as invalid.', $this->renderHandleLink($author_phid)); } } @@ -272,9 +272,9 @@ final class PonderQuestionTransaction '%s closed %s as resolved.', $this->renderHandleLink($author_phid), $this->renderHandleLink($object_phid)); - case PonderQuestionStatus::STATUS_CLOSED_DUPLICATE: + case PonderQuestionStatus::STATUS_CLOSED_INVALID: return pht( - '%s closed %s as duplicate.', + '%s closed %s as invalid.', $this->renderHandleLink($author_phid), $this->renderHandleLink($object_phid)); case PonderQuestionStatus::STATUS_CLOSED_OBSOLETE: From a339e6de9e34e52392c826a2a5c7c7d3fe879391 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Sat, 29 Aug 2015 08:29:53 -0700 Subject: [PATCH 04/35] Update Conpherence layout for logged out view Summary: Fixes T9217, adds detection for logged in users and adjusts the layout accordingly. Test Plan: View logged in and logged out Conpherence Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T9217 Differential Revision: https://secure.phabricator.com/D14002 --- resources/celerity/map.php | 4 ++-- .../conpherence/view/ConpherenceLayoutView.php | 9 ++++++++- .../rsrc/css/application/conpherence/message-pane.css | 8 ++++++-- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 562aef2bd7..4cb738d0f0 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -47,7 +47,7 @@ return array( 'rsrc/css/application/config/unhandled-exception.css' => '4c96257a', 'rsrc/css/application/conpherence/durable-column.css' => '86396117', 'rsrc/css/application/conpherence/menu.css' => 'f99fee4c', - 'rsrc/css/application/conpherence/message-pane.css' => 'dd4f8a3b', + 'rsrc/css/application/conpherence/message-pane.css' => '5897d3ac', 'rsrc/css/application/conpherence/notification.css' => '6cdcc253', 'rsrc/css/application/conpherence/transaction.css' => '85d0974c', 'rsrc/css/application/conpherence/update.css' => 'faf6be09', @@ -509,7 +509,7 @@ return array( 'config-welcome-css' => '6abd79be', 'conpherence-durable-column-view' => '86396117', 'conpherence-menu-css' => 'f99fee4c', - 'conpherence-message-pane-css' => 'dd4f8a3b', + 'conpherence-message-pane-css' => '5897d3ac', 'conpherence-notification-css' => '6cdcc253', 'conpherence-thread-manager' => '01774ab2', 'conpherence-transaction-css' => '85d0974c', diff --git a/src/applications/conpherence/view/ConpherenceLayoutView.php b/src/applications/conpherence/view/ConpherenceLayoutView.php index 2549e556d4..62ca1bb6fa 100644 --- a/src/applications/conpherence/view/ConpherenceLayoutView.php +++ b/src/applications/conpherence/view/ConpherenceLayoutView.php @@ -90,6 +90,11 @@ final class ConpherenceLayoutView extends AphrontView { 'hasWidgets' => false, )); + $class = null; + if (!$this->getUser()->isLoggedIn()) { + $class = 'conpherence-logged-out'; + } + $this->initBehavior( 'conpherence-widget-pane', ConpherenceWidgetConfigConstants::getWidgetPaneBehaviorConfig()); @@ -99,7 +104,9 @@ final class ConpherenceLayoutView extends AphrontView { array( 'id' => $layout_id, 'sigil' => 'conpherence-layout', - 'class' => 'conpherence-layout conpherence-role-'.$this->role, + 'class' => 'conpherence-layout '. + $class. + ' conpherence-role-'.$this->role, ), array( javelin_tag( diff --git a/webroot/rsrc/css/application/conpherence/message-pane.css b/webroot/rsrc/css/application/conpherence/message-pane.css index 886700ccf5..c4b0bf6208 100644 --- a/webroot/rsrc/css/application/conpherence/message-pane.css +++ b/webroot/rsrc/css/application/conpherence/message-pane.css @@ -54,13 +54,17 @@ position: fixed; left: 241px; right: 241px; - top: 76px; + top: 78px; bottom: 172px; overflow-x: hidden; overflow-y: auto; -webkit-overflow-scrolling: touch; } +.conpherence-logged-out .conpherence-message-pane .conpherence-messages { + bottom: 42px; +} + .page-has-warning .conpherence-message-pane .conpherence-messages { top: 110px; } @@ -114,7 +118,7 @@ } .conpherence-message-pane .phui-form-view.login-to-participate { - height: 28px; + height: 26px; } .conpherence-message-pane .login-to-participate a.button { From 4c77ff68aaee7ee40ff739087808bcb91ae59f52 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Sat, 29 Aug 2015 08:33:25 -0700 Subject: [PATCH 05/35] Update Releeph for handleRequest Summary: Updates Releeph callsites to handleRequest Test Plan: Bounce around Releeph, cut a branch, edit a product, view history Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D14001 --- .../ReleephProductActionController.php | 18 ++++--------- .../ReleephProductCreateController.php | 3 +-- .../product/ReleephProductEditController.php | 14 +++------- .../ReleephProductHistoryController.php | 15 ++++------- .../product/ReleephProductListController.php | 11 +++----- .../product/ReleephProductViewController.php | 19 +++++--------- .../ReleephRequestActionController.php | 20 ++++---------- .../ReleephRequestCommentController.php | 14 +++------- ...ephRequestDifferentialCreateController.php | 17 +++++------- .../request/ReleephRequestEditController.php | 26 +++++++------------ .../ReleephRequestTypeaheadController.php | 4 +-- .../request/ReleephRequestViewController.php | 15 +++-------- 12 files changed, 54 insertions(+), 122 deletions(-) diff --git a/src/applications/releeph/controller/product/ReleephProductActionController.php b/src/applications/releeph/controller/product/ReleephProductActionController.php index cca9b0bf0a..da82e2d6ad 100644 --- a/src/applications/releeph/controller/product/ReleephProductActionController.php +++ b/src/applications/releeph/controller/product/ReleephProductActionController.php @@ -2,20 +2,13 @@ final class ReleephProductActionController extends ReleephProductController { - private $id; - private $action; - - public function willProcessRequest(array $data) { - $this->id = $data['projectID']; - $this->action = $data['action']; - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $request->getViewer(); + $id = $request->getURIData('projectID'); + $action = $request->getURIData('action'); $product = id(new ReleephProductQuery()) - ->withIDs(array($this->id)) + ->withIDs(array($id)) ->requireCapabilities( array( PhabricatorPolicyCapability::CAN_VIEW, @@ -32,7 +25,6 @@ final class ReleephProductActionController extends ReleephProductController { $product_id = $product->getID(); $product_uri = $this->getProductViewURI($product); - $action = $this->action; switch ($action) { case 'deactivate': case 'activate': diff --git a/src/applications/releeph/controller/product/ReleephProductCreateController.php b/src/applications/releeph/controller/product/ReleephProductCreateController.php index 6d5f29ff37..9772b293fe 100644 --- a/src/applications/releeph/controller/product/ReleephProductCreateController.php +++ b/src/applications/releeph/controller/product/ReleephProductCreateController.php @@ -2,8 +2,7 @@ final class ReleephProductCreateController extends ReleephProductController { - public function processRequest() { - $request = $this->getRequest(); + public function handleRequest(AphrontRequest $request) { $name = trim($request->getStr('name')); $trunk_branch = trim($request->getStr('trunkBranch')); $repository_phid = $request->getStr('repositoryPHID'); diff --git a/src/applications/releeph/controller/product/ReleephProductEditController.php b/src/applications/releeph/controller/product/ReleephProductEditController.php index d0769c7b28..7fd8e81563 100644 --- a/src/applications/releeph/controller/product/ReleephProductEditController.php +++ b/src/applications/releeph/controller/product/ReleephProductEditController.php @@ -2,19 +2,13 @@ final class ReleephProductEditController extends ReleephProductController { - private $productID; - - public function willProcessRequest(array $data) { - $this->productID = $data['projectID']; - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $request->getViewer(); + $id = $request->getURIData('projectID'); $product = id(new ReleephProductQuery()) ->setViewer($viewer) - ->withIDs(array($this->productID)) + ->withIDs(array($id)) ->requireCapabilities( array( PhabricatorPolicyCapability::CAN_VIEW, diff --git a/src/applications/releeph/controller/product/ReleephProductHistoryController.php b/src/applications/releeph/controller/product/ReleephProductHistoryController.php index 15d7139854..ebe9f15725 100644 --- a/src/applications/releeph/controller/product/ReleephProductHistoryController.php +++ b/src/applications/releeph/controller/product/ReleephProductHistoryController.php @@ -2,23 +2,17 @@ final class ReleephProductHistoryController extends ReleephProductController { - private $id; - public function shouldAllowPublic() { return true; } - public function willProcessRequest(array $data) { - $this->id = $data['projectID']; - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $request->getViewer(); + $id = $request->getURIData('projectID'); $product = id(new ReleephProductQuery()) ->setViewer($viewer) - ->withIDs(array($this->id)) + ->withIDs(array($id)) ->executeOne(); if (!$product) { return new Aphront404Response(); @@ -32,6 +26,7 @@ final class ReleephProductHistoryController extends ReleephProductController { $crumbs = $this->buildApplicationCrumbs(); $crumbs->addTextCrumb(pht('History')); + $crumbs->setBorder(true); return $this->buildApplicationPage( array( diff --git a/src/applications/releeph/controller/product/ReleephProductListController.php b/src/applications/releeph/controller/product/ReleephProductListController.php index 73d2b27416..14cc964e03 100644 --- a/src/applications/releeph/controller/product/ReleephProductListController.php +++ b/src/applications/releeph/controller/product/ReleephProductListController.php @@ -2,19 +2,14 @@ final class ReleephProductListController extends ReleephController { - 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) { + $query_key = $request->getURIData('queryKey'); $controller = id(new PhabricatorApplicationSearchController()) - ->setQueryKey($this->queryKey) + ->setQueryKey($query_key) ->setSearchEngine(new ReleephProductSearchEngine()) ->setNavigation($this->buildSideNavView()); diff --git a/src/applications/releeph/controller/product/ReleephProductViewController.php b/src/applications/releeph/controller/product/ReleephProductViewController.php index 3bccae0ea0..e35497eec7 100644 --- a/src/applications/releeph/controller/product/ReleephProductViewController.php +++ b/src/applications/releeph/controller/product/ReleephProductViewController.php @@ -2,25 +2,18 @@ final class ReleephProductViewController extends ReleephProductController { - private $productID; - private $queryKey; - public function shouldAllowPublic() { return true; } - public function willProcessRequest(array $data) { - $this->productID = idx($data, 'projectID'); - $this->queryKey = idx($data, 'queryKey'); - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $id = $request->getURIData('projectID'); + $query_key = $request->getURIData('queryKey'); + $viewer = $request->getViewer(); $product = id(new ReleephProductQuery()) ->setViewer($viewer) - ->withIDs(array($this->productID)) + ->withIDs(array($id)) ->executeOne(); if (!$product) { return new Aphront404Response(); @@ -28,7 +21,7 @@ final class ReleephProductViewController extends ReleephProductController { $this->setProduct($product); $controller = id(new PhabricatorApplicationSearchController()) - ->setQueryKey($this->queryKey) + ->setQueryKey($query_key) ->setPreface($this->renderPreface()) ->setSearchEngine( id(new ReleephBranchSearchEngine()) diff --git a/src/applications/releeph/controller/request/ReleephRequestActionController.php b/src/applications/releeph/controller/request/ReleephRequestActionController.php index bbcc1df2bb..1a53b08b8c 100644 --- a/src/applications/releeph/controller/request/ReleephRequestActionController.php +++ b/src/applications/releeph/controller/request/ReleephRequestActionController.php @@ -3,23 +3,16 @@ final class ReleephRequestActionController extends ReleephRequestController { - private $action; - private $requestID; - - public function willProcessRequest(array $data) { - $this->action = $data['action']; - $this->requestID = $data['requestID']; - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $action = $request->getURIData('action'); + $id = $request->getURIData('requestID'); + $viewer = $request->getViewer(); $request->validateCSRF(); $pull = id(new ReleephRequestQuery()) ->setViewer($viewer) - ->withIDs(array($this->requestID)) + ->withIDs(array($id)) ->executeOne(); if (!$pull) { return new Aphront404Response(); @@ -27,9 +20,6 @@ final class ReleephRequestActionController $branch = $pull->getBranch(); $product = $branch->getProduct(); - - $action = $this->action; - $origin_uri = '/'.$pull->getMonogram(); $editor = id(new ReleephRequestTransactionalEditor()) diff --git a/src/applications/releeph/controller/request/ReleephRequestCommentController.php b/src/applications/releeph/controller/request/ReleephRequestCommentController.php index 4c728341af..0a31261a13 100644 --- a/src/applications/releeph/controller/request/ReleephRequestCommentController.php +++ b/src/applications/releeph/controller/request/ReleephRequestCommentController.php @@ -3,15 +3,9 @@ final class ReleephRequestCommentController extends ReleephRequestController { - private $requestID; - - public function willProcessRequest(array $data) { - $this->requestID = $data['requestID']; - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $id = $request->getURIData('requestID'); + $viewer = $request->getViewer(); if (!$request->isFormPost()) { return new Aphront400Response(); @@ -19,7 +13,7 @@ final class ReleephRequestCommentController $pull = id(new ReleephRequestQuery()) ->setViewer($viewer) - ->withIDs(array($this->requestID)) + ->withIDs(array($id)) ->executeOne(); if (!$pull) { return new Aphront404Response(); diff --git a/src/applications/releeph/controller/request/ReleephRequestDifferentialCreateController.php b/src/applications/releeph/controller/request/ReleephRequestDifferentialCreateController.php index 3c2dc3d735..ffd6388284 100644 --- a/src/applications/releeph/controller/request/ReleephRequestDifferentialCreateController.php +++ b/src/applications/releeph/controller/request/ReleephRequestDifferentialCreateController.php @@ -5,20 +5,15 @@ final class ReleephRequestDifferentialCreateController extends ReleephController { - private $revisionID; private $revision; - public function willProcessRequest(array $data) { - $this->revisionID = $data['diffRevID']; - } - - public function processRequest() { - $request = $this->getRequest(); - $user = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $revision_id = $request->getURIData('diffRevID'); + $viewer = $request->getViewer(); $diff_rev = id(new DifferentialRevisionQuery()) - ->setViewer($user) - ->withIDs(array($this->revisionID)) + ->setViewer($viewer) + ->withIDs(array($revision_id)) ->executeOne(); if (!$diff_rev) { return new Aphront404Response(); @@ -63,7 +58,7 @@ final class ReleephRequestDifferentialCreateController require_celerity_resource('releeph-request-differential-create-dialog'); $dialog = id(new AphrontDialogView()) - ->setUser($user) + ->setUser($viewer) ->setTitle(pht('Choose Releeph Branch')) ->setClass('releeph-request-differential-create-dialog') ->addCancelButton('/D'.$request->getStr('D')); diff --git a/src/applications/releeph/controller/request/ReleephRequestEditController.php b/src/applications/releeph/controller/request/ReleephRequestEditController.php index 279fee4bd6..d5f5187349 100644 --- a/src/applications/releeph/controller/request/ReleephRequestEditController.php +++ b/src/applications/releeph/controller/request/ReleephRequestEditController.php @@ -2,22 +2,16 @@ final class ReleephRequestEditController extends ReleephBranchController { - private $requestID; - private $branchID; + public function handleRequest(AphrontRequest $request) { + $action = $request->getURIData('action'); + $request_id = $request->getURIData('requestID'); + $branch_id = $request->getURIData('branchID'); + $viewer = $request->getViewer(); - public function willProcessRequest(array $data) { - $this->requestID = idx($data, 'requestID'); - $this->branchID = idx($data, 'branchID'); - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); - - if ($this->requestID) { + if ($request_id) { $pull = id(new ReleephRequestQuery()) ->setViewer($viewer) - ->withIDs(array($this->requestID)) + ->withIDs(array($request_id)) ->requireCapabilities( array( PhabricatorPolicyCapability::CAN_VIEW, @@ -34,7 +28,7 @@ final class ReleephRequestEditController extends ReleephBranchController { } else { $branch = id(new ReleephBranchQuery()) ->setViewer($viewer) - ->withIDs(array($this->branchID)) + ->withIDs(array($branch_id)) ->executeOne(); if (!$branch) { return new Aphront404Response(); @@ -77,8 +71,8 @@ final class ReleephRequestEditController extends ReleephBranchController { $field_list->readFieldsFromStorage($pull); - if ($this->branchID) { - $cancel_uri = $this->getApplicationURI('branch/'.$this->branchID.'/'); + if ($branch_id) { + $cancel_uri = $this->getApplicationURI('branch/'.$branch_id.'/'); } else { $cancel_uri = '/'.$pull->getMonogram(); } diff --git a/src/applications/releeph/controller/request/ReleephRequestTypeaheadController.php b/src/applications/releeph/controller/request/ReleephRequestTypeaheadController.php index ca49a52858..3a93de7453 100644 --- a/src/applications/releeph/controller/request/ReleephRequestTypeaheadController.php +++ b/src/applications/releeph/controller/request/ReleephRequestTypeaheadController.php @@ -3,9 +3,7 @@ final class ReleephRequestTypeaheadController extends PhabricatorTypeaheadDatasourceController { - public function processRequest() { - $request = $this->getRequest(); - + public function handleRequest(AphrontRequest $request) { $query = $request->getStr('q'); $repo_id = $request->getInt('repo'); $since = $request->getInt('since'); diff --git a/src/applications/releeph/controller/request/ReleephRequestViewController.php b/src/applications/releeph/controller/request/ReleephRequestViewController.php index 04f91b5fc6..694505cd20 100644 --- a/src/applications/releeph/controller/request/ReleephRequestViewController.php +++ b/src/applications/releeph/controller/request/ReleephRequestViewController.php @@ -3,19 +3,13 @@ final class ReleephRequestViewController extends ReleephBranchController { - private $requestID; - - public function willProcessRequest(array $data) { - $this->requestID = $data['requestID']; - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $id = $request->getURIData('requestID'); + $viewer = $request->getViewer(); $pull = id(new ReleephRequestQuery()) ->setViewer($viewer) - ->withIDs(array($this->requestID)) + ->withIDs(array($id)) ->executeOne(); if (!$pull) { return new Aphront404Response(); @@ -92,7 +86,6 @@ final class ReleephRequestViewController ), array( 'title' => $title, - 'device' => true, )); } From 96e7f766ff03e9db5b3fab47ddb4ba053b65f2bd Mon Sep 17 00:00:00 2001 From: Chad Little Date: Sat, 29 Aug 2015 11:18:22 -0700 Subject: [PATCH 06/35] Nudge users to close their question if it's been answered Summary: Adds a notice reminding viewers of their own question to resolve it and mark the correct answer. Test Plan: View my own open question, see notice. Resolve question, notice goes away. {F743481} Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D13958 --- .../PonderQuestionViewController.php | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/applications/ponder/controller/PonderQuestionViewController.php b/src/applications/ponder/controller/PonderQuestionViewController.php index fde0f067a3..3a4256c394 100644 --- a/src/applications/ponder/controller/PonderQuestionViewController.php +++ b/src/applications/ponder/controller/PonderQuestionViewController.php @@ -20,7 +20,7 @@ final class PonderQuestionViewController extends PonderController { return new Aphront404Response(); } - $answers = $this->buildAnswers($question->getAnswers()); + $answers = $this->buildAnswers($question); $answer_add_panel = id(new PonderAddAnswerView()) ->setQuestion($question) @@ -81,6 +81,20 @@ final class PonderQuestionViewController extends PonderController { ->addPropertyList($properties) ->appendChild($footer); + if ($viewer->getPHID() == $question->getAuthorPHID()) { + $status = $question->getStatus(); + $answers_list = $question->getAnswers(); + if ($answers_list && ($status == PonderQuestionStatus::STATUS_OPEN)) { + $info_view = id(new PHUIInfoView()) + ->setSeverity(PHUIInfoView::SEVERITY_WARNING) + ->appendChild( + pht( + 'If this question has been resolved, please consider closing + the question and marking the answer as helpful.')); + $object_box->setInfoView($info_view); + } + } + $crumbs = $this->buildApplicationCrumbs($this->buildSideNavView()); $crumbs->addTextCrumb('Q'.$id, '/Q'.$id); @@ -206,8 +220,9 @@ final class PonderQuestionViewController extends PonderController { * TODO - re-factor this to ajax in one answer panel at a time in a more * standard fashion. This is necessary to scale this application. */ - private function buildAnswers(array $answers) { + private function buildAnswers(PonderQuestion $question) { $viewer = $this->getViewer(); + $answers = $question->getAnswers(); $author_phids = mpull($answers, 'getAuthorPHID'); $handles = $this->loadViewerHandles($author_phids); From 2665970762bf52972cb60ac98a09bd5954115064 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Sat, 29 Aug 2015 13:59:33 -0700 Subject: [PATCH 07/35] Basic Answer Wiki for Ponder Summary: Adds an additional field for questions, an answer wiki, should should usually be community editable. Test Plan: New question, edit question, no wiki, lots of wiki. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D14003 --- .../autopatches/20150828.ponder.wiki.1.sql | 2 ++ .../PonderQuestionEditController.php | 22 ++++++++++++++++++- .../PonderQuestionViewController.php | 10 +++++++++ .../ponder/editor/PonderQuestionEditor.php | 8 +++++++ .../ponder/storage/PonderQuestion.php | 2 ++ .../storage/PonderQuestionTransaction.php | 14 ++++++++++++ 6 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 resources/sql/autopatches/20150828.ponder.wiki.1.sql diff --git a/resources/sql/autopatches/20150828.ponder.wiki.1.sql b/resources/sql/autopatches/20150828.ponder.wiki.1.sql new file mode 100644 index 0000000000..dbb8334de4 --- /dev/null +++ b/resources/sql/autopatches/20150828.ponder.wiki.1.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_ponder.ponder_question + ADD answerWiki LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL; diff --git a/src/applications/ponder/controller/PonderQuestionEditController.php b/src/applications/ponder/controller/PonderQuestionEditController.php index 41123d6049..d31db7c157 100644 --- a/src/applications/ponder/controller/PonderQuestionEditController.php +++ b/src/applications/ponder/controller/PonderQuestionEditController.php @@ -32,6 +32,7 @@ final class PonderQuestionEditController extends PonderController { $v_title = $question->getTitle(); $v_content = $question->getContent(); + $v_wiki = $question->getAnswerWiki(); $v_view = $question->getViewPolicy(); $v_space = $question->getSpacePHID(); $v_status = $question->getStatus(); @@ -42,6 +43,7 @@ final class PonderQuestionEditController extends PonderController { if ($request->isFormPost()) { $v_title = $request->getStr('title'); $v_content = $request->getStr('content'); + $v_wiki = $request->getStr('answerWiki'); $v_projects = $request->getArr('projects'); $v_view = $request->getStr('viewPolicy'); $v_space = $request->getStr('spacePHID'); @@ -68,6 +70,10 @@ final class PonderQuestionEditController extends PonderController { ->setTransactionType(PonderQuestionTransaction::TYPE_CONTENT) ->setNewValue($v_content); + $xactions[] = id(clone $template) + ->setTransactionType(PonderQuestionTransaction::TYPE_ANSWERWIKI) + ->setNewValue($v_wiki); + if (!$is_new) { $xactions[] = id(clone $template) ->setTransactionType(PonderQuestionTransaction::TYPE_STATUS) @@ -119,7 +125,15 @@ final class PonderQuestionEditController extends PonderController { ->setName('content') ->setID('content') ->setValue($v_content) - ->setLabel(pht('Description')) + ->setLabel(pht('Question Details')) + ->setUser($viewer)) + ->appendChild( + id(new PhabricatorRemarkupControl()) + ->setUser($viewer) + ->setName('answerWiki') + ->setID('answerWiki') + ->setValue($v_wiki) + ->setLabel(pht('Answer Summary')) ->setUser($viewer)) ->appendControl( id(new AphrontFormPolicyControl()) @@ -157,6 +171,11 @@ final class PonderQuestionEditController extends PonderController { ->setControlID('content') ->setPreviewURI($this->getApplicationURI('preview/')); + $answer_preview = id(new PHUIRemarkupPreviewPanel()) + ->setHeader(pht('Answer Summary Preview')) + ->setControlID('answerWiki') + ->setPreviewURI($this->getApplicationURI('preview/')); + $crumbs = $this->buildApplicationCrumbs(); $id = $question->getID(); @@ -179,6 +198,7 @@ final class PonderQuestionEditController extends PonderController { $crumbs, $form_box, $preview, + $answer_preview, ), array( 'title' => $title, diff --git a/src/applications/ponder/controller/PonderQuestionViewController.php b/src/applications/ponder/controller/PonderQuestionViewController.php index 3a4256c394..f7e70caab0 100644 --- a/src/applications/ponder/controller/PonderQuestionViewController.php +++ b/src/applications/ponder/controller/PonderQuestionViewController.php @@ -98,10 +98,20 @@ final class PonderQuestionViewController extends PonderController { $crumbs = $this->buildApplicationCrumbs($this->buildSideNavView()); $crumbs->addTextCrumb('Q'.$id, '/Q'.$id); + $answer_wiki = null; + if ($question->getAnswerWiki()) { + $answer = phutil_tag_div('mlt mlb msr msl', $question->getAnswerWiki()); + $answer_wiki = id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Answer Summary')) + ->setColor(PHUIObjectBoxView::COLOR_BLUE) + ->appendChild($answer); + } + $ponder_view = id(new PHUITwoColumnView()) ->setMainColumn(array( $object_box, $comment_view, + $answer_wiki, $answers, $answer_add_panel, )) diff --git a/src/applications/ponder/editor/PonderQuestionEditor.php b/src/applications/ponder/editor/PonderQuestionEditor.php index c2d9ef231b..b2f143b176 100644 --- a/src/applications/ponder/editor/PonderQuestionEditor.php +++ b/src/applications/ponder/editor/PonderQuestionEditor.php @@ -74,6 +74,7 @@ final class PonderQuestionEditor $types[] = PonderQuestionTransaction::TYPE_CONTENT; $types[] = PonderQuestionTransaction::TYPE_ANSWERS; $types[] = PonderQuestionTransaction::TYPE_STATUS; + $types[] = PonderQuestionTransaction::TYPE_ANSWERWIKI; return $types; } @@ -91,6 +92,8 @@ final class PonderQuestionEditor return mpull($object->getAnswers(), 'getPHID'); case PonderQuestionTransaction::TYPE_STATUS: return $object->getStatus(); + case PonderQuestionTransaction::TYPE_ANSWERWIKI: + return $object->getAnswerWiki(); } } @@ -102,6 +105,7 @@ final class PonderQuestionEditor case PonderQuestionTransaction::TYPE_TITLE: case PonderQuestionTransaction::TYPE_CONTENT: case PonderQuestionTransaction::TYPE_STATUS: + case PonderQuestionTransaction::TYPE_ANSWERWIKI: return $xaction->getNewValue(); case PonderQuestionTransaction::TYPE_ANSWERS: $raw_new_value = $xaction->getNewValue(); @@ -136,6 +140,9 @@ final class PonderQuestionEditor case PonderQuestionTransaction::TYPE_STATUS: $object->setStatus($xaction->getNewValue()); break; + case PonderQuestionTransaction::TYPE_ANSWERWIKI: + $object->setAnswerWiki($xaction->getNewValue()); + break; case PonderQuestionTransaction::TYPE_ANSWERS: $old = $xaction->getOldValue(); $new = $xaction->getNewValue(); @@ -167,6 +174,7 @@ final class PonderQuestionEditor case PonderQuestionTransaction::TYPE_TITLE: case PonderQuestionTransaction::TYPE_CONTENT: case PonderQuestionTransaction::TYPE_STATUS: + case PonderQuestionTransaction::TYPE_ANSWERWIKI: return $v; } diff --git a/src/applications/ponder/storage/PonderQuestion.php b/src/applications/ponder/storage/PonderQuestion.php index bc71690387..f02823b273 100644 --- a/src/applications/ponder/storage/PonderQuestion.php +++ b/src/applications/ponder/storage/PonderQuestion.php @@ -20,6 +20,7 @@ final class PonderQuestion extends PonderDAO protected $authorPHID; protected $status; protected $content; + protected $answerWiki; protected $contentSource; protected $viewPolicy; protected $spacePHID; @@ -56,6 +57,7 @@ final class PonderQuestion extends PonderDAO 'title' => 'text255', 'status' => 'text32', 'content' => 'text', + 'answerWiki' => 'text', 'answerCount' => 'uint32', 'mailKey' => 'bytes20', diff --git a/src/applications/ponder/storage/PonderQuestionTransaction.php b/src/applications/ponder/storage/PonderQuestionTransaction.php index 7b7188b448..cb90cc70a4 100644 --- a/src/applications/ponder/storage/PonderQuestionTransaction.php +++ b/src/applications/ponder/storage/PonderQuestionTransaction.php @@ -7,6 +7,7 @@ final class PonderQuestionTransaction const TYPE_CONTENT = 'ponder.question:content'; const TYPE_ANSWERS = 'ponder.question:answer'; const TYPE_STATUS = 'ponder.question:status'; + const TYPE_ANSWERWIKI = 'ponder.question:wiki'; const MAILTAG_DETAILS = 'question:details'; const MAILTAG_COMMENT = 'question:comment'; @@ -78,6 +79,10 @@ final class PonderQuestionTransaction return pht( '%s edited the question description.', $this->renderHandleLink($author_phid)); + case self::TYPE_ANSWERWIKI: + return pht( + '%s edited the question answer wiki.', + $this->renderHandleLink($author_phid)); case self::TYPE_ANSWERS: $answer_handle = $this->getHandle($this->getNewAnswerPHID()); $question_handle = $this->getHandle($object_phid); @@ -120,6 +125,7 @@ final class PonderQuestionTransaction case self::TYPE_TITLE: case self::TYPE_CONTENT: case self::TYPE_STATUS: + case self::TYPE_ANSWERWIKI: $tags[] = self::MAILTAG_DETAILS; break; case self::TYPE_ANSWERS: @@ -139,6 +145,7 @@ final class PonderQuestionTransaction switch ($this->getTransactionType()) { case self::TYPE_TITLE: case self::TYPE_CONTENT: + case self::TYPE_ANSWERWIKI: return 'fa-pencil'; case self::TYPE_STATUS: return PonderQuestionStatus::getQuestionStatusIcon($new); @@ -156,6 +163,7 @@ final class PonderQuestionTransaction switch ($this->getTransactionType()) { case self::TYPE_TITLE: case self::TYPE_CONTENT: + case self::TYPE_ANSWERWIKI: return PhabricatorTransactions::COLOR_BLUE; case self::TYPE_ANSWERS: return PhabricatorTransactions::COLOR_GREEN; @@ -167,6 +175,7 @@ final class PonderQuestionTransaction public function hasChangeDetails() { switch ($this->getTransactionType()) { case self::TYPE_CONTENT: + case self::TYPE_ANSWERWIKI: return true; } return parent::hasChangeDetails(); @@ -253,6 +262,11 @@ final class PonderQuestionTransaction '%s edited the description of %s', $this->renderHandleLink($author_phid), $this->renderHandleLink($object_phid)); + case self::TYPE_ANSWERWIKI: + return pht( + '%s edited the answer wiki for %s', + $this->renderHandleLink($author_phid), + $this->renderHandleLink($object_phid)); case self::TYPE_ANSWERS: $answer_handle = $this->getHandle($this->getNewAnswerPHID()); $question_handle = $this->getHandle($object_phid); From bcc5e55af2886e1cb81489460c2479b5ed4cd0bd Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 31 Aug 2015 04:01:01 -0700 Subject: [PATCH 08/35] Push construction of routing maps into Sites Summary: This enables CORGI. Currently, `AphrontSite` subclasses can't really have their own routes. They can do this sort of hacky rewriting of paths, but that's a mess and not desirable in the long run. Instead, let subclasses build their own routing maps. This will let CORP and ORG have their own routing maps. I was able to get rid of the `PhameBlogResourcesSite` since it can really just share the standard resources site. Test Plan: - With no base URI set, and a base URI set, loaded main page and resources (from main site). - With file domain set, loaded resources from main site and file site. - Loaded a skinned blog from a domain. - Loaded a skinned blog from the main site. - Viewed "Request" tab of DarkConsole to see site/controller info. Reviewers: chad Reviewed By: chad Differential Revision: https://secure.phabricator.com/D14008 --- scripts/aphront/aphrontpath.php | 28 --- src/__phutil_library_map__.php | 8 +- src/aphront/AphrontRequest.php | 20 +++ src/aphront/AphrontURIMapper.php | 50 ------ .../AphrontApplicationConfiguration.php | 114 ++++--------- src/aphront/site/AphrontRoutingMap.php | 159 ++++++++++++++++++ src/aphront/site/AphrontRoutingResult.php | 55 ++++++ src/aphront/site/AphrontSite.php | 20 +-- src/aphront/site/PhabricatorPlatformSite.php | 13 ++ src/aphront/site/PhabricatorResourceSite.php | 26 +-- .../base/PhabricatorApplication.php | 4 + .../PhabricatorCelerityApplication.php | 11 ++ .../plugin/DarkConsoleRequestPlugin.php | 127 ++++++++++---- .../PhabricatorFilesApplication.php | 39 +++-- .../PhabricatorPhameApplication.php | 31 +++- .../blog/PhameBlogLiveController.php | 20 ++- .../phame/site/PhameBlogResourceSite.php | 30 ---- src/applications/phame/site/PhameBlogSite.php | 13 +- 18 files changed, 481 insertions(+), 287 deletions(-) delete mode 100755 scripts/aphront/aphrontpath.php delete mode 100644 src/aphront/AphrontURIMapper.php create mode 100644 src/aphront/site/AphrontRoutingMap.php create mode 100644 src/aphront/site/AphrontRoutingResult.php delete mode 100644 src/applications/phame/site/PhameBlogResourceSite.php diff --git a/scripts/aphront/aphrontpath.php b/scripts/aphront/aphrontpath.php deleted file mode 100755 index 09cc04eeea..0000000000 --- a/scripts/aphront/aphrontpath.php +++ /dev/null @@ -1,28 +0,0 @@ -#!/usr/bin/env php -')."\n"; - echo pht( - "Purpose: Print controller which will process passed %s.\n", - ''); - exit(1); -} - -$url = parse_url($argv[1]); -$path = '/'.(isset($url['path']) ? ltrim($url['path'], '/') : ''); - -$config_key = 'aphront.default-application-configuration-class'; -$application = PhabricatorEnv::newObjectFromConfig($config_key); -$application->setRequest(new AphrontRequest('', $path)); - -list($controller) = $application->buildControllerForPath($path); -if (!$controller && substr($path, -1) !== '/') { - list($controller) = $application->buildControllerForPath($path.'/'); -} -if ($controller) { - echo get_class($controller)."\n"; -} diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 697a5ad11b..a80ca41d9a 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -158,6 +158,8 @@ phutil_register_library_map(array( 'AphrontRequest' => 'aphront/AphrontRequest.php', 'AphrontRequestTestCase' => 'aphront/__tests__/AphrontRequestTestCase.php', 'AphrontResponse' => 'aphront/response/AphrontResponse.php', + 'AphrontRoutingMap' => 'aphront/site/AphrontRoutingMap.php', + 'AphrontRoutingResult' => 'aphront/site/AphrontRoutingResult.php', 'AphrontSideNavFilterView' => 'view/layout/AphrontSideNavFilterView.php', 'AphrontSite' => 'aphront/site/AphrontSite.php', 'AphrontStackTraceView' => 'view/widget/AphrontStackTraceView.php', @@ -166,7 +168,6 @@ phutil_register_library_map(array( 'AphrontTagView' => 'view/AphrontTagView.php', 'AphrontTokenizerTemplateView' => 'view/control/AphrontTokenizerTemplateView.php', 'AphrontTypeaheadTemplateView' => 'view/control/AphrontTypeaheadTemplateView.php', - 'AphrontURIMapper' => 'aphront/AphrontURIMapper.php', 'AphrontUnhandledExceptionResponse' => 'aphront/response/AphrontUnhandledExceptionResponse.php', 'AphrontUsageException' => 'aphront/exception/AphrontUsageException.php', 'AphrontView' => 'view/AphrontView.php', @@ -3126,7 +3127,6 @@ phutil_register_library_map(array( 'PhameBlogListController' => 'applications/phame/controller/blog/PhameBlogListController.php', 'PhameBlogLiveController' => 'applications/phame/controller/blog/PhameBlogLiveController.php', 'PhameBlogQuery' => 'applications/phame/query/PhameBlogQuery.php', - 'PhameBlogResourceSite' => 'applications/phame/site/PhameBlogResourceSite.php', 'PhameBlogSearchEngine' => 'applications/phame/query/PhameBlogSearchEngine.php', 'PhameBlogSite' => 'applications/phame/site/PhameBlogSite.php', 'PhameBlogSkin' => 'applications/phame/skins/PhameBlogSkin.php', @@ -3777,6 +3777,8 @@ phutil_register_library_map(array( 'AphrontRequest' => 'Phobject', 'AphrontRequestTestCase' => 'PhabricatorTestCase', 'AphrontResponse' => 'Phobject', + 'AphrontRoutingMap' => 'Phobject', + 'AphrontRoutingResult' => 'Phobject', 'AphrontSideNavFilterView' => 'AphrontView', 'AphrontSite' => 'Phobject', 'AphrontStackTraceView' => 'AphrontView', @@ -3785,7 +3787,6 @@ phutil_register_library_map(array( 'AphrontTagView' => 'AphrontView', 'AphrontTokenizerTemplateView' => 'AphrontView', 'AphrontTypeaheadTemplateView' => 'AphrontView', - 'AphrontURIMapper' => 'Phobject', 'AphrontUnhandledExceptionResponse' => 'AphrontStandaloneHTMLResponse', 'AphrontUsageException' => 'AphrontException', 'AphrontView' => array( @@ -7238,7 +7239,6 @@ phutil_register_library_map(array( 'PhameBlogListController' => 'PhameController', 'PhameBlogLiveController' => 'PhameController', 'PhameBlogQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', - 'PhameBlogResourceSite' => 'PhameSite', 'PhameBlogSearchEngine' => 'PhabricatorApplicationSearchEngine', 'PhameBlogSite' => 'PhameSite', 'PhameBlogSkin' => 'PhabricatorController', diff --git a/src/aphront/AphrontRequest.php b/src/aphront/AphrontRequest.php index d29cd926d1..026129404e 100644 --- a/src/aphront/AphrontRequest.php +++ b/src/aphront/AphrontRequest.php @@ -26,6 +26,8 @@ final class AphrontRequest extends Phobject { private $requestData; private $user; private $applicationConfiguration; + private $site; + private $controller; private $uriData; private $cookiePrefix; @@ -77,6 +79,24 @@ final class AphrontRequest extends Phobject { return $uri->getDomain(); } + public function setSite(AphrontSite $site) { + $this->site = $site; + return $this; + } + + public function getSite() { + return $this->site; + } + + public function setController(AphrontController $controller) { + $this->controller = $controller; + return $this; + } + + public function getController() { + return $this->controller; + } + /* -( Accessing Request Data )--------------------------------------------- */ diff --git a/src/aphront/AphrontURIMapper.php b/src/aphront/AphrontURIMapper.php deleted file mode 100644 index 13108729b7..0000000000 --- a/src/aphront/AphrontURIMapper.php +++ /dev/null @@ -1,50 +0,0 @@ -map = $map; - } - - public function mapPath($path) { - $map = $this->map; - foreach ($map as $rule => $value) { - list($controller, $data) = $this->tryRule($rule, $value, $path); - if ($controller) { - foreach ($data as $k => $v) { - if (is_numeric($k)) { - unset($data[$k]); - } - } - return array($controller, $data); - } - } - - return array(null, null); - } - - private function tryRule($rule, $value, $path) { - $match = null; - $pattern = '#^'.$rule.(is_array($value) ? '' : '$').'#'; - if (!preg_match($pattern, $path, $match)) { - return array(null, null); - } - - if (!is_array($value)) { - return array($value, $match); - } - - $path = substr($path, strlen($match[0])); - foreach ($value as $srule => $sval) { - list($controller, $data) = $this->tryRule($srule, $sval, $path); - if ($controller) { - return array($controller, $data + $match); - } - } - - return array(null, null); - } - -} diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php index 7943b8046d..e37268380c 100644 --- a/src/aphront/configuration/AphrontApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontApplicationConfiguration.php @@ -210,7 +210,9 @@ abstract class AphrontApplicationConfiguration extends Phobject { )); $multimeter->setEventContext('web.'.$controller_class); + $request->setController($controller); $request->setURIMap($uri_data); + $controller->setRequest($request); // If execution throws an exception and then trying to render that @@ -283,35 +285,13 @@ abstract class AphrontApplicationConfiguration extends Phobject { /** - * Using builtin and application routes, build the appropriate - * @{class:AphrontController} class for the request. To route a request, we - * first test if the HTTP_HOST is configured as a valid Phabricator URI. If - * it isn't, we do a special check to see if it's a custom domain for a blog - * in the Phame application and if that fails we error. Otherwise, we test - * against all application routes from installed - * @{class:PhabricatorApplication}s. - * - * If we match a route, we construct the controller it points at, build it, - * and return it. - * - * If we fail to match a route, but the current path is missing a trailing - * "/", we try routing the same path with a trailing "/" and do a redirect - * if that has a valid route. The idea is to canoncalize URIs for consistency, - * but avoid breaking noncanonical URIs that we can easily salvage. - * - * NOTE: We only redirect on GET. On POST, we'd drop parameters and most - * likely mutate the request implicitly, and a bad POST usually indicates a - * programming error rather than a sloppy typist. - * - * If the failing path already has a trailing "/", or we can't route the - * version with a "/", we call @{method:build404Controller}, which build a - * fallback @{class:AphrontController}. + * Build a controller to respond to the request. * * @return pair Controller and dictionary of request * parameters. * @task routing */ - final public function buildController() { + final private function buildController() { $request = $this->getRequest(); // If we're configured to operate in cluster mode, reject requests which @@ -373,78 +353,48 @@ abstract class AphrontApplicationConfiguration extends Phobject { } } - // TODO: Really, the Site should get more control here and be able to - // do its own routing logic if it wants, but we don't need that for now. - $path = $site->getPathForRouting($request); + $maps = $site->getRoutingMaps(); + $path = $request->getPath(); - list($controller, $uri_data) = $this->buildControllerForPath($path); - if (!$controller) { - if (!preg_match('@/$@', $path)) { - // If we failed to match anything but don't have a trailing slash, try - // to add a trailing slash and issue a redirect if that resolves. - list($controller, $uri_data) = $this->buildControllerForPath($path.'/'); - - // NOTE: For POST, just 404 instead of redirecting, since the redirect - // will be a GET without parameters. - - if ($controller && !$request->isHTTPPost()) { - $slash_uri = $request->getRequestURI()->setPath($path.'/'); - - $external = strlen($request->getRequestURI()->getDomain()); - return $this->buildRedirectController($slash_uri, $external); - } - } - return $this->build404Controller(); + $result = $this->routePath($maps, $path); + if ($result) { + return $result; } - return array($controller, $uri_data); - } + // If we failed to match anything but don't have a trailing slash, try + // to add a trailing slash and issue a redirect if that resolves. + // NOTE: We only do this for GET, since redirects switch to GET and drop + // data like POST parameters. + if (!preg_match('@/$@', $path) && $request->isHTTPGet()) { + $result = $this->routePath($maps, $path.'/'); + if ($result) { + $slash_uri = $request->getRequestURI()->setPath($path.'/'); + $external = strlen($request->getRequestURI()->getDomain()); + return $this->buildRedirectController($slash_uri, $external); + } + } + + return $this->build404Controller(); + } /** * Map a specific path to the corresponding controller. For a description * of routing, see @{method:buildController}. * + * @param list List of routing maps. + * @param string Path to route. * @return pair Controller and dictionary of request * parameters. * @task routing */ - final public function buildControllerForPath($path) { - $maps = array(); - - $applications = PhabricatorApplication::getAllInstalledApplications(); - foreach ($applications as $application) { - $maps[] = array($application, $application->getRoutes()); - } - - $current_application = null; - $controller_class = null; - foreach ($maps as $map_info) { - list($application, $map) = $map_info; - - $mapper = new AphrontURIMapper($map); - list($controller_class, $uri_data) = $mapper->mapPath($path); - - if ($controller_class) { - if ($application) { - $current_application = $application; - } - break; + private function routePath(array $maps, $path) { + foreach ($maps as $map) { + $result = $map->routePath($path); + if ($result) { + return array($result->getController(), $result->getURIData()); } } - - if (!$controller_class) { - return array(null, null); - } - - $request = $this->getRequest(); - - $controller = newv($controller_class, array()); - if ($current_application) { - $controller->setCurrentApplication($current_application); - } - - return array($controller, $uri_data); } private function buildSiteForRequest(AphrontRequest $request) { @@ -469,6 +419,8 @@ abstract class AphrontApplicationConfiguration extends Phobject { $host)); } + $request->setSite($site); + return $site; } diff --git a/src/aphront/site/AphrontRoutingMap.php b/src/aphront/site/AphrontRoutingMap.php new file mode 100644 index 0000000000..bda98429f9 --- /dev/null +++ b/src/aphront/site/AphrontRoutingMap.php @@ -0,0 +1,159 @@ +site = $site; + return $this; + } + + public function getSite() { + return $this->site; + } + + public function setApplication(PhabricatorApplication $application) { + $this->application = $application; + return $this; + } + + public function getApplication() { + return $this->application; + } + + public function setRoutes(array $routes) { + $this->routes = $routes; + return $this; + } + + public function getRoutes() { + return $this->routes; + } + + +/* -( Routing )------------------------------------------------------------ */ + + + /** + * Find the route matching a path, if one exists. + * + * @param string Path to route. + * @return AphrontRoutingResult|null Routing result, if path matches map. + * @task routing + */ + public function routePath($path) { + $map = $this->getRoutes(); + + foreach ($map as $route => $value) { + $match = $this->tryRoute($route, $value, $path); + if (!$match) { + continue; + } + + $result = $this->newRoutingResult(); + $application = $result->getApplication(); + + $controller_class = $match['class']; + $controller = newv($controller_class, array()); + $controller->setCurrentApplication($application); + + $result + ->setController($controller) + ->setURIData($match['data']); + + return $result; + } + + return null; + } + + + /** + * Test a sub-map to see if any routes match a path. + * + * @param string Path to route. + * @param string Pattern from the map. + * @param string Value from the map. + * @return dict|null Match details, if path matches sub-map. + * @task routing + */ + private function tryRoute($route, $value, $path) { + $has_submap = is_array($value); + + if (!$has_submap) { + // If the value is a controller rather than a sub-map, any matching + // route must completely consume the path. + $pattern = '(^'.$route.'\z)'; + } else { + $pattern = '(^'.$route.')'; + } + + $data = null; + $ok = preg_match($pattern, $path, $data); + if ($ok === false) { + throw new Exception( + pht( + 'Routing fragment "%s" is not a valid regular expression.', + $route)); + } + + if (!$ok) { + return null; + } + + $path_match = $data[0]; + + // Clean up the data. We only want to retain named capturing groups, not + // the duplicated numeric captures. + foreach ($data as $k => $v) { + if (is_numeric($k)) { + unset($data[$k]); + } + } + + if (!$has_submap) { + return array( + 'class' => $value, + 'data' => $data, + ); + } + + $sub_path = substr($path, strlen($path_match)); + foreach ($value as $sub_route => $sub_value) { + $result = $this->tryRoute($sub_route, $sub_value, $sub_path); + if ($result) { + $result['data'] += $data; + return $result; + } + } + + return null; + } + + + /** + * Build a new routing result for this map. + * + * @return AphrontRoutingResult New, empty routing result. + * @task routing + */ + private function newRoutingResult() { + return id(new AphrontRoutingResult()) + ->setSite($this->getSite()) + ->setApplication($this->getApplication()); + } + +} diff --git a/src/aphront/site/AphrontRoutingResult.php b/src/aphront/site/AphrontRoutingResult.php new file mode 100644 index 0000000000..a6a5da767e --- /dev/null +++ b/src/aphront/site/AphrontRoutingResult.php @@ -0,0 +1,55 @@ +site = $site; + return $this; + } + + public function getSite() { + return $this->site; + } + + public function setApplication(PhabricatorApplication $application) { + $this->application = $application; + return $this; + } + + public function getApplication() { + return $this->application; + } + + public function setController(AphrontController $controller) { + $this->controller = $controller; + return $this; + } + + public function getController() { + return $this->controller; + } + + public function setURIData(array $uri_data) { + $this->uriData = $uri_data; + return $this; + } + + public function getURIData() { + return $this->uriData; + } + +} diff --git a/src/aphront/site/AphrontSite.php b/src/aphront/site/AphrontSite.php index 8c2373565b..b4d4fed0d1 100644 --- a/src/aphront/site/AphrontSite.php +++ b/src/aphront/site/AphrontSite.php @@ -7,14 +7,7 @@ abstract class AphrontSite extends Phobject { abstract public function shouldRequireHTTPS(); abstract public function newSiteForRequest(AphrontRequest $request); - - /** - * NOTE: This is temporary glue; eventually, sites will return an entire - * route map. - */ - public function getPathForRouting(AphrontRequest $request) { - return $request->getPath(); - } + abstract public function getRoutingMaps(); protected function isHostMatch($host, array $uris) { foreach ($uris as $uri) { @@ -32,14 +25,9 @@ abstract class AphrontSite extends Phobject { return false; } - protected function isPathPrefixMatch($path, array $paths) { - foreach ($paths as $candidate) { - if (strncmp($path, $candidate, strlen($candidate)) === 0) { - return true; - } - } - - return false; + protected function newRoutingMap() { + return id(new AphrontRoutingMap()) + ->setSite($this); } final public static function getAllSites() { diff --git a/src/aphront/site/PhabricatorPlatformSite.php b/src/aphront/site/PhabricatorPlatformSite.php index 63c2b9cde3..0973758bcd 100644 --- a/src/aphront/site/PhabricatorPlatformSite.php +++ b/src/aphront/site/PhabricatorPlatformSite.php @@ -37,4 +37,17 @@ final class PhabricatorPlatformSite extends PhabricatorSite { return null; } + public function getRoutingMaps() { + $applications = PhabricatorApplication::getAllInstalledApplications(); + + $maps = array(); + foreach ($applications as $application) { + $maps[] = $this->newRoutingMap() + ->setApplication($application) + ->setRoutes($application->getRoutes()); + } + + return $maps; + } + } diff --git a/src/aphront/site/PhabricatorResourceSite.php b/src/aphront/site/PhabricatorResourceSite.php index 006fcf99d7..88f7777607 100644 --- a/src/aphront/site/PhabricatorResourceSite.php +++ b/src/aphront/site/PhabricatorResourceSite.php @@ -22,20 +22,20 @@ final class PhabricatorResourceSite extends PhabricatorSite { return new PhabricatorResourceSite(); } - // These are CDN routes, so we let them through even if the "Host" header - // doesn't match anything we recognize. The - $whitelist = array( - '/res/', - '/file/data/', - '/file/xform/', - ); - - $path = $request->getPath(); - if ($this->isPathPrefixMatch($path, $whitelist)) { - return new PhabricatorResourceSite(); - } - return null; } + public function getRoutingMaps() { + $applications = PhabricatorApplication::getAllInstalledApplications(); + + $maps = array(); + foreach ($applications as $application) { + $maps[] = $this->newRoutingMap() + ->setApplication($application) + ->setRoutes($application->getResourceRoutes()); + } + + return $maps; + } + } diff --git a/src/applications/base/PhabricatorApplication.php b/src/applications/base/PhabricatorApplication.php index 1292282d25..f499bea5c9 100644 --- a/src/applications/base/PhabricatorApplication.php +++ b/src/applications/base/PhabricatorApplication.php @@ -243,6 +243,10 @@ abstract class PhabricatorApplication return array(); } + public function getResourceRoutes() { + return array(); + } + /* -( Email Integration )-------------------------------------------------- */ diff --git a/src/applications/celerity/application/PhabricatorCelerityApplication.php b/src/applications/celerity/application/PhabricatorCelerityApplication.php index e6c2015c9d..77ae9448ef 100644 --- a/src/applications/celerity/application/PhabricatorCelerityApplication.php +++ b/src/applications/celerity/application/PhabricatorCelerityApplication.php @@ -15,6 +15,17 @@ final class PhabricatorCelerityApplication extends PhabricatorApplication { } public function getRoutes() { + // We serve resources from both the platform site and the resource site. + // This is safe because the user doesn't have any direct control over + // resources. + + // The advantage of serving resources from the resource site (if possible) + // is that we can use a CDN there if one is configured, but there is no + // particular security concern. + return $this->getResourceRoutes(); + } + + public function getResourceRoutes() { $extensions = CelerityResourceController::getSupportedResourceTypes(); $extensions = array_keys($extensions); $extensions = implode('|', $extensions); diff --git a/src/applications/console/plugin/DarkConsoleRequestPlugin.php b/src/applications/console/plugin/DarkConsoleRequestPlugin.php index 8333903131..adba9e73e2 100644 --- a/src/applications/console/plugin/DarkConsoleRequestPlugin.php +++ b/src/applications/console/plugin/DarkConsoleRequestPlugin.php @@ -14,66 +14,121 @@ final class DarkConsoleRequestPlugin extends DarkConsolePlugin { } public function generateData() { + $addr = idx($_SERVER, 'SERVER_ADDR'); + if ($addr) { + $hostname = @gethostbyaddr($addr); + } else { + $hostname = null; + } + + $controller = $this->getRequest()->getController(); + if ($controller) { + $controller_class = get_class($controller); + } else { + $controller_class = null; + } + + $site = $this->getRequest()->getSite(); + if ($site) { + $site_class = get_class($site); + } else { + $site_class = null; + } + return array( - 'Request' => $_REQUEST, - 'Server' => $_SERVER, + 'request' => $_REQUEST, + 'server' => $_SERVER, + 'special' => array( + 'site' => $site_class, + 'controller' => $controller_class, + 'machine' => php_uname('n'), + 'host' => $addr, + 'hostname' => $hostname, + ), ); } public function renderPanel() { $data = $this->getData(); - $sections = array( - 'Basics' => array( - 'Machine' => php_uname('n'), - ), + $special_map = array( + 'site' => pht('Site'), + 'controller' => pht('Controller'), + 'machine' => pht('Machine'), + 'host' => pht('Host'), + 'hostname' => pht('Hostname'), ); - // NOTE: This may not be present for some SAPIs, like php-fpm. - if (!empty($data['Server']['SERVER_ADDR'])) { - $addr = $data['Server']['SERVER_ADDR']; - $sections['Basics']['Host'] = $addr; - $sections['Basics']['Hostname'] = @gethostbyaddr($addr); + $special = idx($data, 'special', array()); + + $rows = array(); + foreach ($special_map as $key => $label) { + $rows[] = array( + $label, + idx($special, $key), + ); } - $sections = array_merge($sections, $data); + $sections = array(); + $sections[] = array( + 'name' => pht('Basics'), + 'rows' => $rows, + ); $mask = array( 'HTTP_COOKIE' => true, 'HTTP_X_PHABRICATOR_CSRF' => true, ); - $out = array(); - foreach ($sections as $header => $map) { + $maps = array( + array( + 'name' => pht('Request'), + 'data' => idx($data, 'request', array()), + ), + array( + 'name' => pht('Server'), + 'data' => idx($data, 'server', array()), + ), + ); + + foreach ($maps as $map) { + $data = $map['data']; $rows = array(); - foreach ($map as $key => $value) { + foreach ($data as $key => $value) { if (isset($mask[$key])) { - $rows[] = array( - $key, - phutil_tag('em', array(), pht('(Masked)')), - ); + $value = phutil_tag('em', array(), pht('(Masked)')); + } else if (is_array($value)) { + $value = @json_encode($value); } else { - $rows[] = array( - $key, - (is_array($value) ? json_encode($value) : $value), - ); + $value = $value; } + + $rows[] = array( + $key, + $value, + ); } - $table = new AphrontTableView($rows); - $table->setHeaders( - array( - $header, - null, - )); - $table->setColumnClasses( - array( - 'header', - 'wide wrap', - )); - $out[] = $table->render(); + $sections[] = array( + 'name' => $map['name'], + 'rows' => $rows, + ); } - return phutil_implode_html("\n", $out); + $out = array(); + foreach ($sections as $section) { + $out[] = id(new AphrontTableView($section['rows'])) + ->setHeaders( + array( + $section['name'], + null, + )) + ->setColumnClasses( + array( + 'header', + 'wide wrap', + )); + } + return $out; } } diff --git a/src/applications/files/application/PhabricatorFilesApplication.php b/src/applications/files/application/PhabricatorFilesApplication.php index 6fed230fd2..7ad7b8f52c 100644 --- a/src/applications/files/application/PhabricatorFilesApplication.php +++ b/src/applications/files/application/PhabricatorFilesApplication.php @@ -78,25 +78,36 @@ final class PhabricatorFilesApplication extends PhabricatorApplication { 'delete/(?P[1-9]\d*)/' => 'PhabricatorFileDeleteController', 'edit/(?P[1-9]\d*)/' => 'PhabricatorFileEditController', 'info/(?P[^/]+)/' => 'PhabricatorFileInfoController', - 'data/'. - '(?:@(?P[^/]+)/)?'. - '(?P[^/]+)/'. - '(?P[^/]+)/'. - '(?:(?P[^/]+)/)?'. - '.*' - => 'PhabricatorFileDataController', 'proxy/' => 'PhabricatorFileProxyController', - 'xform/'. - '(?:@(?P[^/]+)/)?'. - '(?P[^/]+)/'. - '(?P[^/]+)/'. - '(?P[^/]+)/' - => 'PhabricatorFileTransformController', 'transforms/(?P[1-9]\d*)/' => 'PhabricatorFileTransformListController', 'uploaddialog/' => 'PhabricatorFileUploadDialogController', 'download/(?P[^/]+)/' => 'PhabricatorFileDialogController', - ), + ) + $this->getResourceSubroutes(), + ); + } + + public function getResourceRoutes() { + return array( + '/file/' => $this->getResourceSubroutes(), + ); + } + + private function getResourceSubroutes() { + return array( + 'data/'. + '(?:@(?P[^/]+)/)?'. + '(?P[^/]+)/'. + '(?P[^/]+)/'. + '(?:(?P[^/]+)/)?'. + '.*' + => 'PhabricatorFileDataController', + 'xform/'. + '(?:@(?P[^/]+)/)?'. + '(?P[^/]+)/'. + '(?P[^/]+)/'. + '(?P[^/]+)/' + => 'PhabricatorFileTransformController', ); } diff --git a/src/applications/phame/application/PhabricatorPhameApplication.php b/src/applications/phame/application/PhabricatorPhameApplication.php index 75c68852d0..f7643c6097 100644 --- a/src/applications/phame/application/PhabricatorPhameApplication.php +++ b/src/applications/phame/application/PhabricatorPhameApplication.php @@ -39,9 +39,6 @@ final class PhabricatorPhameApplication extends PhabricatorApplication { return array( '/phame/' => array( '' => 'PhamePostListController', - 'r/(?P\d+)/(?P[^/]+)/(?P.*)' - => 'PhameResourceController', - 'live/(?P[^/]+)/(?P.*)' => 'PhameBlogLiveController', 'post/' => array( '(?:(?Pdraft|all)/)?' => 'PhamePostListController', @@ -65,6 +62,34 @@ final class PhabricatorPhameApplication extends PhabricatorApplication { 'feed/(?P[^/]+)/' => 'PhameBlogFeedController', 'new/' => 'PhameBlogEditController', ), + ) + $this->getResourceSubroutes(), + ); + } + + public function getResourceRoutes() { + return array( + '/phame/' => $this->getResourceSubroutes(), + ); + } + + private function getResourceSubroutes() { + return array( + 'r/(?P\d+)/(?P[^/]+)/(?P.*)' => + 'PhameResourceController', + ); + } + + public function getBlogRoutes() { + return array( + '/(?P.*)' => 'PhameBlogLiveController', + ); + } + + public function getBlogCDNRoutes() { + return array( + '/phame/' => array( + 'r/(?P\d+)/(?P[^/]+)/(?P.*)' => + 'PhameResourceController', ), ); } diff --git a/src/applications/phame/controller/blog/PhameBlogLiveController.php b/src/applications/phame/controller/blog/PhameBlogLiveController.php index 1ac4486183..a79b45b799 100644 --- a/src/applications/phame/controller/blog/PhameBlogLiveController.php +++ b/src/applications/phame/controller/blog/PhameBlogLiveController.php @@ -8,14 +8,20 @@ final class PhameBlogLiveController extends PhameController { public function handleRequest(AphrontRequest $request) { $user = $request->getUser(); - $id = $request->getURIData('id'); - $blog = id(new PhameBlogQuery()) - ->setViewer($user) - ->withIDs(array($id)) - ->executeOne(); - if (!$blog) { - return new Aphront404Response(); + $site = $request->getSite(); + if ($site instanceof PhameBlogSite) { + $blog = $site->getBlog(); + } else { + $id = $request->getURIData('id'); + + $blog = id(new PhameBlogQuery()) + ->setViewer($user) + ->withIDs(array($id)) + ->executeOne(); + if (!$blog) { + return new Aphront404Response(); + } } if ($blog->getDomain() && ($request->getHost() != $blog->getDomain())) { diff --git a/src/applications/phame/site/PhameBlogResourceSite.php b/src/applications/phame/site/PhameBlogResourceSite.php deleted file mode 100644 index 8bf6094a77..0000000000 --- a/src/applications/phame/site/PhameBlogResourceSite.php +++ /dev/null @@ -1,30 +0,0 @@ -isPhameActive()) { - return null; - } - - $whitelist = array( - '/phame/r/', - ); - - $path = $request->getPath(); - if (!$this->isPathPrefixMatch($path, $whitelist)) { - return null; - } - - return new PhameBlogResourceSite(); - } - -} diff --git a/src/applications/phame/site/PhameBlogSite.php b/src/applications/phame/site/PhameBlogSite.php index f4e0a62c20..253fca0fbd 100644 --- a/src/applications/phame/site/PhameBlogSite.php +++ b/src/applications/phame/site/PhameBlogSite.php @@ -24,7 +24,7 @@ final class PhameBlogSite extends PhameSite { } public function getPriority() { - return 4000; + return 3000; } public function newSiteForRequest(AphrontRequest $request) { @@ -53,11 +53,14 @@ final class PhameBlogSite extends PhameSite { return id(new PhameBlogSite())->setBlog($blog); } - public function getPathForRouting(AphrontRequest $request) { - $path = $request->getPath(); - $id = $this->getBlog()->getID(); + public function getRoutingMaps() { + $app = PhabricatorApplication::getByClass('PhabricatorPhameApplication'); - return "/phame/live/{$id}/{$path}"; + $maps = array(); + $maps[] = $this->newRoutingMap() + ->setApplication($app) + ->setRoutes($app->getBlogRoutes()); + return $maps; } } From 506168c3073c283a6c54813a37849ac32153e555 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Mon, 31 Aug 2015 09:14:11 -0700 Subject: [PATCH 09/35] Show "Login to Answer" in Ponder if viewer is logged out Summary: Fixes T9278. Logged out viewers shouldn't see a form field to answer, just a login button. Test Plan: Log out, go to question, click Login to Answer, login, get redirected back. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T9278 Differential Revision: https://secure.phabricator.com/D14012 --- src/applications/ponder/view/PonderAddAnswerView.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/applications/ponder/view/PonderAddAnswerView.php b/src/applications/ponder/view/PonderAddAnswerView.php index 5d3be0460d..0c2e013051 100644 --- a/src/applications/ponder/view/PonderAddAnswerView.php +++ b/src/applications/ponder/view/PonderAddAnswerView.php @@ -90,6 +90,18 @@ final class PonderAddAnswerView extends AphrontView { id(new AphrontFormSubmitControl()) ->setValue(pht('Add Answer'))); + if (!$viewer->isLoggedIn()) { + $login_href = id(new PhutilURI('/auth/start/')) + ->setQueryParam('next', '/Q'.$question->getID()); + $form = id(new PHUIFormLayoutView()) + ->addClass('login-to-participate') + ->appendChild( + id(new PHUIButtonView()) + ->setTag('a') + ->setText(pht('Login to Answer')) + ->setHref((string)$login_href)); + } + $box = id(new PHUIObjectBoxView()) ->setHeader($header) ->appendChild($form); From 51c52f50fdda3f2a9c5b4223779371ee5735610e Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 31 Aug 2015 10:17:30 -0700 Subject: [PATCH 10/35] Prevent users from hiding unpublished inlines Summary: Fixes T9135. This is (probably) never intended and can be confusing. Test Plan: Saw no hide button on unpublished inlines. Saw hide button on published inlines. Clicked hide button. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9135 Differential Revision: https://secure.phabricator.com/D14014 --- .../view/PHUIDiffInlineCommentDetailView.php | 37 +++++++++++++++---- 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php index 80d93a7486..99ac7e1194 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php @@ -211,14 +211,14 @@ final class PHUIDiffInlineCommentDetailView ->addSigil('differential-inline-next') ->setMustCapture(true); - $hide = id(new PHUIButtonView()) - ->setTag('a') - ->setTooltip(pht('Hide Comment')) - ->setIconFont('fa-times') - ->addSigil('hide-inline') - ->setMustCapture(true); + if ($this->canHide()) { + $hide = id(new PHUIButtonView()) + ->setTag('a') + ->setTooltip(pht('Hide Comment')) + ->setIconFont('fa-times') + ->addSigil('hide-inline') + ->setMustCapture(true); - if ($viewer_phid && $inline->getID() && $inline->supportsHiding()) { $nextprev->addButton($hide); } @@ -456,4 +456,27 @@ final class PHUIDiffInlineCommentDetailView return $markup; } + private function canHide() { + $inline = $this->inlineComment; + + if ($inline->isDraft()) { + return false; + } + + if (!$inline->getID()) { + return false; + } + + $viewer = $this->getUser(); + if (!$viewer->isLoggedIn()) { + return false; + } + + if (!$inline->supportsHiding()) { + return false; + } + + return true; + } + } From e9614df76e0b27312f9a0b39847ec36231b2d8a0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 31 Aug 2015 10:17:37 -0700 Subject: [PATCH 11/35] Fix permission check for "Create Task" on workboards Summary: Fixes T9090. You don't need to be able to edit a project to create tasks on its workboard. Being able to view the project is sufficient, and the user certianly can if they got this far. Test Plan: Viewed workboard, hit "Create Task". Reviewers: chad Reviewed By: chad Maniphest Tasks: T9090 Differential Revision: https://secure.phabricator.com/D14015 --- .../controller/PhabricatorProjectBoardViewController.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index 8fd1810f33..077cca9a5f 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -624,8 +624,7 @@ final class PhabricatorProjectBoardViewController ->setMetadata( array( 'columnPHID' => $column->getPHID(), - )) - ->setDisabled(!$can_edit); + )); $batch_edit_uri = $request->getRequestURI(); $batch_edit_uri->setQueryParam('batch', $column->getID()); From e67c438943f2bd1e6144063ae9138da0f346f1f7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 31 Aug 2015 10:17:44 -0700 Subject: [PATCH 12/35] Rename "Edit Column" to "Column Details" Summary: Ref T9089. This link leads to a detail page, not an edit page, and is always visible by users with permission to see the column. Test Plan: Clicked "Column Details" with and without edit permission. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9089 Differential Revision: https://secure.phabricator.com/D14016 --- .../PhabricatorProjectBoardViewController.php | 10 ++++------ .../PhabricatorProjectColumnDetailController.php | 12 +----------- 2 files changed, 5 insertions(+), 17 deletions(-) diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index 077cca9a5f..ffe3ea2e73 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -639,15 +639,13 @@ final class PhabricatorProjectBoardViewController ->setHref($batch_edit_uri) ->setDisabled(!$can_batch_edit); - $edit_uri = $this->getApplicationURI( + $detail_uri = $this->getApplicationURI( 'board/'.$this->id.'/column/'.$column->getID().'/'); $column_items[] = id(new PhabricatorActionView()) - ->setIcon('fa-pencil') - ->setName(pht('Edit Column')) - ->setHref($edit_uri) - ->setDisabled(!$can_edit) - ->setWorkflow(!$can_edit); + ->setIcon('fa-columns') + ->setName(pht('Column Details')) + ->setHref($detail_uri); $can_hide = ($can_edit && !$column->isDefaultColumn()); $hide_uri = 'board/'.$this->id.'/hide/'.$column->getID().'/'; diff --git a/src/applications/project/controller/PhabricatorProjectColumnDetailController.php b/src/applications/project/controller/PhabricatorProjectColumnDetailController.php index c44fb5b87c..bf0b659859 100644 --- a/src/applications/project/controller/PhabricatorProjectColumnDetailController.php +++ b/src/applications/project/controller/PhabricatorProjectColumnDetailController.php @@ -17,7 +17,6 @@ final class PhabricatorProjectColumnDetailController ->withIDs(array($project_id)) ->needImages(true) ->executeOne(); - if (!$project) { return new Aphront404Response(); } @@ -40,7 +39,7 @@ final class PhabricatorProjectColumnDetailController new PhabricatorProjectColumnTransactionQuery()); $timeline->setShouldTerminate(true); - $title = pht('%s', $column->getDisplayName()); + $title = $column->getDisplayName(); $header = $this->buildHeaderView($column); $actions = $this->buildActionView($column); @@ -113,15 +112,6 @@ final class PhabricatorProjectColumnDetailController ->setObject($column) ->setActionList($actions); - $descriptions = PhabricatorPolicyQuery::renderPolicyDescriptions( - $viewer, - $column); - - $properties->addProperty( - pht('Editable By'), - $descriptions[PhabricatorPolicyCapability::CAN_EDIT]); - - $limit = $column->getPointLimit(); $properties->addProperty( pht('Point Limit'), From bce0698a0f5dd20c003f3089abe4c9cba2f71434 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 31 Aug 2015 10:17:54 -0700 Subject: [PATCH 13/35] Modernize Audit search engine Summary: Fixes T9279. Modernizes the SearchEngine and Query classes. User-facing changes: - Added order by commit date, default to order by commit date with newest commits first. - Added explicit "Needs Audit by". - Added new `packages(...)` typeahead function. - Picked up automatic subscribers, projects, and order fields. This changes behavior a little bit: we previously attempted to exclude, e.g., commits which a package you own needs to audit, but which you have resigned from. This is difficult in general and I think it needs a more comprehensive solution. This shouldn't impact users much, anyway. Test Plan: {F767628} Reviewers: chad Reviewed By: chad Maniphest Tasks: T9279 Differential Revision: https://secure.phabricator.com/D14013 --- src/__phutil_library_map__.php | 10 + .../PhabricatorAuditApplication.php | 3 +- .../query/PhabricatorCommitSearchEngine.php | 148 +++++------ .../diffusion/query/DiffusionCommitQuery.php | 234 ++++++++++-------- .../DiffusionAuditorFunctionDatasource.php | 25 ++ .../PhabricatorHomeMainController.php | 3 +- ...ricatorOwnersPackageFunctionDatasource.php | 25 ++ ...habricatorOwnersPackageOwnerDatasource.php | 140 +++++++++++ ...ricatorProjectOrUserFunctionDatasource.php | 25 ++ ...abricatorProjectUserFunctionDatasource.php | 36 +++ .../storage/PhabricatorRepositoryCommit.php | 6 + 11 files changed, 449 insertions(+), 206 deletions(-) create mode 100644 src/applications/diffusion/typeahead/DiffusionAuditorFunctionDatasource.php create mode 100644 src/applications/owners/typeahead/PhabricatorOwnersPackageFunctionDatasource.php create mode 100644 src/applications/owners/typeahead/PhabricatorOwnersPackageOwnerDatasource.php create mode 100644 src/applications/project/typeahead/PhabricatorProjectOrUserFunctionDatasource.php create mode 100644 src/applications/project/typeahead/PhabricatorProjectUserFunctionDatasource.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a80ca41d9a..9388482854 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -496,6 +496,7 @@ phutil_register_library_map(array( 'DifferentialUpdateRevisionConduitAPIMethod' => 'applications/differential/conduit/DifferentialUpdateRevisionConduitAPIMethod.php', 'DifferentialViewPolicyField' => 'applications/differential/customfield/DifferentialViewPolicyField.php', 'DiffusionAuditorDatasource' => 'applications/diffusion/typeahead/DiffusionAuditorDatasource.php', + 'DiffusionAuditorFunctionDatasource' => 'applications/diffusion/typeahead/DiffusionAuditorFunctionDatasource.php', 'DiffusionAuditorsAddAuditorsHeraldAction' => 'applications/diffusion/herald/DiffusionAuditorsAddAuditorsHeraldAction.php', 'DiffusionAuditorsAddSelfHeraldAction' => 'applications/diffusion/herald/DiffusionAuditorsAddSelfHeraldAction.php', 'DiffusionAuditorsHeraldAction' => 'applications/diffusion/herald/DiffusionAuditorsHeraldAction.php', @@ -2436,6 +2437,8 @@ phutil_register_library_map(array( 'PhabricatorOwnersOwner' => 'applications/owners/storage/PhabricatorOwnersOwner.php', 'PhabricatorOwnersPackage' => 'applications/owners/storage/PhabricatorOwnersPackage.php', 'PhabricatorOwnersPackageDatasource' => 'applications/owners/typeahead/PhabricatorOwnersPackageDatasource.php', + 'PhabricatorOwnersPackageFunctionDatasource' => 'applications/owners/typeahead/PhabricatorOwnersPackageFunctionDatasource.php', + 'PhabricatorOwnersPackageOwnerDatasource' => 'applications/owners/typeahead/PhabricatorOwnersPackageOwnerDatasource.php', 'PhabricatorOwnersPackagePHIDType' => 'applications/owners/phid/PhabricatorOwnersPackagePHIDType.php', 'PhabricatorOwnersPackageQuery' => 'applications/owners/query/PhabricatorOwnersPackageQuery.php', 'PhabricatorOwnersPackageSearchEngine' => 'applications/owners/query/PhabricatorOwnersPackageSearchEngine.php', @@ -2632,6 +2635,7 @@ phutil_register_library_map(array( 'PhabricatorProjectNoProjectsDatasource' => 'applications/project/typeahead/PhabricatorProjectNoProjectsDatasource.php', 'PhabricatorProjectObjectHasProjectEdgeType' => 'applications/project/edge/PhabricatorProjectObjectHasProjectEdgeType.php', 'PhabricatorProjectOrUserDatasource' => 'applications/project/typeahead/PhabricatorProjectOrUserDatasource.php', + 'PhabricatorProjectOrUserFunctionDatasource' => 'applications/project/typeahead/PhabricatorProjectOrUserFunctionDatasource.php', 'PhabricatorProjectProfileController' => 'applications/project/controller/PhabricatorProjectProfileController.php', 'PhabricatorProjectProjectHasMemberEdgeType' => 'applications/project/edge/PhabricatorProjectProjectHasMemberEdgeType.php', 'PhabricatorProjectProjectHasObjectEdgeType' => 'applications/project/edge/PhabricatorProjectProjectHasObjectEdgeType.php', @@ -2651,6 +2655,7 @@ phutil_register_library_map(array( 'PhabricatorProjectTransactionQuery' => 'applications/project/query/PhabricatorProjectTransactionQuery.php', 'PhabricatorProjectUIEventListener' => 'applications/project/events/PhabricatorProjectUIEventListener.php', 'PhabricatorProjectUpdateController' => 'applications/project/controller/PhabricatorProjectUpdateController.php', + 'PhabricatorProjectUserFunctionDatasource' => 'applications/project/typeahead/PhabricatorProjectUserFunctionDatasource.php', 'PhabricatorProjectViewController' => 'applications/project/controller/PhabricatorProjectViewController.php', 'PhabricatorProjectWatchController' => 'applications/project/controller/PhabricatorProjectWatchController.php', 'PhabricatorProjectsPolicyRule' => 'applications/policy/rule/PhabricatorProjectsPolicyRule.php', @@ -4158,6 +4163,7 @@ phutil_register_library_map(array( 'DifferentialUpdateRevisionConduitAPIMethod' => 'DifferentialConduitAPIMethod', 'DifferentialViewPolicyField' => 'DifferentialCoreCustomField', 'DiffusionAuditorDatasource' => 'PhabricatorTypeaheadCompositeDatasource', + 'DiffusionAuditorFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'DiffusionAuditorsAddAuditorsHeraldAction' => 'DiffusionAuditorsHeraldAction', 'DiffusionAuditorsAddSelfHeraldAction' => 'DiffusionAuditorsHeraldAction', 'DiffusionAuditorsHeraldAction' => 'HeraldAction', @@ -6407,6 +6413,8 @@ phutil_register_library_map(array( 'PhabricatorApplicationTransactionInterface', ), 'PhabricatorOwnersPackageDatasource' => 'PhabricatorTypeaheadDatasource', + 'PhabricatorOwnersPackageFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource', + 'PhabricatorOwnersPackageOwnerDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'PhabricatorOwnersPackagePHIDType' => 'PhabricatorPHIDType', 'PhabricatorOwnersPackageQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorOwnersPackageSearchEngine' => 'PhabricatorApplicationSearchEngine', @@ -6650,6 +6658,7 @@ phutil_register_library_map(array( 'PhabricatorProjectNoProjectsDatasource' => 'PhabricatorTypeaheadDatasource', 'PhabricatorProjectObjectHasProjectEdgeType' => 'PhabricatorEdgeType', 'PhabricatorProjectOrUserDatasource' => 'PhabricatorTypeaheadCompositeDatasource', + 'PhabricatorProjectOrUserFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'PhabricatorProjectProfileController' => 'PhabricatorProjectController', 'PhabricatorProjectProjectHasMemberEdgeType' => 'PhabricatorEdgeType', 'PhabricatorProjectProjectHasObjectEdgeType' => 'PhabricatorEdgeType', @@ -6672,6 +6681,7 @@ phutil_register_library_map(array( 'PhabricatorProjectTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'PhabricatorProjectUIEventListener' => 'PhabricatorEventListener', 'PhabricatorProjectUpdateController' => 'PhabricatorProjectController', + 'PhabricatorProjectUserFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'PhabricatorProjectViewController' => 'PhabricatorProjectController', 'PhabricatorProjectWatchController' => 'PhabricatorProjectController', 'PhabricatorProjectsPolicyRule' => 'PhabricatorPolicyRule', diff --git a/src/applications/audit/application/PhabricatorAuditApplication.php b/src/applications/audit/application/PhabricatorAuditApplication.php index e186215c96..858993e5aa 100644 --- a/src/applications/audit/application/PhabricatorAuditApplication.php +++ b/src/applications/audit/application/PhabricatorAuditApplication.php @@ -70,9 +70,8 @@ final class PhabricatorAuditApplication extends PhabricatorApplication { $query = id(new DiffusionCommitQuery()) ->setViewer($user) - ->withAuditorPHIDs($phids) + ->withNeedsAuditByPHIDs($phids) ->withAuditStatus(DiffusionCommitQuery::AUDIT_STATUS_OPEN) - ->withAuditAwaitingUser($user) ->setLimit(self::MAX_STATUS_ITEMS); $commits = $query->execute(); diff --git a/src/applications/audit/query/PhabricatorCommitSearchEngine.php b/src/applications/audit/query/PhabricatorCommitSearchEngine.php index 97b3a942e4..af0194bc79 100644 --- a/src/applications/audit/query/PhabricatorCommitSearchEngine.php +++ b/src/applications/audit/query/PhabricatorCommitSearchEngine.php @@ -11,103 +11,65 @@ final class PhabricatorCommitSearchEngine return 'PhabricatorDiffusionApplication'; } - public function buildSavedQueryFromRequest(AphrontRequest $request) { - $saved = new PhabricatorSavedQuery(); - - $saved->setParameter( - 'auditorPHIDs', - $this->readPHIDsFromRequest($request, 'auditorPHIDs')); - - $saved->setParameter( - 'commitAuthorPHIDs', - $this->readUsersFromRequest($request, 'authors')); - - $saved->setParameter( - 'auditStatus', - $request->getStr('auditStatus')); - - $saved->setParameter( - 'repositoryPHIDs', - $this->readPHIDsFromRequest($request, 'repositoryPHIDs')); - - // -- TODO - T4173 - file location - - return $saved; - } - - public function buildQueryFromSavedQuery(PhabricatorSavedQuery $saved) { - $query = id(new DiffusionCommitQuery()) + public function newQuery() { + return id(new DiffusionCommitQuery()) ->needAuditRequests(true) ->needCommitData(true); + } - $auditor_phids = $saved->getParameter('auditorPHIDs', array()); - if ($auditor_phids) { - $query->withAuditorPHIDs($auditor_phids); + protected function buildQueryFromParameters(array $map) { + $query = $this->newQuery(); + + if ($map['needsAuditByPHIDs']) { + $query->withNeedsAuditByPHIDs($map['needsAuditByPHIDs']); } - $commit_author_phids = $saved->getParameter('commitAuthorPHIDs', array()); - if ($commit_author_phids) { - $query->withAuthorPHIDs($commit_author_phids); + if ($map['auditorPHIDs']) { + $query->withAuditorPHIDs($map['auditorPHIDs']); } - $audit_status = $saved->getParameter('auditStatus', null); - if ($audit_status) { - $query->withAuditStatus($audit_status); + if ($map['commitAuthorPHIDs']) { + $query->withAuthorPHIDs($map['commitAuthorPHIDs']); } - $awaiting_user_phid = $saved->getParameter('awaitingUserPHID', null); - if ($awaiting_user_phid) { - // This is used only for the built-in "needs attention" filter, - // so cheat and just use the already-loaded viewer rather than reloading - // it. - $query->withAuditAwaitingUser($this->requireViewer()); + if ($map['auditStatus']) { + $query->withAuditStatus($map['auditStatus']); } - $repository_phids = $saved->getParameter('repositoryPHIDs', array()); - if ($repository_phids) { - $query->withRepositoryPHIDs($repository_phids); + if ($map['repositoryPHIDs']) { + $query->withRepositoryPHIDs($map['repositoryPHIDs']); } return $query; } - public function buildSearchForm( - AphrontFormView $form, - PhabricatorSavedQuery $saved) { - - $auditor_phids = $saved->getParameter('auditorPHIDs', array()); - $commit_author_phids = $saved->getParameter( - 'commitAuthorPHIDs', - array()); - $audit_status = $saved->getParameter('auditStatus', null); - $repository_phids = $saved->getParameter('repositoryPHIDs', array()); - - $form - ->appendControl( - id(new AphrontFormTokenizerControl()) - ->setDatasource(new DiffusionAuditorDatasource()) - ->setName('auditorPHIDs') - ->setLabel(pht('Auditors')) - ->setValue($auditor_phids)) - ->appendControl( - id(new AphrontFormTokenizerControl()) - ->setDatasource(new PhabricatorPeopleDatasource()) - ->setName('authors') - ->setLabel(pht('Commit Authors')) - ->setValue($commit_author_phids)) - ->appendChild( - id(new AphrontFormSelectControl()) - ->setName('auditStatus') - ->setLabel(pht('Audit Status')) - ->setOptions($this->getAuditStatusOptions()) - ->setValue($audit_status)) - ->appendControl( - id(new AphrontFormTokenizerControl()) - ->setLabel(pht('Repositories')) - ->setName('repositoryPHIDs') - ->setDatasource(new DiffusionRepositoryDatasource()) - ->setValue($repository_phids)); - + protected function buildCustomSearchFields() { + return array( + id(new PhabricatorSearchDatasourceField()) + ->setLabel(pht('Needs Audit By')) + ->setKey('needsAuditByPHIDs') + ->setAliases(array('needs', 'need')) + ->setDatasource(new DiffusionAuditorFunctionDatasource()), + id(new PhabricatorSearchDatasourceField()) + ->setLabel(pht('Auditors')) + ->setKey('auditorPHIDs') + ->setAliases(array('auditor', 'auditors')) + ->setDatasource(new DiffusionAuditorFunctionDatasource()), + id(new PhabricatorUsersSearchField()) + ->setLabel(pht('Authors')) + ->setKey('commitAuthorPHIDs') + ->setAliases(array('author', 'authors')), + id(new PhabricatorSearchSelectField()) + ->setLabel(pht('Audit Status')) + ->setKey('auditStatus') + ->setAliases(array('status')) + ->setOptions($this->getAuditStatusOptions()), + id(new PhabricatorSearchDatasourceField()) + ->setLabel(pht('Repositories')) + ->setKey('repositoryPHIDs') + ->setAliases(array('repository', 'repositories')) + ->setDatasource(new DiffusionRepositoryDatasource()), + ); } protected function getURI($path) { @@ -118,7 +80,7 @@ final class PhabricatorCommitSearchEngine $names = array(); if ($this->requireViewer()->isLoggedIn()) { - $names['need'] = pht('Need Attention'); + $names['need'] = pht('Needs Audit'); $names['problem'] = pht('Problem Commits'); } @@ -138,22 +100,24 @@ final class PhabricatorCommitSearchEngine $query->setQueryKey($query_key); $viewer = $this->requireViewer(); + $viewer_phid = $viewer->getPHID(); + $status_open = DiffusionCommitQuery::AUDIT_STATUS_OPEN; + switch ($query_key) { case 'all': return $query; case 'open': - $query->setParameter( - 'auditStatus', - DiffusionCommitQuery::AUDIT_STATUS_OPEN); + $query->setParameter('auditStatus', $status_open); return $query; case 'need': - $query->setParameter('awaitingUserPHID', $viewer->getPHID()); - $query->setParameter( - 'auditStatus', - DiffusionCommitQuery::AUDIT_STATUS_OPEN); - $query->setParameter( - 'auditorPHIDs', - PhabricatorAuditCommentEditor::loadAuditPHIDsForUser($viewer)); + $needs_tokens = array( + $viewer_phid, + 'projects('.$viewer_phid.')', + 'packages('.$viewer_phid.')', + ); + + $query->setParameter('needsAuditByPHIDs', $needs_tokens); + $query->setParameter('auditStatus', $status_open); return $query; case 'authored': $query->setParameter('commitAuthorPHIDs', array($viewer->getPHID())); diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php index 45f8b063af..5d6b2623b5 100644 --- a/src/applications/diffusion/query/DiffusionCommitQuery.php +++ b/src/applications/diffusion/query/DiffusionCommitQuery.php @@ -15,7 +15,7 @@ final class DiffusionCommitQuery private $needAuditRequests; private $auditIDs; private $auditorPHIDs; - private $auditAwaitingUser; + private $needsAuditByPHIDs; private $auditStatus; private $epochMin; private $epochMax; @@ -103,27 +103,6 @@ final class DiffusionCommitQuery return $this; } - /** - * Returns true if we should join the audit table, either because we're - * interested in the information if it's available or because matching rows - * must always have it. - */ - private function shouldJoinAudits() { - return $this->auditStatus || - $this->rowsMustHaveAudits(); - } - - /** - * Return true if we should `JOIN` (vs `LEFT JOIN`) the audit table, because - * matching commits will always have audit rows. - */ - private function rowsMustHaveAudits() { - return - $this->auditIDs || - $this->auditorPHIDs || - $this->auditAwaitingUser; - } - public function withAuditIDs(array $ids) { $this->auditIDs = $ids; return $this; @@ -134,8 +113,8 @@ final class DiffusionCommitQuery return $this; } - public function withAuditAwaitingUser(PhabricatorUser $user) { - $this->auditAwaitingUser = $user; + public function withNeedsAuditByPHIDs(array $needs_phids) { + $this->needsAuditByPHIDs = $needs_phids; return $this; } @@ -175,21 +154,12 @@ final class DiffusionCommitQuery } } + public function newResultObject() { + return new PhabricatorRepositoryCommit(); + } + protected function loadPage() { - $table = new PhabricatorRepositoryCommit(); - $conn_r = $table->establishConnection('r'); - - $data = queryfx_all( - $conn_r, - 'SELECT commit.* FROM %T commit %Q %Q %Q %Q %Q', - $table->getTableName(), - $this->buildJoinClause($conn_r), - $this->buildWhereClause($conn_r), - $this->buildGroupClause($conn_r), - $this->buildOrderClause($conn_r), - $this->buildLimitClause($conn_r)); - - return $table->loadAllFromArray($data); + return $this->loadStandardPage($this->newResultObject()); } protected function willFilterPage(array $commits) { @@ -296,8 +266,8 @@ final class DiffusionCommitQuery return $commits; } - protected function buildWhereClause(AphrontDatabaseConnection $conn_r) { - $where = array(); + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); if ($this->repositoryPHIDs !== null) { $map_repositories = id(new PhabricatorRepositoryQuery()) @@ -317,42 +287,42 @@ final class DiffusionCommitQuery if ($this->ids !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'commit.id IN (%Ld)', $this->ids); } if ($this->phids !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'commit.phid IN (%Ls)', $this->phids); } if ($this->repositoryIDs !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'commit.repositoryID IN (%Ld)', $this->repositoryIDs); } if ($this->authorPHIDs !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'commit.authorPHID IN (%Ls)', $this->authorPHIDs); } if ($this->epochMin !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'commit.epoch >= %d', $this->epochMin); } if ($this->epochMax !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'commit.epoch <= %d', $this->epochMax); } @@ -360,13 +330,13 @@ final class DiffusionCommitQuery if ($this->importing !== null) { if ($this->importing) { $where[] = qsprintf( - $conn_r, + $conn, '(commit.importStatus & %d) != %d', PhabricatorRepositoryCommit::IMPORTED_ALL, PhabricatorRepositoryCommit::IMPORTED_ALL); } else { $where[] = qsprintf( - $conn_r, + $conn, '(commit.importStatus & %d) = %d', PhabricatorRepositoryCommit::IMPORTED_ALL, PhabricatorRepositoryCommit::IMPORTED_ALL); @@ -409,7 +379,7 @@ final class DiffusionCommitQuery foreach ($bare as $identifier) { $sql[] = qsprintf( - $conn_r, + $conn, '(commit.commitIdentifier LIKE %> AND '. 'LENGTH(commit.commitIdentifier) = 40)', $identifier); @@ -437,7 +407,7 @@ final class DiffusionCommitQuery continue; } $sql[] = qsprintf( - $conn_r, + $conn, '(commit.repositoryID = %d AND commit.commitIdentifier = %s)', $repo->getID(), // NOTE: Because the 'commitIdentifier' column is a string, MySQL @@ -449,7 +419,7 @@ final class DiffusionCommitQuery continue; } $sql[] = qsprintf( - $conn_r, + $conn, '(commit.repositoryID = %d AND commit.commitIdentifier LIKE %>)', $repo->getID(), $ref['identifier']); @@ -470,28 +440,23 @@ final class DiffusionCommitQuery if ($this->auditIDs !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'audit.id IN (%Ld)', $this->auditIDs); } if ($this->auditorPHIDs !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'audit.auditorPHID IN (%Ls)', $this->auditorPHIDs); } - if ($this->auditAwaitingUser) { - $awaiting_user_phid = $this->auditAwaitingUser->getPHID(); - // Exclude package and project audits associated with commits where - // the user is the author. + if ($this->needsAuditByPHIDs !== null) { $where[] = qsprintf( - $conn_r, - '(commit.authorPHID IS NULL OR commit.authorPHID != %s) - OR (audit.auditorPHID = %s)', - $awaiting_user_phid, - $awaiting_user_phid); + $conn, + 'needs.auditorPHID IN (%Ls)', + $this->needsAuditByPHIDs); } $status = $this->auditStatus; @@ -499,33 +464,27 @@ final class DiffusionCommitQuery switch ($status) { case self::AUDIT_STATUS_PARTIAL: $where[] = qsprintf( - $conn_r, + $conn, 'commit.auditStatus = %d', PhabricatorAuditCommitStatusConstants::PARTIALLY_AUDITED); break; case self::AUDIT_STATUS_ACCEPTED: $where[] = qsprintf( - $conn_r, + $conn, 'commit.auditStatus = %d', PhabricatorAuditCommitStatusConstants::FULLY_AUDITED); break; case self::AUDIT_STATUS_CONCERN: $where[] = qsprintf( - $conn_r, - 'audit.auditStatus = %s', + $conn, + 'status.auditStatus = %s', PhabricatorAuditStatusConstants::CONCERNED); break; case self::AUDIT_STATUS_OPEN: $where[] = qsprintf( - $conn_r, - 'audit.auditStatus in (%Ls)', + $conn, + 'status.auditStatus in (%Ls)', PhabricatorAuditStatusConstants::getOpenStatusConstants()); - if ($this->auditAwaitingUser) { - $where[] = qsprintf( - $conn_r, - 'awaiting.auditStatus IS NULL OR awaiting.auditStatus != %s', - PhabricatorAuditStatusConstants::RESIGNED); - } break; case self::AUDIT_STATUS_ANY: break; @@ -545,9 +504,7 @@ final class DiffusionCommitQuery } } - $where[] = $this->buildPagingClause($conn_r); - - return $this->formatWhereClause($where); + return $where; } protected function didFilterResults(array $filtered) { @@ -560,56 +517,113 @@ final class DiffusionCommitQuery } } - protected function buildJoinClause(AphrontDatabaseConnection $conn_r) { - $joins = array(); + private function shouldJoinStatus() { + return $this->auditStatus; + } + + private function shouldJoinAudits() { + return $this->auditIDs || $this->auditorPHIDs; + } + + private function shouldJoinNeeds() { + return $this->needsAuditByPHIDs; + } + + protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) { + $join = parent::buildJoinClauseParts($conn); $audit_request = new PhabricatorRepositoryAuditRequest(); - if ($this->shouldJoinAudits()) { - $joins[] = qsprintf( - $conn_r, - '%Q %T audit ON commit.phid = audit.commitPHID', - ($this->rowsMustHaveAudits() ? 'JOIN' : 'LEFT JOIN'), + if ($this->shouldJoinStatus()) { + $join[] = qsprintf( + $conn, + 'LEFT JOIN %T status ON commit.phid = status.commitPHID', $audit_request->getTableName()); } - if ($this->auditAwaitingUser) { - // Join the request table on the awaiting user's requests, so we can - // filter out package and project requests which the user has resigned - // from. - $joins[] = qsprintf( - $conn_r, - 'LEFT JOIN %T awaiting ON audit.commitPHID = awaiting.commitPHID AND - awaiting.auditorPHID = %s', - $audit_request->getTableName(), - $this->auditAwaitingUser->getPHID()); + if ($this->shouldJoinAudits()) { + $join[] = qsprintf( + $conn, + 'JOIN %T audit ON commit.phid = audit.commitPHID', + $audit_request->getTableName()); } - if ($joins) { - return implode(' ', $joins); - } else { - return ''; + if ($this->shouldJoinNeeds()) { + $join[] = qsprintf( + $conn, + 'JOIN %T needs ON commit.phid = needs.commitPHID + AND needs.auditStatus IN (%Ls)', + $audit_request->getTableName(), + array( + PhabricatorAuditStatusConstants::AUDIT_REQUESTED, + PhabricatorAuditStatusConstants::AUDIT_REQUIRED, + )); } + + return $join; } - protected function buildGroupClause(AphrontDatabaseConnection $conn_r) { - $should_group = $this->shouldJoinAudits(); - - // TODO: Currently, the audit table is missing a unique key, so we may - // require a GROUP BY if we perform this join. See T1768. This can be - // removed once the table has the key. - if ($this->auditAwaitingUser) { - $should_group = true; + protected function shouldGroupQueryResultRows() { + if ($this->shouldJoinStatus()) { + return true; } - if ($should_group) { - return 'GROUP BY commit.id'; - } else { - return ''; + if ($this->shouldJoinAudits()) { + return true; } + + if ($this->shouldJoinNeeds()) { + return true; + } + + return parent::shouldGroupQueryResultRows(); } public function getQueryApplicationClass() { return 'PhabricatorDiffusionApplication'; } + public function getOrderableColumns() { + return parent::getOrderableColumns() + array( + 'epoch' => array( + 'table' => $this->getPrimaryTableAlias(), + 'column' => 'epoch', + 'type' => 'int', + 'reverse' => false, + ), + ); + } + + protected function getPagingValueMap($cursor, array $keys) { + $commit = $this->loadCursorObject($cursor); + return array( + 'id' => $commit->getID(), + 'epoch' => $commit->getEpoch(), + ); + } + + public function getBuiltinOrders() { + $parent = parent::getBuiltinOrders(); + + // Rename the default ID-based orders. + $parent['importnew'] = array( + 'name' => pht('Import Date (Newest First)'), + ) + $parent['newest']; + + $parent['importold'] = array( + 'name' => pht('Import Date (Oldest First)'), + ) + $parent['oldest']; + + return array( + 'newest' => array( + 'vector' => array('epoch', 'id'), + 'name' => pht('Commit Date (Newest First)'), + ), + 'oldest' => array( + 'vector' => array('-epoch', '-id'), + 'name' => pht('Commit Date (Oldest First)'), + ), + ) + $parent; + } + + } diff --git a/src/applications/diffusion/typeahead/DiffusionAuditorFunctionDatasource.php b/src/applications/diffusion/typeahead/DiffusionAuditorFunctionDatasource.php new file mode 100644 index 0000000000..2bfc1ee037 --- /dev/null +++ b/src/applications/diffusion/typeahead/DiffusionAuditorFunctionDatasource.php @@ -0,0 +1,25 @@ +setViewer($user) - ->withAuditorPHIDs($phids) + ->withNeedsAuditByPHIDs($phids) ->withAuditStatus(DiffusionCommitQuery::AUDIT_STATUS_OPEN) - ->withAuditAwaitingUser($user) ->needAuditRequests(true) ->needCommitData(true) ->setLimit(10); diff --git a/src/applications/owners/typeahead/PhabricatorOwnersPackageFunctionDatasource.php b/src/applications/owners/typeahead/PhabricatorOwnersPackageFunctionDatasource.php new file mode 100644 index 0000000000..2ad0512ee2 --- /dev/null +++ b/src/applications/owners/typeahead/PhabricatorOwnersPackageFunctionDatasource.php @@ -0,0 +1,25 @@ +) or packages()...'); + } + + public function getDatasourceApplicationClass() { + return 'PhabricatorOwnersApplication'; + } + + public function getComponentDatasources() { + return array( + new PhabricatorPeopleDatasource(), + new PhabricatorProjectDatasource(), + ); + } + + public function getDatasourceFunctions() { + return array( + 'packages' => array( + 'name' => pht('Packages: ...'), + 'arguments' => pht('owner'), + 'summary' => pht("Find results in any of an owner's projects."), + 'description' => pht( + "This function allows you to find results associated with any ". + "of the packages a specified user or project is an owner of. ". + "For example, this will find results associated with all of ". + "the projects `%s` owns:\n\n%s\n\n", + 'alincoln', + '> packages(alincoln)'), + ), + ); + } + + protected function didLoadResults(array $results) { + foreach ($results as $result) { + $result + ->setColor(null) + ->setTokenType(PhabricatorTypeaheadTokenView::TYPE_FUNCTION) + ->setIcon('fa-asterisk') + ->setPHID('packages('.$result->getPHID().')') + ->setDisplayName(pht('Packages: %s', $result->getDisplayName())) + ->setName($result->getName().' packages'); + } + + return $results; + } + + protected function evaluateFunction($function, array $argv_list) { + $phids = array(); + foreach ($argv_list as $argv) { + $phids[] = head($argv); + } + + $phids = $this->resolvePHIDs($phids); + + $user_phids = array(); + foreach ($phids as $phid) { + if (phid_get_type($phid) == PhabricatorPeopleUserPHIDType::TYPECONST) { + $user_phids[] = $phid; + } + } + + if ($user_phids) { + $projects = id(new PhabricatorProjectQuery()) + ->setViewer($this->getViewer()) + ->withMemberPHIDs($user_phids) + ->execute(); + foreach ($projects as $project) { + $phids[] = $project->getPHID(); + } + } + + return $phids; + } + + public function renderFunctionTokens($function, array $argv_list) { + $phids = array(); + foreach ($argv_list as $argv) { + $phids[] = head($argv); + } + + $phids = $this->resolvePHIDs($phids); + + $tokens = $this->renderTokens($phids); + foreach ($tokens as $token) { + $token->setColor(null); + if ($token->isInvalid()) { + $token + ->setValue(pht('Packages: Invalid Owner')); + } else { + $token + ->setIcon('fa-asterisk') + ->setTokenType(PhabricatorTypeaheadTokenView::TYPE_FUNCTION) + ->setKey('packages('.$token->getKey().')') + ->setValue(pht('Packages: %s', $token->getValue())); + } + } + + return $tokens; + } + + private function resolvePHIDs(array $phids) { + + // TODO: It would be nice for this to handle `packages(#project)` from a + // query string or eventually via Conduit. This could also share code with + // PhabricatorProjectLogicalUserDatasource. + + $usernames = array(); + foreach ($phids as $key => $phid) { + if (phid_get_type($phid) != PhabricatorPeopleUserPHIDType::TYPECONST) { + $usernames[$key] = $phid; + } + } + + if ($usernames) { + $users = id(new PhabricatorPeopleQuery()) + ->setViewer($this->getViewer()) + ->withUsernames($usernames) + ->execute(); + $users = mpull($users, null, 'getUsername'); + foreach ($usernames as $key => $username) { + $user = idx($users, $username); + if ($user) { + $phids[$key] = $user->getPHID(); + } + } + } + + return $phids; + } + +} diff --git a/src/applications/project/typeahead/PhabricatorProjectOrUserFunctionDatasource.php b/src/applications/project/typeahead/PhabricatorProjectOrUserFunctionDatasource.php new file mode 100644 index 0000000000..6a6bc25ee0 --- /dev/null +++ b/src/applications/project/typeahead/PhabricatorProjectOrUserFunctionDatasource.php @@ -0,0 +1,25 @@ +)...'); + } + + public function getDatasourceApplicationClass() { + return 'PhabricatorProjectApplication'; + } + + public function getComponentDatasources() { + return array( + new PhabricatorProjectLogicalUserDatasource(), + ); + } + + protected function evaluateFunction($function, array $argv_list) { + $result = parent::evaluateFunction($function, $argv_list); + + foreach ($result as $k => $v) { + if ($v instanceof PhabricatorQueryConstraint) { + $result[$k] = $v->getValue(); + } + } + + return $result; + } + +} diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index 18328758ba..984e6325f7 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -98,6 +98,12 @@ final class PhabricatorRepositoryCommit 'columns' => array('commitIdentifier', 'repositoryID'), 'unique' => true, ), + 'key_epoch' => array( + 'columns' => array('epoch'), + ), + 'key_author' => array( + 'columns' => array('authorPHID', 'epoch'), + ), ), self::CONFIG_NO_MUTATE => array( 'importStatus', From 9692c1f2c24970bdb9789fe132b2e1aa9f869d9f Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Mon, 31 Aug 2015 14:05:51 -0700 Subject: [PATCH 14/35] Quickly fix phpqrcode syntax Summary: phpqrcode has some old looking php syntax. Fix it quickly since it's one line. Test Plan: Before this patch, went to add a TOTP token, saw the error about the undefined variable. After this patch, successfully added a TOTP token, and used it. Reviewers: avivey, epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Maniphest Tasks: T9300 Differential Revision: https://secure.phabricator.com/D14019 --- externals/phpqrcode/phpqrcode.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/externals/phpqrcode/phpqrcode.php b/externals/phpqrcode/phpqrcode.php index 80adb9df23..54cde3ee9e 100644 --- a/externals/phpqrcode/phpqrcode.php +++ b/externals/phpqrcode/phpqrcode.php @@ -2937,7 +2937,7 @@ //---------------------------------------------------------------------- public function getCode() { - $ret; + $ret = null; if($this->count < $this->dataLength) { $row = $this->count % $this->blocks; From 9159c0e0017de6f2f2fcb9687b456c5855865d1e Mon Sep 17 00:00:00 2001 From: Chad Little Date: Mon, 31 Aug 2015 14:51:51 -0700 Subject: [PATCH 15/35] Update bar colors to match icon colors Summary: Fixes T8901 by adding in additional colors used by icons. Plus fire. Fire is cool. Test Plan: Try out new colors in maniphest priorities. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T8901 Differential Revision: https://secure.phabricator.com/D14020 --- resources/celerity/map.php | 10 +++---- webroot/rsrc/css/font/phui-font-icon-base.css | 3 +++ .../css/phui/phui-object-item-list-view.css | 26 ++++++++++++++++--- 3 files changed, 30 insertions(+), 9 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 4cb738d0f0..7e000ecb6a 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -7,7 +7,7 @@ */ return array( 'names' => array( - 'core.pkg.css' => 'aced76a5', + 'core.pkg.css' => '928faf7e', 'core.pkg.js' => 'a590b451', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '2de124c9', @@ -111,7 +111,7 @@ return array( 'rsrc/css/font/font-awesome.css' => 'd2fc4e8d', 'rsrc/css/font/font-lato.css' => '5ab1a46a', 'rsrc/css/font/font-roboto-slab.css' => 'f24a53cb', - 'rsrc/css/font/phui-font-icon-base.css' => '3dad2ae3', + 'rsrc/css/font/phui-font-icon-base.css' => 'ecbbb4c2', 'rsrc/css/layout/phabricator-filetree-view.css' => 'fccf9f82', 'rsrc/css/layout/phabricator-hovercard-view.css' => '1239cd52', 'rsrc/css/layout/phabricator-side-menu-view.css' => 'bec2458e', @@ -138,7 +138,7 @@ return array( 'rsrc/css/phui/phui-info-view.css' => '5b16bac6', 'rsrc/css/phui/phui-list.css' => '125599df', 'rsrc/css/phui/phui-object-box.css' => '407eaf5a', - 'rsrc/css/phui/phui-object-item-list-view.css' => '36ce366c', + 'rsrc/css/phui/phui-object-item-list-view.css' => 'ab1bf393', 'rsrc/css/phui/phui-pager.css' => 'bea33d23', 'rsrc/css/phui/phui-pinboard-view.css' => '2495140e', 'rsrc/css/phui/phui-property-list-view.css' => '15bbe0b0', @@ -779,7 +779,7 @@ return array( 'phui-crumbs-view-css' => 'd842f867', 'phui-document-view-css' => '0267054b', 'phui-feed-story-css' => 'b7b26d23', - 'phui-font-icon-base-css' => '3dad2ae3', + 'phui-font-icon-base-css' => 'ecbbb4c2', 'phui-fontkit-css' => 'cb8ae7ad', 'phui-form-css' => 'afdb2c6e', 'phui-form-view-css' => '621b21c5', @@ -791,7 +791,7 @@ return array( 'phui-inline-comment-view-css' => '0fdb3667', 'phui-list-view-css' => '125599df', 'phui-object-box-css' => '407eaf5a', - 'phui-object-item-list-view-css' => '36ce366c', + 'phui-object-item-list-view-css' => 'ab1bf393', 'phui-pager-css' => 'bea33d23', 'phui-pinboard-view-css' => '2495140e', 'phui-property-list-view-css' => '15bbe0b0', diff --git a/webroot/rsrc/css/font/phui-font-icon-base.css b/webroot/rsrc/css/font/phui-font-icon-base.css index 2ce692e57c..2ddd423d3c 100644 --- a/webroot/rsrc/css/font/phui-font-icon-base.css +++ b/webroot/rsrc/css/font/phui-font-icon-base.css @@ -137,6 +137,9 @@ .phui-icon-view.pink { color: {$pink}; } +.phui-icon-view.fire { + color: {$fire}; +} .phui-icon-view.violet { color: {$violet}; } diff --git a/webroot/rsrc/css/phui/phui-object-item-list-view.css b/webroot/rsrc/css/phui/phui-object-item-list-view.css index fe965b1b00..ecaaa32f70 100644 --- a/webroot/rsrc/css/phui/phui-object-item-list-view.css +++ b/webroot/rsrc/css/phui/phui-object-item-list-view.css @@ -416,12 +416,30 @@ ul.phui-object-item-icons { border-left-color: {$violet}; } -.phui-object-item-bar-color-grey { - border-left-color: #bdc3c7; +.phui-object-item-bar-color-pink { + border-left-color: {$pink}; } -.phui-object-item-bar-color-black { - border-left-color: #333333; +.phui-object-item-bar-color-fire { + border-left-color: {$fire}; +} + +.phui-object-item-bar-color-bluegrey { + border-left-color: {$bluetext}; +} + +.phui-object-item-bar-color-lightbluetext { + border-left-color: {$lightbluetext}; +} + +.phui-object-item-bar-color-grey, +.phui-object-item-bar-color-lightgreytext { + border-left-color: {$lightgreytext}; +} + +.phui-object-item-bar-color-black, +.phui-object-item-bar-color-dark { + border-left-color: {$darkgreytext}; } From fe66d52a22a5957efc24af6bf21f0a94db504ae0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 31 Aug 2015 15:52:04 -0700 Subject: [PATCH 16/35] Don't copy `null` attributes passed to JX.$N() Summary: Fixes T8919. In Safari, `node.href = null;` has no effect, but in Chrome it is like `node.href = "null";`. Instead, just use semantics similar to `phutil_tag()`: don't assign attributes with `null` values. Test Plan: No more `/null` href in Chrome in Owners typehaead. Typeahead still works in Chrome/Safari. Reviewers: chad Reviewed By: chad Maniphest Tasks: T8919 Differential Revision: https://secure.phabricator.com/D14021 --- webroot/rsrc/externals/javelin/lib/DOM.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/webroot/rsrc/externals/javelin/lib/DOM.js b/webroot/rsrc/externals/javelin/lib/DOM.js index c131d107cb..189d778ad5 100644 --- a/webroot/rsrc/externals/javelin/lib/DOM.js +++ b/webroot/rsrc/externals/javelin/lib/DOM.js @@ -310,7 +310,13 @@ JX.$N = function(tag, attr, content) { } } - JX.copy(node, attr); + for (var k in attr) { + if (attr[k] === null) { + continue; + } + node[k] = attr[k]; + } + if (content) { JX.DOM.setContent(node, content); } From ce7c2097b216fb372e96402f9c22761eeb5e7f41 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 31 Aug 2015 16:01:01 -0700 Subject: [PATCH 17/35] Update Owners docs a bit Summary: Fixes T9218. Fixes T8320. Fixes T8661. This isn't exhaustive but documents the stuff that cropped up in this iteration as needing documentation. In particular: - Be explicit about multiple ownership. - Explain value of having one place to update your giant regexp of a trillion paths. Test Plan: Read documentation. Reviewers: chad Reviewed By: chad Maniphest Tasks: T8320, T8661, T9218 Differential Revision: https://secure.phabricator.com/D14023 --- .../PhabricatorOwnersApplication.php | 2 +- src/docs/user/userguide/owners.diviner | 80 ++++++++++++++----- 2 files changed, 61 insertions(+), 21 deletions(-) diff --git a/src/applications/owners/application/PhabricatorOwnersApplication.php b/src/applications/owners/application/PhabricatorOwnersApplication.php index 3ed14212dc..79f63b26b8 100644 --- a/src/applications/owners/application/PhabricatorOwnersApplication.php +++ b/src/applications/owners/application/PhabricatorOwnersApplication.php @@ -26,7 +26,7 @@ final class PhabricatorOwnersApplication extends PhabricatorApplication { return array( array( 'name' => pht('Owners User Guide'), - 'href' => PhabricatorEnv::getDoclink('Owners Tool User Guide'), + 'href' => PhabricatorEnv::getDoclink('Owners User Guide'), ), ); } diff --git a/src/docs/user/userguide/owners.diviner b/src/docs/user/userguide/owners.diviner index 4d4095fc39..9347ca9d42 100644 --- a/src/docs/user/userguide/owners.diviner +++ b/src/docs/user/userguide/owners.diviner @@ -1,30 +1,70 @@ -@title Owners Tool User Guide +@title Owners User Guide @group userguide -Use Owners to define and/or monitor code you care about. +Group files in a codebase into packages and define ownership. -= Packages = +Overview +======== -Owners tool allows you to define a code package by specifying a group of paths. -The package can then be used to monitor the paths. For example, it can be used -in Herald rules and in the "Related Commits" feature (see below). +The Owners application allows you to group files in a codebase (or across +codebases) into packages. This can make it easier to reference a module or +subsystem in other applications, like Herald. -= Related Commits = -Once the package is defined, all future commits touching any path defined in -the package will be recorded as "Related Commits" of the package. +Creating a Package +================== -= Commits Needing Attention = +To create a package, choose a name and add some files which belong to the +package. For example, you might define an "iOS Application" package by +including these paths: -Owners tool enables the owners of the package to monitor the commits that might -need attention. If "auditing" is enabled for a package, a related commit will -be marked as "Needing Attention" if + /conf/ios/ + /src/ios/ + /shared/assets/mobile/ - - it's neither authored nor reviewed by an owner of the package, - - no revision found for the commit, - - the commit author is not recognized, or - - the author or the reviewer specified in the commits don't match the ones in - the Differential revision +Any files in those directories are considered to be part of the package, and +you can now conveniently refer to them (for example, in a Herald rule) by +refering to the package instead of copy/pasting a huge regular expression +into a bunch of places. -The owners of the package can accept or specify concern for such commits by -clicking the "Audit Status" link. +If new source files are later added, or the scope of the package otherwise +expands or contracts, you can edit the package definition to keep things +updated. + +You can use "exclude" paths to ignore subdirectories which would otherwise +be considered part of the package. For example, you might exclude a path +like this: + + /conf/ios/generated/ + +Perhaps that directory contains some generated configuration which frequently +changes, and which you aren't concerned about. + +After creating a package, files the package contains will be identified as +belonging to the package when you look at them in Diffusion, or look at changes +which affect them in Diffusion or Differential. + + +Files in Multiple Packages +========================== + +Multiple packages may own the same file. For example, both the +"Android Application" and the "iOS Application" packages might own a path +like this, containing resources used by both: + + /shared/assets/mobile/ + +If both packages own this directory, files in the directory are considered to +be part of both packages. + +Packages do not need to have claims of equal specificity to own files. For +example, if you have a "Design Assets" package which owns this path: + + /shared/assets/ + +...it will //also// own all of the files in the `mobile/` subdirectory. In this +configuration, these files are part of three packages: "iOS Application", +"Android Application", and "Design Assets". + +(You can use an "exclude" rule if you want to make a different package with a +more specific claim the owner of a file or subdirectory.) From 809c7fb4f35ea6a204dd70f8ac48f8044f801a24 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 1 Sep 2015 08:07:06 -0700 Subject: [PATCH 18/35] Fix an issue where paths could bleed across repos in Owners Summary: Ref T8320. I missed this a while ago and then it came to me in a dream. Only consider paths in the same repo when looking at ownership. (I think this is rarely reachable in practice.) Test Plan: Verified that files and commits still listed ownership properly. Reviewers: chad Reviewed By: chad Maniphest Tasks: T8320 Differential Revision: https://secure.phabricator.com/D14022 --- .../owners/query/PhabricatorOwnersPackageQuery.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/applications/owners/query/PhabricatorOwnersPackageQuery.php b/src/applications/owners/query/PhabricatorOwnersPackageQuery.php index f57ddf5a58..ef6a1e1a67 100644 --- a/src/applications/owners/query/PhabricatorOwnersPackageQuery.php +++ b/src/applications/owners/query/PhabricatorOwnersPackageQuery.php @@ -372,6 +372,11 @@ final class PhabricatorOwnersPackageQuery $include = false; foreach ($package->getPaths() as $package_path) { + if ($package_path->getRepositoryPHID() != $repository_phid) { + // If this path is for some other repository, skip it. + continue; + } + $strength = $package_path->getPathMatchStrength($path); if ($strength > $best_match) { $best_match = $strength; From 13516cf35f0369632814e4e14a66403cdaa4eda8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 1 Sep 2015 15:35:25 -0700 Subject: [PATCH 19/35] Fix an issue with "packages(...)" in typeaheads Summary: Fixes T9302. This datasource wasn't resolving package PHIDs correctly for the actual query. Also fixes an issue with the "Affected packages that need audit" Herald rule. Test Plan: Ran a "Needs Audit" query with only packages, and only `packages(user)`. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9302 Differential Revision: https://secure.phabricator.com/D14029 --- resources/celerity/map.php | 20 +++++++++---------- .../diffusion/herald/HeraldCommitAdapter.php | 4 +--- ...habricatorOwnersPackageOwnerDatasource.php | 11 +++++----- 3 files changed, 17 insertions(+), 18 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 7e000ecb6a..c600d8d7bd 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -8,7 +8,7 @@ return array( 'names' => array( 'core.pkg.css' => '928faf7e', - 'core.pkg.js' => 'a590b451', + 'core.pkg.js' => '47dc9ebb', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '2de124c9', 'differential.pkg.js' => '813c1633', @@ -210,7 +210,7 @@ return array( 'rsrc/externals/javelin/ext/view/__tests__/ViewInterpreter.js' => '7a94d6a5', 'rsrc/externals/javelin/ext/view/__tests__/ViewRenderer.js' => '6ea96ac9', 'rsrc/externals/javelin/lib/Cookie.js' => '62dfea03', - 'rsrc/externals/javelin/lib/DOM.js' => '147805fa', + 'rsrc/externals/javelin/lib/DOM.js' => '805b806a', 'rsrc/externals/javelin/lib/History.js' => 'd4505101', 'rsrc/externals/javelin/lib/JSON.js' => '69adf288', 'rsrc/externals/javelin/lib/Leader.js' => '331b1611', @@ -659,7 +659,7 @@ return array( 'javelin-color' => '7e41274a', 'javelin-cookie' => '62dfea03', 'javelin-diffusion-locate-file-source' => 'b42eddc7', - 'javelin-dom' => '147805fa', + 'javelin-dom' => '805b806a', 'javelin-dynval' => 'f6555212', 'javelin-event' => '85ea0626', 'javelin-fx' => '54b612ba', @@ -918,13 +918,6 @@ return array( 'javelin-uri', 'phabricator-textareautils', ), - '147805fa' => array( - 'javelin-magical-init', - 'javelin-install', - 'javelin-util', - 'javelin-vector', - 'javelin-stratcom', - ), '1499a8cb' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1451,6 +1444,13 @@ return array( 'javelin-behavior', 'javelin-history', ), + '805b806a' => array( + 'javelin-magical-init', + 'javelin-install', + 'javelin-util', + 'javelin-vector', + 'javelin-stratcom', + ), 82439934 => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/applications/diffusion/herald/HeraldCommitAdapter.php b/src/applications/diffusion/herald/HeraldCommitAdapter.php index ab3e850475..ed82e90820 100644 --- a/src/applications/diffusion/herald/HeraldCommitAdapter.php +++ b/src/applications/diffusion/herald/HeraldCommitAdapter.php @@ -168,9 +168,7 @@ final class HeraldCommitAdapter 'commitPHID = %s AND auditStatus IN (%Ls)', $this->commit->getPHID(), $status_arr); - - $packages = mpull($requests, 'getAuditorPHID'); - $this->auditNeededPackages = $packages; + $this->auditNeededPackages = $requests; } return $this->auditNeededPackages; } diff --git a/src/applications/owners/typeahead/PhabricatorOwnersPackageOwnerDatasource.php b/src/applications/owners/typeahead/PhabricatorOwnersPackageOwnerDatasource.php index a9aa80ef48..99e770d266 100644 --- a/src/applications/owners/typeahead/PhabricatorOwnersPackageOwnerDatasource.php +++ b/src/applications/owners/typeahead/PhabricatorOwnersPackageOwnerDatasource.php @@ -62,19 +62,20 @@ final class PhabricatorOwnersPackageOwnerDatasource $phids = $this->resolvePHIDs($phids); $user_phids = array(); - foreach ($phids as $phid) { + foreach ($phids as $key => $phid) { if (phid_get_type($phid) == PhabricatorPeopleUserPHIDType::TYPECONST) { $user_phids[] = $phid; + unset($phids[$key]); } } if ($user_phids) { - $projects = id(new PhabricatorProjectQuery()) + $packages = id(new PhabricatorOwnersPackageQuery()) ->setViewer($this->getViewer()) - ->withMemberPHIDs($user_phids) + ->withOwnerPHIDs($user_phids) ->execute(); - foreach ($projects as $project) { - $phids[] = $project->getPHID(); + foreach ($packages as $package) { + $phids[] = $package->getPHID(); } } From 29948eaa5bd2e2865a4b580d2c84225620fc1844 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 1 Sep 2015 15:52:44 -0700 Subject: [PATCH 20/35] Use phutil_hashes_are_identical() when comparing hashes in Phabricator Summary: See D14025. In all cases where we compare hashes, use strict, constant-time comparisons. Test Plan: Logged in, logged out, added TOTP, ran Conduit, terminated sessions, submitted forms, changed password. Tweaked CSRF token, got rejected. Reviewers: chad Reviewed By: chad Subscribers: chenxiruanhai Differential Revision: https://secure.phabricator.com/D14026 --- .../controller/PhabricatorAuthController.php | 2 +- ...bricatorAuthTerminateSessionController.php | 5 ++- .../engine/PhabricatorAuthSessionEngine.php | 5 ++- .../auth/factor/PhabricatorTOTPAuthFactor.php | 2 +- .../auth/provider/PhabricatorAuthProvider.php | 2 +- .../PhabricatorConduitAPIController.php | 3 +- .../method/ConduitConnectConduitAPIMethod.php | 2 +- ...ricatorMetaMTAMailgunReceiveController.php | 4 ++- .../people/storage/PhabricatorUser.php | 36 ++++++------------- .../PhabricatorSessionsSettingsPanel.php | 5 ++- .../password/PhabricatorPasswordHasher.php | 2 +- 11 files changed, 33 insertions(+), 35 deletions(-) diff --git a/src/applications/auth/controller/PhabricatorAuthController.php b/src/applications/auth/controller/PhabricatorAuthController.php index ece2d01dc4..db1c8c6da4 100644 --- a/src/applications/auth/controller/PhabricatorAuthController.php +++ b/src/applications/auth/controller/PhabricatorAuthController.php @@ -209,7 +209,7 @@ abstract class PhabricatorAuthController extends PhabricatorController { $actual = $account->getProperty('registrationKey'); $expect = PhabricatorHash::digest($registration_key); - if ($actual !== $expect) { + if (!phutil_hashes_are_identical($actual, $expect)) { $response = $this->renderError( pht( 'Your browser submitted a different registration key than the one '. diff --git a/src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php b/src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php index ae8179a798..43ac80bb70 100644 --- a/src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php +++ b/src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php @@ -21,7 +21,10 @@ final class PhabricatorAuthTerminateSessionController $sessions = $query->execute(); foreach ($sessions as $key => $session) { - if ($session->getSessionKey() == $current_key) { + $is_current = phutil_hashes_are_identical( + $session->getSessionKey(), + $current_key); + if ($is_current) { // Don't terminate the current login session. unset($sessions[$key]); } diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php index cec965f6b5..77ba80bccb 100644 --- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php @@ -296,7 +296,10 @@ final class PhabricatorAuthSessionEngine extends Phobject { foreach ($sessions as $key => $session) { if ($except_session !== null) { - if ($except_session == $session->getSessionKey()) { + $is_except = phutil_hashes_are_identical( + $session->getSessionKey(), + $except_session); + if ($is_except) { continue; } } diff --git a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php index 519df58ffb..f99a5f4a35 100644 --- a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php @@ -201,7 +201,7 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor { // case the server or client has some clock skew. for ($offset = -2; $offset <= 2; $offset++) { $real = self::getTOTPCode($key, $now + $offset); - if ($real === $code) { + if (phutil_hashes_are_identical($real, $code)) { return true; } } diff --git a/src/applications/auth/provider/PhabricatorAuthProvider.php b/src/applications/auth/provider/PhabricatorAuthProvider.php index 594bb89ea6..55e193f481 100644 --- a/src/applications/auth/provider/PhabricatorAuthProvider.php +++ b/src/applications/auth/provider/PhabricatorAuthProvider.php @@ -482,7 +482,7 @@ abstract class PhabricatorAuthProvider extends Phobject { 'problem persists, you may need to clear your cookies.')); } - if ($actual !== $expect) { + if (!phutil_hashes_are_identical($actual, $expect)) { throw new Exception( pht( 'The authentication provider did not return the correct client '. diff --git a/src/applications/conduit/controller/PhabricatorConduitAPIController.php b/src/applications/conduit/controller/PhabricatorConduitAPIController.php index 41106d110b..4bbe167c39 100644 --- a/src/applications/conduit/controller/PhabricatorConduitAPIController.php +++ b/src/applications/conduit/controller/PhabricatorConduitAPIController.php @@ -434,7 +434,8 @@ final class PhabricatorConduitAPIController $token = idx($metadata, 'authToken'); $signature = idx($metadata, 'authSignature'); $certificate = $user->getConduitCertificate(); - if (sha1($token.$certificate) !== $signature) { + $hash = sha1($token.$certificate); + if (!phutil_hashes_are_identical($hash, $signature)) { return array( 'ERR-INVALID-AUTH', pht('Authentication is invalid.'), diff --git a/src/applications/conduit/method/ConduitConnectConduitAPIMethod.php b/src/applications/conduit/method/ConduitConnectConduitAPIMethod.php index a6ec09ae09..9f57365d38 100644 --- a/src/applications/conduit/method/ConduitConnectConduitAPIMethod.php +++ b/src/applications/conduit/method/ConduitConnectConduitAPIMethod.php @@ -142,7 +142,7 @@ final class ConduitConnectConduitAPIMethod extends ConduitAPIMethod { $threshold)); } $valid = sha1($token.$user->getConduitCertificate()); - if ($valid != $signature) { + if (!phutil_hashes_are_identical($valid, $signature)) { throw new ConduitException('ERR-INVALID-CERTIFICATE'); } $session_key = id(new PhabricatorAuthSessionEngine())->establishSession( diff --git a/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php b/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php index 104fb128a8..0c353bf543 100644 --- a/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php +++ b/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php @@ -13,9 +13,11 @@ final class PhabricatorMetaMTAMailgunReceiveController $timestamp = $request->getStr('timestamp'); $token = $request->getStr('token'); $sig = $request->getStr('signature'); - return hash_hmac('sha256', $timestamp.$token, $api_key) == $sig; + $hash = hash_hmac('sha256', $timestamp.$token, $api_key); + return phutil_hashes_are_identical($sig, $hash); } + public function processRequest() { // No CSRF for Mailgun. diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 148a2168b4..82d17983e8 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -365,19 +365,16 @@ final class PhabricatorUser } public function validateCSRFToken($token) { - $salt = null; - $version = 'plain'; - - // This is a BREACH-mitigating token. See T3684. + // We expect a BREACH-mitigating token. See T3684. $breach_prefix = self::CSRF_BREACH_PREFIX; $breach_prelen = strlen($breach_prefix); - - if (!strncmp($token, $breach_prefix, $breach_prelen)) { - $version = 'breach'; - $salt = substr($token, $breach_prelen, self::CSRF_SALT_LENGTH); - $token = substr($token, $breach_prelen + self::CSRF_SALT_LENGTH); + if (strncmp($token, $breach_prefix, $breach_prelen) !== 0) { + return false; } + $salt = substr($token, $breach_prelen, self::CSRF_SALT_LENGTH); + $token = substr($token, $breach_prelen + self::CSRF_SALT_LENGTH); + // When the user posts a form, we check that it contains a valid CSRF token. // Tokens cycle each hour (every CSRF_CYLCE_FREQUENCY seconds) and we accept // either the current token, the next token (users can submit a "future" @@ -407,22 +404,11 @@ final class PhabricatorUser for ($ii = -$csrf_window; $ii <= 1; $ii++) { $valid = $this->getRawCSRFToken($ii); - switch ($version) { - // TODO: We can remove this after the BREACH version has been in the - // wild for a while. - case 'plain': - if ($token == $valid) { - return true; - } - break; - case 'breach': - $digest = PhabricatorHash::digest($valid, $salt); - if (substr($digest, 0, self::CSRF_TOKEN_LENGTH) == $token) { - return true; - } - break; - default: - throw new Exception(pht('Unknown CSRF token format!')); + + $digest = PhabricatorHash::digest($valid, $salt); + $digest = substr($digest, 0, self::CSRF_TOKEN_LENGTH); + if (phutil_hashes_are_identical($digest, $token)) { + return true; } } diff --git a/src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php b/src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php index b89ea8e902..8bb6653471 100644 --- a/src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php @@ -50,7 +50,10 @@ final class PhabricatorSessionsSettingsPanel extends PhabricatorSettingsPanel { $rows = array(); $rowc = array(); foreach ($sessions as $session) { - if ($session->getSessionKey() == $current_key) { + $is_current = phutil_hashes_are_identical( + $session->getSessionKey(), + $current_key); + if ($is_current) { $rowc[] = 'highlighted'; $button = phutil_tag( 'a', diff --git a/src/infrastructure/util/password/PhabricatorPasswordHasher.php b/src/infrastructure/util/password/PhabricatorPasswordHasher.php index ef8c208b9f..b7a4c1453b 100644 --- a/src/infrastructure/util/password/PhabricatorPasswordHasher.php +++ b/src/infrastructure/util/password/PhabricatorPasswordHasher.php @@ -126,7 +126,7 @@ abstract class PhabricatorPasswordHasher extends Phobject { $actual_hash = $this->getPasswordHash($password)->openEnvelope(); $expect_hash = $hash->openEnvelope(); - return ($actual_hash === $expect_hash); + return phutil_hashes_are_identical($actual_hash, $expect_hash); } From a13db0a3ec0c1c250b4199e0dbef911d1a28fbbc Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 1 Sep 2015 15:52:52 -0700 Subject: [PATCH 21/35] Allow Controllers to return a wider range of "response-like" objects Summary: Ref T1806. Ref T5752. Currently, `handleRequest()` needs to return an `AphrontResponse`, but sometimes it's really convenient to return some other object, like a Dialog, and let that convert into a response elsewhere. Formalize this and clean up some of the existing hacks for it so there's less custom/magical code in Phabricator-specific classes and more general code in Aphront classes. More broadly, I want to clean up T5752 before pursuing T9132, since I'm generally happy with how `SearchEngine` works except for how it interacts with side navs / application menus. I want to fix that first so a new Editor (which will have a lot in common with SearchEngine in terms of how controllers interact with it) doesn't make the problem twice as bad. Test Plan: - Loaded a bunch of normal pages. - Loaded dialogs. - Loaded proxy responses (submitted empty comments in Maniphest). Reviewers: chad Reviewed By: chad Subscribers: joshuaspence Maniphest Tasks: T1806, T5752 Differential Revision: https://secure.phabricator.com/D14032 --- src/__phutil_library_map__.php | 11 +- src/aphront/AphrontController.php | 4 - .../AphrontApplicationConfiguration.php | 152 +++++++++++++++++- ...AphrontDefaultApplicationConfiguration.php | 4 - .../AphrontResponseProducerInterface.php | 24 +++ src/aphront/response/AphrontProxyResponse.php | 12 +- .../base/controller/PhabricatorController.php | 22 +-- src/view/AphrontDialogView.php | 13 +- 8 files changed, 206 insertions(+), 36 deletions(-) create mode 100644 src/aphront/interface/AphrontResponseProducerInterface.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 9388482854..184bd19229 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -158,6 +158,7 @@ phutil_register_library_map(array( 'AphrontRequest' => 'aphront/AphrontRequest.php', 'AphrontRequestTestCase' => 'aphront/__tests__/AphrontRequestTestCase.php', 'AphrontResponse' => 'aphront/response/AphrontResponse.php', + 'AphrontResponseProducerInterface' => 'aphront/interface/AphrontResponseProducerInterface.php', 'AphrontRoutingMap' => 'aphront/site/AphrontRoutingMap.php', 'AphrontRoutingResult' => 'aphront/site/AphrontRoutingResult.php', 'AphrontSideNavFilterView' => 'view/layout/AphrontSideNavFilterView.php', @@ -3732,7 +3733,10 @@ phutil_register_library_map(array( 'AphrontCursorPagerView' => 'AphrontView', 'AphrontDefaultApplicationConfiguration' => 'AphrontApplicationConfiguration', 'AphrontDialogResponse' => 'AphrontResponse', - 'AphrontDialogView' => 'AphrontView', + 'AphrontDialogView' => array( + 'AphrontView', + 'AphrontResponseProducerInterface', + ), 'AphrontException' => 'Exception', 'AphrontFileResponse' => 'AphrontResponse', 'AphrontFormCheckboxControl' => 'AphrontFormControl', @@ -3775,7 +3779,10 @@ phutil_register_library_map(array( 'AphrontPageView' => 'AphrontView', 'AphrontPlainTextResponse' => 'AphrontResponse', 'AphrontProgressBarView' => 'AphrontBarView', - 'AphrontProxyResponse' => 'AphrontResponse', + 'AphrontProxyResponse' => array( + 'AphrontResponse', + 'AphrontResponseProducerInterface', + ), 'AphrontRedirectResponse' => 'AphrontResponse', 'AphrontRedirectResponseTestCase' => 'PhabricatorTestCase', 'AphrontReloadResponse' => 'AphrontRedirectResponse', diff --git a/src/aphront/AphrontController.php b/src/aphront/AphrontController.php index 97a5fc1fd1..ded0b362a3 100644 --- a/src/aphront/AphrontController.php +++ b/src/aphront/AphrontController.php @@ -24,10 +24,6 @@ abstract class AphrontController extends Phobject { return; } - public function didProcessRequest($response) { - return $response; - } - public function handleRequest(AphrontRequest $request) { if (method_exists($this, 'processRequest')) { return $this->processRequest(); diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php index e37268380c..c8b092290e 100644 --- a/src/aphront/configuration/AphrontApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontApplicationConfiguration.php @@ -1,7 +1,8 @@ willProcessRequest($uri_data); $response = $controller->handleRequest($request); + $this->validateControllerResponse($controller, $response); } } catch (Exception $ex) { $original_exception = $ex; @@ -241,8 +243,8 @@ abstract class AphrontApplicationConfiguration extends Phobject { } try { - $response = $controller->didProcessRequest($response); - $response = $this->willSendResponse($response, $controller); + $response = $this->produceResponse($request, $response); + $response = $controller->willSendResponse($response); $response->setRequest($request); $unexpected_output = PhabricatorStartup::endOutputCapture(); @@ -424,4 +426,148 @@ abstract class AphrontApplicationConfiguration extends Phobject { return $site; } + +/* -( Response Handling )-------------------------------------------------- */ + + + /** + * Tests if a response is of a valid type. + * + * @param wild Supposedly valid response. + * @return bool True if the object is of a valid type. + * @task response + */ + private function isValidResponseObject($response) { + if ($response instanceof AphrontResponse) { + return true; + } + + if ($response instanceof AphrontResponseProducerInterface) { + return true; + } + + return false; + } + + + /** + * Verifies that the return value from an @{class:AphrontController} is + * of an allowed type. + * + * @param AphrontController Controller which returned the response. + * @param wild Supposedly valid response. + * @return void + * @task response + */ + private function validateControllerResponse( + AphrontController $controller, + $response) { + + if ($this->isValidResponseObject($response)) { + return; + } + + throw new Exception( + pht( + 'Controller "%s" returned an invalid response from call to "%s". '. + 'This method must return an object of class "%s", or an object '. + 'which implements the "%s" interface.', + get_class($controller), + 'handleRequest()', + 'AphrontResponse', + 'AphrontResponseProducerInterface')); + } + + + /** + * Verifies that the erturn value from an + * @{class:AphrontResponseProducerInterface} is of an allowed type. + * + * @param AphrontResponseProducerInterface Object which produced + * this response. + * @param wild Supposedly valid response. + * @return void + * @task response + */ + private function validateProducerResponse( + AphrontResponseProducerInterface $producer, + $response) { + + if ($this->isValidResponseObject($response)) { + return; + } + + throw new Exception( + pht( + 'Producer "%s" returned an invalid response from call to "%s". '. + 'This method must return an object of class "%s", or an object '. + 'which implements the "%s" interface.', + get_class($producer), + 'produceAphrontResponse()', + 'AphrontResponse', + 'AphrontResponseProducerInterface')); + } + + + /** + * Resolves a response object into an @{class:AphrontResponse}. + * + * Controllers are permitted to return actual responses of class + * @{class:AphrontResponse}, or other objects which implement + * @{interface:AphrontResponseProducerInterface} and can produce a response. + * + * If a controller returns a response producer, invoke it now and produce + * the real response. + * + * @param AphrontRequest Request being handled. + * @param AphrontResponse|AphrontResponseProducerInterface Response, or + * response producer. + * @return AphrontResponse Response after any required production. + * @task response + */ + private function produceResponse(AphrontRequest $request, $response) { + $original = $response; + + // Detect cycles on the exact same objects. It's still possible to produce + // infinite responses as long as they're all unique, but we can only + // reasonably detect cycles, not guarantee that response production halts. + + $seen = array(); + while (true) { + // NOTE: It is permissible for an object to be both a response and a + // response producer. If so, being a producer is "stronger". This is + // used by AphrontProxyResponse. + + // If this response is a valid response, hand over the request first. + if ($response instanceof AphrontResponse) { + $response->setRequest($request); + } + + // If this isn't a producer, we're all done. + if (!($response instanceof AphrontResponseProducerInterface)) { + break; + } + + $hash = spl_object_hash($response); + if (isset($seen[$hash])) { + throw new Exception( + pht( + 'Failure while producing response for object of class "%s": '. + 'encountered production cycle (identical object, of class "%s", '. + 'was produced twice).', + get_class($original), + get_class($response))); + } + + $seen[$hash] = true; + + $new_response = $response->produceAphrontResponse(); + $this->validateProducerResponse($response, $new_response); + $response = $new_response; + } + + return $response; + } + + } diff --git a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php index 78f3c6139d..4d5f24eea1 100644 --- a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php @@ -273,10 +273,6 @@ class AphrontDefaultApplicationConfiguration return $response; } - public function willSendResponse(AphrontResponse $response) { - return $response; - } - public function build404Controller() { return array(new Phabricator404Controller(), array()); } diff --git a/src/aphront/interface/AphrontResponseProducerInterface.php b/src/aphront/interface/AphrontResponseProducerInterface.php new file mode 100644 index 0000000000..84eba28b35 --- /dev/null +++ b/src/aphront/interface/AphrontResponseProducerInterface.php @@ -0,0 +1,24 @@ +reduceProxyResponse(); + } + } diff --git a/src/applications/base/controller/PhabricatorController.php b/src/applications/base/controller/PhabricatorController.php index 33c7dfcd53..9dcfa62fa2 100644 --- a/src/applications/base/controller/PhabricatorController.php +++ b/src/applications/base/controller/PhabricatorController.php @@ -370,28 +370,8 @@ abstract class PhabricatorController extends AphrontController { return $this->buildPageResponse($page); } - public function didProcessRequest($response) { - // If a bare DialogView is returned, wrap it in a DialogResponse. - if ($response instanceof AphrontDialogView) { - $response = id(new AphrontDialogResponse())->setDialog($response); - } - + public function willSendResponse(AphrontResponse $response) { $request = $this->getRequest(); - $response->setRequest($request); - - $seen = array(); - while ($response instanceof AphrontProxyResponse) { - $hash = spl_object_hash($response); - if (isset($seen[$hash])) { - $seen[] = get_class($response); - throw new Exception( - pht('Cycle while reducing proxy responses: %s', - implode(' -> ', $seen))); - } - $seen[$hash] = get_class($response); - - $response = $response->reduceProxyResponse(); - } if ($response instanceof AphrontDialogResponse) { if (!$request->isAjax() && !$request->isQuicksand()) { diff --git a/src/view/AphrontDialogView.php b/src/view/AphrontDialogView.php index ca866b11a8..a336e990a2 100644 --- a/src/view/AphrontDialogView.php +++ b/src/view/AphrontDialogView.php @@ -1,6 +1,8 @@ setDialog($this); + } + } From 28621244ad0792b594720c6ad852bb4289789b24 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 3 Sep 2015 10:03:31 -0700 Subject: [PATCH 22/35] Fix abrupt failure mode for uncloned repositories in Diffusion Summary: Fixes T9080. We try to list alternatives for the current ref (for example, if you're viewing a branch named "master" but there's also a tag named "master", or, in Mercurial, there are several branches named "master") but fail to abruptly if we can't get the list. It's fine if we can't get the list; just continue. This is common when the repository hasn't cloned yet. Test Plan: In a local repository with bad credentials, tried to do anything before and after. Before: completely blocked by error; after: things work normally. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9080 Differential Revision: https://secure.phabricator.com/D14044 --- .../controller/DiffusionRepositoryController.php | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/applications/diffusion/controller/DiffusionRepositoryController.php b/src/applications/diffusion/controller/DiffusionRepositoryController.php index 03637b288f..7f2201e7fd 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryController.php @@ -302,7 +302,16 @@ final class DiffusionRepositoryController extends DiffusionController { $info = null; $drequest = $this->getDiffusionRequest(); - if ($drequest->getRefAlternatives()) { + + // Try to load alternatives. This may fail for repositories which have not + // cloned yet. If it does, just ignore it and continue. + try { + $alternatives = $drequest->getRefAlternatives(); + } catch (ConduitClientException $ex) { + $alternatives = array(); + } + + if ($alternatives) { $message = array( pht( 'The ref "%s" is ambiguous in this repository.', From 7ebbe0fe71a6fff9876a8de795e82d5de1c82348 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 3 Sep 2015 10:03:50 -0700 Subject: [PATCH 23/35] Add a "Printable Version" link to Phortune invoices Summary: Ref T9309. This is a minor quality of life improvement, hopefully. We already have print CSS, just expose it more clearly. Also, hide actions (these never seem useful?) and footers from printable versions. I opened the printable version in a new window since it now doesn't have any actions. Test Plan: {F777241} {F777242} Reviewers: chad Reviewed By: chad Maniphest Tasks: T9309 Differential Revision: https://secure.phabricator.com/D14045 --- resources/celerity/map.php | 10 +++++----- .../controller/PhortuneCartViewController.php | 8 ++++++++ src/view/layout/PhabricatorActionView.php | 17 +++++++++++++++++ .../css/application/base/standard-page-view.css | 4 ++++ .../rsrc/css/phui/phui-property-list-view.css | 4 ++++ 5 files changed, 38 insertions(+), 5 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index c600d8d7bd..a076cf7814 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -7,7 +7,7 @@ */ return array( 'names' => array( - 'core.pkg.css' => '928faf7e', + 'core.pkg.css' => '994de4ed', 'core.pkg.js' => '47dc9ebb', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '2de124c9', @@ -36,7 +36,7 @@ return array( 'rsrc/css/application/base/notification-menu.css' => 'f31c0bde', 'rsrc/css/application/base/phabricator-application-launch-view.css' => '95351601', 'rsrc/css/application/base/phui-theme.css' => '6b451f24', - 'rsrc/css/application/base/standard-page-view.css' => '4d176b67', + 'rsrc/css/application/base/standard-page-view.css' => '1f53d056', 'rsrc/css/application/calendar/calendar-icon.css' => 'c69aa59f', 'rsrc/css/application/chatlog/chatlog.css' => 'd295b020', 'rsrc/css/application/conduit/conduit-api.css' => '7bc725c4', @@ -141,7 +141,7 @@ return array( 'rsrc/css/phui/phui-object-item-list-view.css' => 'ab1bf393', 'rsrc/css/phui/phui-pager.css' => 'bea33d23', 'rsrc/css/phui/phui-pinboard-view.css' => '2495140e', - 'rsrc/css/phui/phui-property-list-view.css' => '15bbe0b0', + 'rsrc/css/phui/phui-property-list-view.css' => '318d4dea', 'rsrc/css/phui/phui-remarkup-preview.css' => '867f85b3', 'rsrc/css/phui/phui-spacing.css' => '042804d6', 'rsrc/css/phui/phui-status.css' => '888cedb8', @@ -743,7 +743,7 @@ return array( 'phabricator-side-menu-view-css' => 'bec2458e', 'phabricator-slowvote-css' => '475b4bd2', 'phabricator-source-code-view-css' => '5e0178de', - 'phabricator-standard-page-view' => '4d176b67', + 'phabricator-standard-page-view' => '1f53d056', 'phabricator-textareautils' => '5c93c52c', 'phabricator-title' => 'df5e11d2', 'phabricator-tooltip' => '1d298e3a', @@ -794,7 +794,7 @@ return array( 'phui-object-item-list-view-css' => 'ab1bf393', 'phui-pager-css' => 'bea33d23', 'phui-pinboard-view-css' => '2495140e', - 'phui-property-list-view-css' => '15bbe0b0', + 'phui-property-list-view-css' => '318d4dea', 'phui-remarkup-preview-css' => '867f85b3', 'phui-spacing-css' => '042804d6', 'phui-status-list-view-css' => '888cedb8', diff --git a/src/applications/phortune/controller/PhortuneCartViewController.php b/src/applications/phortune/controller/PhortuneCartViewController.php index f4eb0d9612..67f7c3df01 100644 --- a/src/applications/phortune/controller/PhortuneCartViewController.php +++ b/src/applications/phortune/controller/PhortuneCartViewController.php @@ -268,6 +268,7 @@ final class PhortuneCartViewController $refund_uri = $this->getApplicationURI("{$prefix}cart/{$id}/refund/"); $update_uri = $this->getApplicationURI("{$prefix}cart/{$id}/update/"); $accept_uri = $this->getApplicationURI("{$prefix}cart/{$id}/accept/"); + $print_uri = $this->getApplicationURI("{$prefix}cart/{$id}/?__print__=1"); $view->addAction( id(new PhabricatorActionView()) @@ -309,6 +310,13 @@ final class PhortuneCartViewController ->setHref($resume_uri)); } + $view->addAction( + id(new PhabricatorActionView()) + ->setName(pht('Printable Version')) + ->setHref($print_uri) + ->setOpenInNewWindow(true) + ->setIcon('fa-print')); + return $view; } diff --git a/src/view/layout/PhabricatorActionView.php b/src/view/layout/PhabricatorActionView.php index 43f84495c7..8c56f66968 100644 --- a/src/view/layout/PhabricatorActionView.php +++ b/src/view/layout/PhabricatorActionView.php @@ -14,6 +14,7 @@ final class PhabricatorActionView extends AphrontView { private $sigils = array(); private $metadata; private $selected; + private $openInNewWindow; public function setSelected($selected) { $this->selected = $selected; @@ -107,6 +108,15 @@ final class PhabricatorActionView extends AphrontView { return $this; } + public function setOpenInNewWindow($open_in_new_window) { + $this->openInNewWindow = $open_in_new_window; + return $this; + } + + public function getOpenInNewWindow() { + return $this->openInNewWindow; + } + public function render() { $icon = null; @@ -161,11 +171,18 @@ final class PhabricatorActionView extends AphrontView { ), $item); } else { + if ($this->getOpenInNewWindow()) { + $target = '_blank'; + } else { + $target = null; + } + $item = javelin_tag( 'a', array( 'href' => $this->getHref(), 'class' => 'phabricator-action-view-item', + 'target' => $target, 'sigil' => $sigils, 'meta' => $this->metadata, ), diff --git a/webroot/rsrc/css/application/base/standard-page-view.css b/webroot/rsrc/css/application/base/standard-page-view.css index 2d9b79dfea..3e24e597e3 100644 --- a/webroot/rsrc/css/application/base/standard-page-view.css +++ b/webroot/rsrc/css/application/base/standard-page-view.css @@ -25,6 +25,10 @@ color: {$greytext}; } +!print .phabricator-standard-page-footer { + display: none; +} + .device-desktop .has-local-nav + .phabricator-standard-page-footer { margin-left: 221px; } diff --git a/webroot/rsrc/css/phui/phui-property-list-view.css b/webroot/rsrc/css/phui/phui-property-list-view.css index 71a096e69e..e59906a6a3 100644 --- a/webroot/rsrc/css/phui/phui-property-list-view.css +++ b/webroot/rsrc/css/phui/phui-property-list-view.css @@ -176,6 +176,10 @@ border-left: 1px solid {$thinblueborder}; } +!print .phui-property-list-actions { + display: none; +} + .device .phui-property-list-actions { float: none; width: auto; From 20ce1a905f3a51aff1e9f976db7977862625d99d Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 3 Sep 2015 10:04:17 -0700 Subject: [PATCH 24/35] Replace AphrontUsageException with AphrontMalformedRequestException Summary: Ref T1806. Ref T7173. Context here is that I want to fix "you can not log in to this instance" being a confusing mess with an opaque error. To do this without hacks, I want to: - clean up some exception handling behavior (this diff); - modularize exception handling (next diff); - replace confusing, over-general exceptions with tailored ones in the Phacility cluster, using the new modular stuff. This cleans up an awkward "AphrontUsageException" which does some weird stuff right now. In particular, it is extensible and extended in one place in Diffusion, but that extension is meaningless. Realign this as "AphrontMalformedRequestException", which is a better description of what it is and does: raises errors before we can get as far as normal routing and site handling. Test Plan: Hit some of these exceptions, saw the expected "abandon all hope" error page. Reviewers: chad Reviewed By: chad Maniphest Tasks: T1806, T7173 Differential Revision: https://secure.phabricator.com/D14047 --- src/__phutil_library_map__.php | 6 ++-- .../AphrontApplicationConfiguration.php | 10 +++--- ...AphrontDefaultApplicationConfiguration.php | 16 --------- .../AphrontMalformedRequestException.php | 34 +++++++++++++++++++ .../exception/AphrontUsageException.php | 21 ------------ .../AphrontUnhandledExceptionResponse.php | 18 ++++++++-- .../exception/DiffusionSetupException.php | 8 +---- 7 files changed, 60 insertions(+), 53 deletions(-) create mode 100644 src/aphront/exception/AphrontMalformedRequestException.php delete mode 100644 src/aphront/exception/AphrontUsageException.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 184bd19229..9bde309819 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -143,6 +143,7 @@ phutil_register_library_map(array( 'AphrontJavelinView' => 'view/AphrontJavelinView.php', 'AphrontKeyboardShortcutsAvailableView' => 'view/widget/AphrontKeyboardShortcutsAvailableView.php', 'AphrontListFilterView' => 'view/layout/AphrontListFilterView.php', + 'AphrontMalformedRequestException' => 'aphront/exception/AphrontMalformedRequestException.php', 'AphrontMoreView' => 'view/layout/AphrontMoreView.php', 'AphrontMultiColumnView' => 'view/layout/AphrontMultiColumnView.php', 'AphrontMySQLDatabaseConnectionTestCase' => 'infrastructure/storage/__tests__/AphrontMySQLDatabaseConnectionTestCase.php', @@ -170,7 +171,6 @@ phutil_register_library_map(array( 'AphrontTokenizerTemplateView' => 'view/control/AphrontTokenizerTemplateView.php', 'AphrontTypeaheadTemplateView' => 'view/control/AphrontTypeaheadTemplateView.php', 'AphrontUnhandledExceptionResponse' => 'aphront/response/AphrontUnhandledExceptionResponse.php', - 'AphrontUsageException' => 'aphront/exception/AphrontUsageException.php', 'AphrontView' => 'view/AphrontView.php', 'AphrontWebpageResponse' => 'aphront/response/AphrontWebpageResponse.php', 'ArcanistConduitAPIMethod' => 'applications/arcanist/conduit/ArcanistConduitAPIMethod.php', @@ -3771,6 +3771,7 @@ phutil_register_library_map(array( 'AphrontJavelinView' => 'AphrontView', 'AphrontKeyboardShortcutsAvailableView' => 'AphrontView', 'AphrontListFilterView' => 'AphrontView', + 'AphrontMalformedRequestException' => 'AphrontException', 'AphrontMoreView' => 'AphrontView', 'AphrontMultiColumnView' => 'AphrontView', 'AphrontMySQLDatabaseConnectionTestCase' => 'PhabricatorTestCase', @@ -3800,7 +3801,6 @@ phutil_register_library_map(array( 'AphrontTokenizerTemplateView' => 'AphrontView', 'AphrontTypeaheadTemplateView' => 'AphrontView', 'AphrontUnhandledExceptionResponse' => 'AphrontStandaloneHTMLResponse', - 'AphrontUsageException' => 'AphrontException', 'AphrontView' => array( 'Phobject', 'PhutilSafeHTMLProducerInterface', @@ -4383,7 +4383,7 @@ phutil_register_library_map(array( 'DiffusionSearchQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', 'DiffusionServeController' => 'DiffusionController', 'DiffusionSetPasswordSettingsPanel' => 'PhabricatorSettingsPanel', - 'DiffusionSetupException' => 'AphrontUsageException', + 'DiffusionSetupException' => 'Exception', 'DiffusionSubversionSSHWorkflow' => 'DiffusionSSHWorkflow', 'DiffusionSubversionServeSSHWorkflow' => 'DiffusionSubversionSSHWorkflow', 'DiffusionSubversionWireProtocol' => 'Phobject', diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php index c8b092290e..dad223a63f 100644 --- a/src/aphront/configuration/AphrontApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontApplicationConfiguration.php @@ -318,7 +318,7 @@ abstract class AphrontApplicationConfiguration extends Phobject { // This is a command line script (probably something like a unit // test) so it's fine that we don't have SERVER_ADDR defined. } else { - throw new AphrontUsageException( + throw new AphrontMalformedRequestException( pht('No %s', 'SERVER_ADDR'), pht( 'Phabricator is configured to operate in cluster mode, but '. @@ -330,7 +330,7 @@ abstract class AphrontApplicationConfiguration extends Phobject { } } else { if (!PhabricatorEnv::isClusterAddress($server_addr)) { - throw new AphrontUsageException( + throw new AphrontMalformedRequestException( pht('External Interface'), pht( 'Phabricator is configured in cluster mode and the address '. @@ -413,12 +413,14 @@ abstract class AphrontApplicationConfiguration extends Phobject { if (!$site) { $path = $request->getPath(); $host = $request->getHost(); - throw new Exception( + throw new AphrontMalformedRequestException( + pht('Site Not Found'), pht( 'This request asked for "%s" on host "%s", but no site is '. 'configured which can serve this request.', $path, - $host)); + $host), + true); } $request->setSite($site); diff --git a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php index 4d5f24eea1..c51aac3749 100644 --- a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php @@ -210,22 +210,6 @@ class AphrontDefaultApplicationConfiguration return $response; } - if ($ex instanceof AphrontUsageException) { - $error = new PHUIInfoView(); - $error->setTitle($ex->getTitle()); - $error->appendChild($ex->getMessage()); - - $view = new PhabricatorStandardPageView(); - $view->setRequest($this->getRequest()); - $view->appendChild($error); - - $response = new AphrontWebpageResponse(); - $response->setContent($view->render()); - $response->setHTTPResponseCode(500); - - return $response; - } - // Always log the unhandled exception. phlog($ex); diff --git a/src/aphront/exception/AphrontMalformedRequestException.php b/src/aphront/exception/AphrontMalformedRequestException.php new file mode 100644 index 0000000000..a4f265f473 --- /dev/null +++ b/src/aphront/exception/AphrontMalformedRequestException.php @@ -0,0 +1,34 @@ +title = $title; + $this->isUnlogged = $unlogged; + parent::__construct($message); + } + + public function getTitle() { + return $this->title; + } + + public function getIsUnlogged() { + return $this->isUnlogged; + } + +} diff --git a/src/aphront/exception/AphrontUsageException.php b/src/aphront/exception/AphrontUsageException.php deleted file mode 100644 index d242b56dea..0000000000 --- a/src/aphront/exception/AphrontUsageException.php +++ /dev/null @@ -1,21 +0,0 @@ -title = $title; - parent::__construct($message); - } - - public function getTitle() { - return $this->title; - } - -} diff --git a/src/aphront/response/AphrontUnhandledExceptionResponse.php b/src/aphront/response/AphrontUnhandledExceptionResponse.php index c741c35898..efd9d70ead 100644 --- a/src/aphront/response/AphrontUnhandledExceptionResponse.php +++ b/src/aphront/response/AphrontUnhandledExceptionResponse.php @@ -6,6 +6,20 @@ final class AphrontUnhandledExceptionResponse private $exception; public function setException(Exception $exception) { + // Log the exception unless it's specifically a silent malformed request + // exception. + + $should_log = true; + if ($exception instanceof AphrontMalformedRequestException) { + if ($exception->getIsUnlogged()) { + $should_log = false; + } + } + + if ($should_log) { + phlog($exception); + } + $this->exception = $exception; return $this; } @@ -24,7 +38,7 @@ final class AphrontUnhandledExceptionResponse protected function getResponseTitle() { $ex = $this->exception; - if ($ex instanceof AphrontUsageException) { + if ($ex instanceof AphrontMalformedRequestException) { return $ex->getTitle(); } else { return pht('Unhandled Exception'); @@ -38,7 +52,7 @@ final class AphrontUnhandledExceptionResponse protected function getResponseBody() { $ex = $this->exception; - if ($ex instanceof AphrontUsageException) { + if ($ex instanceof AphrontMalformedRequestException) { $title = $ex->getTitle(); } else { $title = get_class($ex); diff --git a/src/applications/diffusion/exception/DiffusionSetupException.php b/src/applications/diffusion/exception/DiffusionSetupException.php index 0ce5994b7e..6efaefaf94 100644 --- a/src/applications/diffusion/exception/DiffusionSetupException.php +++ b/src/applications/diffusion/exception/DiffusionSetupException.php @@ -1,9 +1,3 @@ Date: Thu, 3 Sep 2015 10:04:42 -0700 Subject: [PATCH 25/35] Modularize Aphront exception handling Summary: Ref T1806. Ref T7173. Depends on D14047. Currently, all exception handling is in this big messy clump in `AphrontDefaultApplicationConfiguration`. Split it out into modular classes. This will let a future change add new classes in the Phacility cluster which intercept particular exceptions we care about and replaces the default, generic responses with more useful, tailored responses. Test Plan: {F777391} - Hit a Conduit error (made a method throw). - Hit an Ajax error (made comment preview throw). - Hit a high security error (tried to edit TOTP). - Hit a rate limiting error (added a bunch of email addresses). - Hit a policy error (tried to look at something with no permission). - Hit an arbitrary exception (made a randomc ontroller throw). Reviewers: chad Reviewed By: chad Maniphest Tasks: T1806, T7173 Differential Revision: https://secure.phabricator.com/D14049 --- src/__phutil_library_map__.php | 18 ++ .../AphrontApplicationConfiguration.php | 64 +++++- ...AphrontDefaultApplicationConfiguration.php | 213 ------------------ .../AphrontRequestExceptionHandler.php | 36 +++ ...PhabricatorAjaxRequestExceptionHandler.php | 38 ++++ ...bricatorConduitRequestExceptionHandler.php | 33 +++ ...bricatorDefaultRequestExceptionHandler.php | 76 +++++++ ...torHighSecurityRequestExceptionHandler.php | 76 +++++++ ...abricatorPolicyRequestExceptionHandler.php | 93 ++++++++ ...icatorRateLimitRequestExceptionHandler.php | 42 ++++ .../PhabricatorRequestExceptionHandler.php | 26 +++ .../module/PhabricatorConfigEdgeModule.php | 2 +- .../module/PhabricatorConfigPHIDModule.php | 2 +- ...torConfigRequestExceptionHandlerModule.php | 47 ++++ .../module/PhabricatorConfigSiteModule.php | 2 +- 15 files changed, 550 insertions(+), 218 deletions(-) create mode 100644 src/aphront/handler/AphrontRequestExceptionHandler.php create mode 100644 src/aphront/handler/PhabricatorAjaxRequestExceptionHandler.php create mode 100644 src/aphront/handler/PhabricatorConduitRequestExceptionHandler.php create mode 100644 src/aphront/handler/PhabricatorDefaultRequestExceptionHandler.php create mode 100644 src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php create mode 100644 src/aphront/handler/PhabricatorPolicyRequestExceptionHandler.php create mode 100644 src/aphront/handler/PhabricatorRateLimitRequestExceptionHandler.php create mode 100644 src/aphront/handler/PhabricatorRequestExceptionHandler.php create mode 100644 src/applications/config/module/PhabricatorConfigRequestExceptionHandlerModule.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 9bde309819..262d144ecd 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -157,6 +157,7 @@ phutil_register_library_map(array( 'AphrontRedirectResponseTestCase' => 'aphront/response/__tests__/AphrontRedirectResponseTestCase.php', 'AphrontReloadResponse' => 'aphront/response/AphrontReloadResponse.php', 'AphrontRequest' => 'aphront/AphrontRequest.php', + 'AphrontRequestExceptionHandler' => 'aphront/handler/AphrontRequestExceptionHandler.php', 'AphrontRequestTestCase' => 'aphront/__tests__/AphrontRequestTestCase.php', 'AphrontResponse' => 'aphront/response/AphrontResponse.php', 'AphrontResponseProducerInterface' => 'aphront/interface/AphrontResponseProducerInterface.php', @@ -1484,6 +1485,7 @@ phutil_register_library_map(array( 'PhabricatorActionView' => 'view/layout/PhabricatorActionView.php', 'PhabricatorActivitySettingsPanel' => 'applications/settings/panel/PhabricatorActivitySettingsPanel.php', 'PhabricatorAdministratorsPolicyRule' => 'applications/policy/rule/PhabricatorAdministratorsPolicyRule.php', + 'PhabricatorAjaxRequestExceptionHandler' => 'aphront/handler/PhabricatorAjaxRequestExceptionHandler.php', 'PhabricatorAlmanacApplication' => 'applications/almanac/application/PhabricatorAlmanacApplication.php', 'PhabricatorAmazonAuthProvider' => 'applications/auth/provider/PhabricatorAmazonAuthProvider.php', 'PhabricatorAnchorView' => 'view/layout/PhabricatorAnchorView.php', @@ -1786,6 +1788,7 @@ phutil_register_library_map(array( 'PhabricatorConduitLogQuery' => 'applications/conduit/query/PhabricatorConduitLogQuery.php', 'PhabricatorConduitMethodCallLog' => 'applications/conduit/storage/PhabricatorConduitMethodCallLog.php', 'PhabricatorConduitMethodQuery' => 'applications/conduit/query/PhabricatorConduitMethodQuery.php', + 'PhabricatorConduitRequestExceptionHandler' => 'aphront/handler/PhabricatorConduitRequestExceptionHandler.php', 'PhabricatorConduitSearchEngine' => 'applications/conduit/query/PhabricatorConduitSearchEngine.php', 'PhabricatorConduitTestCase' => '__tests__/PhabricatorConduitTestCase.php', 'PhabricatorConduitToken' => 'applications/conduit/storage/PhabricatorConduitToken.php', @@ -1838,6 +1841,7 @@ phutil_register_library_map(array( 'PhabricatorConfigOptionType' => 'applications/config/custom/PhabricatorConfigOptionType.php', 'PhabricatorConfigPHIDModule' => 'applications/config/module/PhabricatorConfigPHIDModule.php', 'PhabricatorConfigProxySource' => 'infrastructure/env/PhabricatorConfigProxySource.php', + 'PhabricatorConfigRequestExceptionHandlerModule' => 'applications/config/module/PhabricatorConfigRequestExceptionHandlerModule.php', 'PhabricatorConfigResponse' => 'applications/config/response/PhabricatorConfigResponse.php', 'PhabricatorConfigSchemaQuery' => 'applications/config/schema/PhabricatorConfigSchemaQuery.php', 'PhabricatorConfigSchemaSpec' => 'applications/config/schema/PhabricatorConfigSchemaSpec.php', @@ -1993,6 +1997,7 @@ phutil_register_library_map(array( 'PhabricatorDatabaseSetupCheck' => 'applications/config/check/PhabricatorDatabaseSetupCheck.php', 'PhabricatorDateTimeSettingsPanel' => 'applications/settings/panel/PhabricatorDateTimeSettingsPanel.php', 'PhabricatorDebugController' => 'applications/system/controller/PhabricatorDebugController.php', + 'PhabricatorDefaultRequestExceptionHandler' => 'aphront/handler/PhabricatorDefaultRequestExceptionHandler.php', 'PhabricatorDesktopNotificationsSettingsPanel' => 'applications/settings/panel/PhabricatorDesktopNotificationsSettingsPanel.php', 'PhabricatorDestructibleInterface' => 'applications/system/interface/PhabricatorDestructibleInterface.php', 'PhabricatorDestructionEngine' => 'applications/system/engine/PhabricatorDestructionEngine.php', @@ -2183,6 +2188,7 @@ phutil_register_library_map(array( 'PhabricatorHelpEditorProtocolController' => 'applications/help/controller/PhabricatorHelpEditorProtocolController.php', 'PhabricatorHelpKeyboardShortcutController' => 'applications/help/controller/PhabricatorHelpKeyboardShortcutController.php', 'PhabricatorHeraldApplication' => 'applications/herald/application/PhabricatorHeraldApplication.php', + 'PhabricatorHighSecurityRequestExceptionHandler' => 'aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php', 'PhabricatorHomeApplication' => 'applications/home/application/PhabricatorHomeApplication.php', 'PhabricatorHomeController' => 'applications/home/controller/PhabricatorHomeController.php', 'PhabricatorHomeMainController' => 'applications/home/controller/PhabricatorHomeMainController.php', @@ -2580,6 +2586,7 @@ phutil_register_library_map(array( 'PhabricatorPolicyManagementWorkflow' => 'applications/policy/management/PhabricatorPolicyManagementWorkflow.php', 'PhabricatorPolicyPHIDTypePolicy' => 'applications/policy/phid/PhabricatorPolicyPHIDTypePolicy.php', 'PhabricatorPolicyQuery' => 'applications/policy/query/PhabricatorPolicyQuery.php', + 'PhabricatorPolicyRequestExceptionHandler' => 'aphront/handler/PhabricatorPolicyRequestExceptionHandler.php', 'PhabricatorPolicyRule' => 'applications/policy/rule/PhabricatorPolicyRule.php', 'PhabricatorPolicyTestCase' => 'applications/policy/__tests__/PhabricatorPolicyTestCase.php', 'PhabricatorPolicyTestObject' => 'applications/policy/__tests__/PhabricatorPolicyTestObject.php', @@ -2667,6 +2674,7 @@ phutil_register_library_map(array( 'PhabricatorQueryOrderItem' => 'infrastructure/query/order/PhabricatorQueryOrderItem.php', 'PhabricatorQueryOrderTestCase' => 'infrastructure/query/order/__tests__/PhabricatorQueryOrderTestCase.php', 'PhabricatorQueryOrderVector' => 'infrastructure/query/order/PhabricatorQueryOrderVector.php', + 'PhabricatorRateLimitRequestExceptionHandler' => 'aphront/handler/PhabricatorRateLimitRequestExceptionHandler.php', 'PhabricatorRecaptchaConfigOptions' => 'applications/config/option/PhabricatorRecaptchaConfigOptions.php', 'PhabricatorRecipientHasBadgeEdgeType' => 'applications/badges/edge/PhabricatorRecipientHasBadgeEdgeType.php', 'PhabricatorRedirectController' => 'applications/base/controller/PhabricatorRedirectController.php', @@ -2758,6 +2766,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryURITestCase' => 'applications/repository/storage/__tests__/PhabricatorRepositoryURITestCase.php', 'PhabricatorRepositoryVCSPassword' => 'applications/repository/storage/PhabricatorRepositoryVCSPassword.php', 'PhabricatorRepositoryVersion' => 'applications/repository/constants/PhabricatorRepositoryVersion.php', + 'PhabricatorRequestExceptionHandler' => 'aphront/handler/PhabricatorRequestExceptionHandler.php', 'PhabricatorResourceSite' => 'aphront/site/PhabricatorResourceSite.php', 'PhabricatorRobotsController' => 'applications/system/controller/PhabricatorRobotsController.php', 'PhabricatorS3FileStorageEngine' => 'applications/files/engine/PhabricatorS3FileStorageEngine.php', @@ -3788,6 +3797,7 @@ phutil_register_library_map(array( 'AphrontRedirectResponseTestCase' => 'PhabricatorTestCase', 'AphrontReloadResponse' => 'AphrontRedirectResponse', 'AphrontRequest' => 'Phobject', + 'AphrontRequestExceptionHandler' => 'Phobject', 'AphrontRequestTestCase' => 'PhabricatorTestCase', 'AphrontResponse' => 'Phobject', 'AphrontRoutingMap' => 'Phobject', @@ -5303,6 +5313,7 @@ phutil_register_library_map(array( 'PhabricatorActionView' => 'AphrontView', 'PhabricatorActivitySettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorAdministratorsPolicyRule' => 'PhabricatorPolicyRule', + 'PhabricatorAjaxRequestExceptionHandler' => 'PhabricatorRequestExceptionHandler', 'PhabricatorAlmanacApplication' => 'PhabricatorApplication', 'PhabricatorAmazonAuthProvider' => 'PhabricatorOAuth2AuthProvider', 'PhabricatorAnchorView' => 'AphrontView', @@ -5667,6 +5678,7 @@ phutil_register_library_map(array( 'PhabricatorPolicyInterface', ), 'PhabricatorConduitMethodQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'PhabricatorConduitRequestExceptionHandler' => 'PhabricatorRequestExceptionHandler', 'PhabricatorConduitSearchEngine' => 'PhabricatorApplicationSearchEngine', 'PhabricatorConduitTestCase' => 'PhabricatorTestCase', 'PhabricatorConduitToken' => array( @@ -5729,6 +5741,7 @@ phutil_register_library_map(array( 'PhabricatorConfigOptionType' => 'Phobject', 'PhabricatorConfigPHIDModule' => 'PhabricatorConfigModule', 'PhabricatorConfigProxySource' => 'PhabricatorConfigSource', + 'PhabricatorConfigRequestExceptionHandlerModule' => 'PhabricatorConfigModule', 'PhabricatorConfigResponse' => 'AphrontStandaloneHTMLResponse', 'PhabricatorConfigSchemaQuery' => 'Phobject', 'PhabricatorConfigSchemaSpec' => 'Phobject', @@ -5913,6 +5926,7 @@ phutil_register_library_map(array( 'PhabricatorDatabaseSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorDateTimeSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorDebugController' => 'PhabricatorController', + 'PhabricatorDefaultRequestExceptionHandler' => 'PhabricatorRequestExceptionHandler', 'PhabricatorDesktopNotificationsSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorDestructionEngine' => 'Phobject', 'PhabricatorDeveloperConfigOptions' => 'PhabricatorApplicationConfigOptions', @@ -6138,6 +6152,7 @@ phutil_register_library_map(array( 'PhabricatorHelpEditorProtocolController' => 'PhabricatorHelpController', 'PhabricatorHelpKeyboardShortcutController' => 'PhabricatorHelpController', 'PhabricatorHeraldApplication' => 'PhabricatorApplication', + 'PhabricatorHighSecurityRequestExceptionHandler' => 'PhabricatorRequestExceptionHandler', 'PhabricatorHomeApplication' => 'PhabricatorApplication', 'PhabricatorHomeController' => 'PhabricatorController', 'PhabricatorHomeMainController' => 'PhabricatorHomeController', @@ -6587,6 +6602,7 @@ phutil_register_library_map(array( 'PhabricatorPolicyManagementWorkflow' => 'PhabricatorManagementWorkflow', 'PhabricatorPolicyPHIDTypePolicy' => 'PhabricatorPHIDType', 'PhabricatorPolicyQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'PhabricatorPolicyRequestExceptionHandler' => 'PhabricatorRequestExceptionHandler', 'PhabricatorPolicyRule' => 'Phobject', 'PhabricatorPolicyTestCase' => 'PhabricatorTestCase', 'PhabricatorPolicyTestObject' => array( @@ -6702,6 +6718,7 @@ phutil_register_library_map(array( 'Phobject', 'Iterator', ), + 'PhabricatorRateLimitRequestExceptionHandler' => 'PhabricatorRequestExceptionHandler', 'PhabricatorRecaptchaConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorRecipientHasBadgeEdgeType' => 'PhabricatorEdgeType', 'PhabricatorRedirectController' => 'PhabricatorController', @@ -6828,6 +6845,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryURITestCase' => 'PhabricatorTestCase', 'PhabricatorRepositoryVCSPassword' => 'PhabricatorRepositoryDAO', 'PhabricatorRepositoryVersion' => 'Phobject', + 'PhabricatorRequestExceptionHandler' => 'AphrontRequestExceptionHandler', 'PhabricatorResourceSite' => 'PhabricatorSite', 'PhabricatorRobotsController' => 'PhabricatorController', 'PhabricatorS3FileStorageEngine' => 'PhabricatorFileStorageEngine', diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php index dad223a63f..50b8c123fd 100644 --- a/src/aphront/configuration/AphrontApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontApplicationConfiguration.php @@ -3,6 +3,7 @@ /** * @task routing URI Routing * @task response Response Handling + * @task exception Exception Handling */ abstract class AphrontApplicationConfiguration extends Phobject { @@ -11,7 +12,6 @@ abstract class AphrontApplicationConfiguration extends Phobject { private $path; private $console; - abstract public function getApplicationName(); abstract public function buildRequest(); abstract public function build404Controller(); abstract public function buildRedirectController($uri, $external); @@ -482,7 +482,7 @@ abstract class AphrontApplicationConfiguration extends Phobject { /** - * Verifies that the erturn value from an + * Verifies that the return value from an * @{class:AphrontResponseProducerInterface} is of an allowed type. * * @param AphrontResponseProducerInterface Object which produced @@ -511,6 +511,36 @@ abstract class AphrontApplicationConfiguration extends Phobject { } + /** + * Verifies that the return value from an + * @{class:AphrontRequestExceptionHandler} is of an allowed type. + * + * @param AphrontRequestExceptionHandler Object which produced this + * response. + * @param wild Supposedly valid response. + * @return void + * @task response + */ + private function validateErrorHandlerResponse( + AphrontRequestExceptionHandler $handler, + $response) { + + if ($this->isValidResponseObject($response)) { + return; + } + + throw new Exception( + pht( + 'Exception handler "%s" returned an invalid response from call to '. + '"%s". This method must return an object of class "%s", or an object '. + 'which implements the "%s" interface.', + get_class($handler), + 'handleRequestException()', + 'AphrontResponse', + 'AphrontResponseProducerInterface')); + } + + /** * Resolves a response object into an @{class:AphrontResponse}. * @@ -572,4 +602,34 @@ abstract class AphrontApplicationConfiguration extends Phobject { } +/* -( Error Handling )----------------------------------------------------- */ + + + /** + * Convert an exception which has escaped the controller into a response. + * + * This method delegates exception handling to available subclasses of + * @{class:AphrontRequestExceptionHandler}. + * + * @param Exception Exception which needs to be handled. + * @return wild Response or response producer, or null if no available + * handler can produce a response. + * @task exception + */ + private function handleException(Exception $ex) { + $handlers = AphrontRequestExceptionHandler::getAllHandlers(); + + $request = $this->getRequest(); + foreach ($handlers as $handler) { + if ($handler->canHandleRequestException($request, $ex)) { + $response = $handler->handleRequestException($request, $ex); + $this->validateErrorHandlerResponse($handler, $response); + return $response; + } + } + + throw $ex; + } + + } diff --git a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php index c51aac3749..cd1347411a 100644 --- a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php @@ -8,12 +8,6 @@ class AphrontDefaultApplicationConfiguration extends AphrontApplicationConfiguration { - public function __construct() {} - - public function getApplicationName() { - return 'aphront-default'; - } - /** * @phutil-external-symbol class PhabricatorStartup */ @@ -50,213 +44,6 @@ class AphrontDefaultApplicationConfiguration return $request; } - public function handleException(Exception $ex) { - $request = $this->getRequest(); - - // For Conduit requests, return a Conduit response. - if ($request->isConduit()) { - $response = new ConduitAPIResponse(); - $response->setErrorCode(get_class($ex)); - $response->setErrorInfo($ex->getMessage()); - - return id(new AphrontJSONResponse()) - ->setAddJSONShield(false) - ->setContent($response->toDictionary()); - } - - // For non-workflow requests, return a Ajax response. - if ($request->isAjax() && !$request->isWorkflow()) { - // Log these; they don't get shown on the client and can be difficult - // to debug. - phlog($ex); - - $response = new AphrontAjaxResponse(); - $response->setError( - array( - 'code' => get_class($ex), - 'info' => $ex->getMessage(), - )); - return $response; - } - - $user = $request->getUser(); - if (!$user) { - // If we hit an exception very early, we won't have a user. - $user = new PhabricatorUser(); - } - - if ($ex instanceof PhabricatorSystemActionRateLimitException) { - $dialog = id(new AphrontDialogView()) - ->setTitle(pht('Slow Down!')) - ->setUser($user) - ->setErrors(array(pht('You are being rate limited.'))) - ->appendParagraph($ex->getMessage()) - ->appendParagraph($ex->getRateExplanation()) - ->addCancelButton('/', pht('Okaaaaaaaaaaaaaay...')); - - $response = new AphrontDialogResponse(); - $response->setDialog($dialog); - return $response; - } - - if ($ex instanceof PhabricatorAuthHighSecurityRequiredException) { - - $form = id(new PhabricatorAuthSessionEngine())->renderHighSecurityForm( - $ex->getFactors(), - $ex->getFactorValidationResults(), - $user, - $request); - - $dialog = id(new AphrontDialogView()) - ->setUser($user) - ->setTitle(pht('Entering High Security')) - ->setShortTitle(pht('Security Checkpoint')) - ->setWidth(AphrontDialogView::WIDTH_FORM) - ->addHiddenInput(AphrontRequest::TYPE_HISEC, true) - ->setErrors( - array( - pht( - 'You are taking an action which requires you to enter '. - 'high security.'), - )) - ->appendParagraph( - pht( - 'High security mode helps protect your account from security '. - 'threats, like session theft or someone messing with your stuff '. - 'while you\'re grabbing a coffee. To enter high security mode, '. - 'confirm your credentials.')) - ->appendChild($form->buildLayoutView()) - ->appendParagraph( - pht( - 'Your account will remain in high security mode for a short '. - 'period of time. When you are finished taking sensitive '. - 'actions, you should leave high security.')) - ->setSubmitURI($request->getPath()) - ->addCancelButton($ex->getCancelURI()) - ->addSubmitButton(pht('Enter High Security')); - - $request_parameters = $request->getPassthroughRequestParameters( - $respect_quicksand = true); - foreach ($request_parameters as $key => $value) { - $dialog->addHiddenInput($key, $value); - } - - $response = new AphrontDialogResponse(); - $response->setDialog($dialog); - return $response; - } - - if ($ex instanceof PhabricatorPolicyException) { - if (!$user->isLoggedIn()) { - // If the user isn't logged in, just give them a login form. This is - // probably a generally more useful response than a policy dialog that - // they have to click through to get a login form. - // - // Possibly we should add a header here like "you need to login to see - // the thing you are trying to look at". - $login_controller = new PhabricatorAuthStartController(); - $login_controller->setRequest($request); - - $auth_app_class = 'PhabricatorAuthApplication'; - $auth_app = PhabricatorApplication::getByClass($auth_app_class); - $login_controller->setCurrentApplication($auth_app); - - return $login_controller->handleRequest($request); - } - - $content = array( - phutil_tag( - 'div', - array( - 'class' => 'aphront-policy-rejection', - ), - $ex->getRejection()), - ); - - $list = null; - if ($ex->getCapabilityName()) { - $list = $ex->getMoreInfo(); - foreach ($list as $key => $item) { - $list[$key] = $item; - } - - $content[] = phutil_tag( - 'div', - array( - 'class' => 'aphront-capability-details', - ), - pht('Users with the "%s" capability:', $ex->getCapabilityName())); - - } - - $dialog = id(new AphrontDialogView()) - ->setTitle($ex->getTitle()) - ->setClass('aphront-access-dialog') - ->setUser($user) - ->appendChild($content); - - if ($list) { - $dialog->appendList($list); - } - - if ($this->getRequest()->isAjax()) { - $dialog->addCancelButton('/', pht('Close')); - } else { - $dialog->addCancelButton('/', pht('OK')); - } - - $response = new AphrontDialogResponse(); - $response->setDialog($dialog); - return $response; - } - - // Always log the unhandled exception. - phlog($ex); - - $class = get_class($ex); - $message = $ex->getMessage(); - - if ($ex instanceof AphrontSchemaQueryException) { - $message .= "\n\n".pht( - "NOTE: This usually indicates that the MySQL schema has not been ". - "properly upgraded. Run '%s' to ensure your schema is up to date.", - 'bin/storage upgrade'); - } - - if (PhabricatorEnv::getEnvConfig('phabricator.developer-mode')) { - $trace = id(new AphrontStackTraceView()) - ->setUser($user) - ->setTrace($ex->getTrace()); - } else { - $trace = null; - } - - $content = phutil_tag( - 'div', - array('class' => 'aphront-unhandled-exception'), - array( - phutil_tag('div', array('class' => 'exception-message'), $message), - $trace, - )); - - $dialog = new AphrontDialogView(); - $dialog - ->setTitle(pht('Unhandled Exception ("%s")', $class)) - ->setClass('aphront-exception-dialog') - ->setUser($user) - ->appendChild($content); - - if ($this->getRequest()->isAjax()) { - $dialog->addCancelButton('/', pht('Close')); - } - - $response = new AphrontDialogResponse(); - $response->setDialog($dialog); - $response->setHTTPResponseCode(500); - - return $response; - } - public function build404Controller() { return array(new Phabricator404Controller(), array()); } diff --git a/src/aphront/handler/AphrontRequestExceptionHandler.php b/src/aphront/handler/AphrontRequestExceptionHandler.php new file mode 100644 index 0000000000..694071e1ba --- /dev/null +++ b/src/aphront/handler/AphrontRequestExceptionHandler.php @@ -0,0 +1,36 @@ +setAncestorClass(__CLASS__) + ->setSortMethod('getRequestExceptionHandlerPriority') + ->execute(); + } + +} diff --git a/src/aphront/handler/PhabricatorAjaxRequestExceptionHandler.php b/src/aphront/handler/PhabricatorAjaxRequestExceptionHandler.php new file mode 100644 index 0000000000..06023e02de --- /dev/null +++ b/src/aphront/handler/PhabricatorAjaxRequestExceptionHandler.php @@ -0,0 +1,38 @@ +isAjax() && !$request->isWorkflow()); + } + + public function handleRequestException( + AphrontRequest $request, + Exception $ex) { + + // Log these; they don't get shown on the client and can be difficult + // to debug. + phlog($ex); + + $response = new AphrontAjaxResponse(); + $response->setError( + array( + 'code' => get_class($ex), + 'info' => $ex->getMessage(), + )); + return $response; + } + +} diff --git a/src/aphront/handler/PhabricatorConduitRequestExceptionHandler.php b/src/aphront/handler/PhabricatorConduitRequestExceptionHandler.php new file mode 100644 index 0000000000..367f607f83 --- /dev/null +++ b/src/aphront/handler/PhabricatorConduitRequestExceptionHandler.php @@ -0,0 +1,33 @@ +isConduit(); + } + + public function handleRequestException( + AphrontRequest $request, + Exception $ex) { + + $response = id(new ConduitAPIResponse()) + ->setErrorCode(get_class($ex)) + ->setErrorInfo($ex->getMessage()); + + return id(new AphrontJSONResponse()) + ->setAddJSONShield(false) + ->setContent($response->toDictionary()); + } + +} diff --git a/src/aphront/handler/PhabricatorDefaultRequestExceptionHandler.php b/src/aphront/handler/PhabricatorDefaultRequestExceptionHandler.php new file mode 100644 index 0000000000..321dd6d69b --- /dev/null +++ b/src/aphront/handler/PhabricatorDefaultRequestExceptionHandler.php @@ -0,0 +1,76 @@ +isPhabricatorSite($request)) { + return false; + } + + return true; + } + + public function handleRequestException( + AphrontRequest $request, + Exception $ex) { + + $viewer = $this->getViewer($request); + + // Always log the unhandled exception. + phlog($ex); + + $class = get_class($ex); + $message = $ex->getMessage(); + + if ($ex instanceof AphrontSchemaQueryException) { + $message .= "\n\n".pht( + "NOTE: This usually indicates that the MySQL schema has not been ". + "properly upgraded. Run '%s' to ensure your schema is up to date.", + 'bin/storage upgrade'); + } + + if (PhabricatorEnv::getEnvConfig('phabricator.developer-mode')) { + $trace = id(new AphrontStackTraceView()) + ->setUser($viewer) + ->setTrace($ex->getTrace()); + } else { + $trace = null; + } + + $content = phutil_tag( + 'div', + array('class' => 'aphront-unhandled-exception'), + array( + phutil_tag('div', array('class' => 'exception-message'), $message), + $trace, + )); + + $dialog = new AphrontDialogView(); + $dialog + ->setTitle(pht('Unhandled Exception ("%s")', $class)) + ->setClass('aphront-exception-dialog') + ->setUser($viewer) + ->appendChild($content); + + if ($request->isAjax()) { + $dialog->addCancelButton('/', pht('Close')); + } + + return id(new AphrontDialogResponse()) + ->setDialog($dialog) + ->setHTTPResponseCode(500); + } + +} diff --git a/src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php b/src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php new file mode 100644 index 0000000000..ff4ace2e4b --- /dev/null +++ b/src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php @@ -0,0 +1,76 @@ +isPhabricatorSite($request)) { + return false; + } + + return ($ex instanceof PhabricatorAuthHighSecurityRequiredException); + } + + public function handleRequestException( + AphrontRequest $request, + Exception $ex) { + + $viewer = $this->getViewer($request); + + $form = id(new PhabricatorAuthSessionEngine())->renderHighSecurityForm( + $ex->getFactors(), + $ex->getFactorValidationResults(), + $viewer, + $request); + + $dialog = id(new AphrontDialogView()) + ->setUser($viewer) + ->setTitle(pht('Entering High Security')) + ->setShortTitle(pht('Security Checkpoint')) + ->setWidth(AphrontDialogView::WIDTH_FORM) + ->addHiddenInput(AphrontRequest::TYPE_HISEC, true) + ->setErrors( + array( + pht( + 'You are taking an action which requires you to enter '. + 'high security.'), + )) + ->appendParagraph( + pht( + 'High security mode helps protect your account from security '. + 'threats, like session theft or someone messing with your stuff '. + 'while you\'re grabbing a coffee. To enter high security mode, '. + 'confirm your credentials.')) + ->appendChild($form->buildLayoutView()) + ->appendParagraph( + pht( + 'Your account will remain in high security mode for a short '. + 'period of time. When you are finished taking sensitive '. + 'actions, you should leave high security.')) + ->setSubmitURI($request->getPath()) + ->addCancelButton($ex->getCancelURI()) + ->addSubmitButton(pht('Enter High Security')); + + $request_parameters = $request->getPassthroughRequestParameters( + $respect_quicksand = true); + foreach ($request_parameters as $key => $value) { + $dialog->addHiddenInput($key, $value); + } + + return $dialog; + } + +} diff --git a/src/aphront/handler/PhabricatorPolicyRequestExceptionHandler.php b/src/aphront/handler/PhabricatorPolicyRequestExceptionHandler.php new file mode 100644 index 0000000000..1c84e90fd1 --- /dev/null +++ b/src/aphront/handler/PhabricatorPolicyRequestExceptionHandler.php @@ -0,0 +1,93 @@ +isPhabricatorSite($request)) { + return false; + } + + return ($ex instanceof PhabricatorPolicyException); + } + + public function handleRequestException( + AphrontRequest $request, + Exception $ex) { + + $viewer = $this->getViewer($request); + + if (!$viewer->isLoggedIn()) { + // If the user isn't logged in, just give them a login form. This is + // probably a generally more useful response than a policy dialog that + // they have to click through to get a login form. + // + // Possibly we should add a header here like "you need to login to see + // the thing you are trying to look at". + $auth_app_class = 'PhabricatorAuthApplication'; + $auth_app = PhabricatorApplication::getByClass($auth_app_class); + + return id(new PhabricatorAuthStartController()) + ->setRequest($request) + ->setCurrentApplication($auth_app) + ->handleRequest($request); + } + + $content = array( + phutil_tag( + 'div', + array( + 'class' => 'aphront-policy-rejection', + ), + $ex->getRejection()), + ); + + $list = null; + if ($ex->getCapabilityName()) { + $list = $ex->getMoreInfo(); + foreach ($list as $key => $item) { + $list[$key] = $item; + } + + $content[] = phutil_tag( + 'div', + array( + 'class' => 'aphront-capability-details', + ), + pht('Users with the "%s" capability:', $ex->getCapabilityName())); + + } + + $dialog = id(new AphrontDialogView()) + ->setTitle($ex->getTitle()) + ->setClass('aphront-access-dialog') + ->setUser($viewer) + ->appendChild($content); + + if ($list) { + $dialog->appendList($list); + } + + if ($request->isAjax()) { + $dialog->addCancelButton('/', pht('Close')); + } else { + $dialog->addCancelButton('/', pht('OK')); + } + + return $dialog; + } + +} diff --git a/src/aphront/handler/PhabricatorRateLimitRequestExceptionHandler.php b/src/aphront/handler/PhabricatorRateLimitRequestExceptionHandler.php new file mode 100644 index 0000000000..3fb549c29f --- /dev/null +++ b/src/aphront/handler/PhabricatorRateLimitRequestExceptionHandler.php @@ -0,0 +1,42 @@ +isPhabricatorSite($request)) { + return false; + } + + return ($ex instanceof PhabricatorSystemActionRateLimitException); + } + + public function handleRequestException( + AphrontRequest $request, + Exception $ex) { + + $viewer = $this->getViewer($request); + + return id(new AphrontDialogView()) + ->setTitle(pht('Slow Down!')) + ->setUser($viewer) + ->setErrors(array(pht('You are being rate limited.'))) + ->appendParagraph($ex->getMessage()) + ->appendParagraph($ex->getRateExplanation()) + ->addCancelButton('/', pht('Okaaaaaaaaaaaaaay...')); + } + +} diff --git a/src/aphront/handler/PhabricatorRequestExceptionHandler.php b/src/aphront/handler/PhabricatorRequestExceptionHandler.php new file mode 100644 index 0000000000..81e21b9107 --- /dev/null +++ b/src/aphront/handler/PhabricatorRequestExceptionHandler.php @@ -0,0 +1,26 @@ +getSite(); + if (!$site) { + return false; + } + + return ($site instanceof PhabricatorSite); + } + + protected function getViewer(AphrontRequest $request) { + $viewer = $request->getUser(); + + if ($viewer) { + return $viewer; + } + + // If we hit an exception very early, we won't have a user yet. + return new PhabricatorUser(); + } + +} diff --git a/src/applications/config/module/PhabricatorConfigEdgeModule.php b/src/applications/config/module/PhabricatorConfigEdgeModule.php index f8a348caf3..8aa5d74664 100644 --- a/src/applications/config/module/PhabricatorConfigEdgeModule.php +++ b/src/applications/config/module/PhabricatorConfigEdgeModule.php @@ -41,7 +41,7 @@ final class PhabricatorConfigEdgeModule extends PhabricatorConfigModule { return id(new PHUIObjectBoxView()) ->setHeaderText(pht('Edge Types')) - ->appendChild($table); + ->setTable($table); } } diff --git a/src/applications/config/module/PhabricatorConfigPHIDModule.php b/src/applications/config/module/PhabricatorConfigPHIDModule.php index 300740ea77..6c363e3bee 100644 --- a/src/applications/config/module/PhabricatorConfigPHIDModule.php +++ b/src/applications/config/module/PhabricatorConfigPHIDModule.php @@ -41,7 +41,7 @@ final class PhabricatorConfigPHIDModule extends PhabricatorConfigModule { return id(new PHUIObjectBoxView()) ->setHeaderText(pht('PHID Types')) - ->appendChild($table); + ->setTable($table); } } diff --git a/src/applications/config/module/PhabricatorConfigRequestExceptionHandlerModule.php b/src/applications/config/module/PhabricatorConfigRequestExceptionHandlerModule.php new file mode 100644 index 0000000000..22539a1bd3 --- /dev/null +++ b/src/applications/config/module/PhabricatorConfigRequestExceptionHandlerModule.php @@ -0,0 +1,47 @@ +getViewer(); + + $handlers = AphrontRequestExceptionHandler::getAllHandlers(); + + $rows = array(); + foreach ($handlers as $key => $handler) { + $rows[] = array( + $handler->getRequestExceptionHandlerPriority(), + $key, + $handler->getRequestExceptionHandlerDescription(), + ); + } + + $table = id(new AphrontTableView($rows)) + ->setHeaders( + array( + pht('Priority'), + pht('Class'), + pht('Description'), + )) + ->setColumnClasses( + array( + null, + 'pri', + 'wide', + )); + + return id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Exception Handlers')) + ->setTable($table); + } + +} diff --git a/src/applications/config/module/PhabricatorConfigSiteModule.php b/src/applications/config/module/PhabricatorConfigSiteModule.php index 0b85db2a10..3f63ae2da4 100644 --- a/src/applications/config/module/PhabricatorConfigSiteModule.php +++ b/src/applications/config/module/PhabricatorConfigSiteModule.php @@ -40,7 +40,7 @@ final class PhabricatorConfigSiteModule extends PhabricatorConfigModule { return id(new PHUIObjectBoxView()) ->setHeaderText(pht('Sites')) - ->appendChild($table); + ->setTable($table); } } From 9d0332c2c0a4c3332b249eaf882b77adda9fd0da Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 3 Sep 2015 10:05:23 -0700 Subject: [PATCH 26/35] Modernize OAuthserver and provide more context on "no permission" exception Summary: Ref T7173. Depends on D14049. Now that Phacility can install custom exception handlers, this puts enough information on the exception so that we can figure out what to do with it. - Generally modernize some of this code. - Add some more information to PolicyExceptions so the new RequestExceptionHandler can handle them properly. Test Plan: Failed authorizations, then succeeded authorizations. See next diff. Reviewers: chad Reviewed By: chad Maniphest Tasks: T7173 Differential Revision: https://secure.phabricator.com/D14050 --- src/__phutil_library_map__.php | 4 +- .../fund/phortune/FundBackerProduct.php | 13 ++-- .../PhabricatorOAuthServerAuthController.php | 62 +++++++------------ .../PhabricatorOAuthServerController.php | 57 ++--------------- .../PhabricatorOAuthServerTestController.php | 31 ++-------- .../PhabricatorOAuthServerTokenController.php | 22 ++++--- .../exception/PhabricatorPolicyException.php | 30 +++++++++ .../policy/filter/PhabricatorPolicyFilter.php | 11 +++- .../policy/storage/PhabricatorPolicy.php | 6 ++ 9 files changed, 106 insertions(+), 130 deletions(-) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 262d144ecd..aba0b8a1c5 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -6376,7 +6376,7 @@ phutil_register_library_map(array( 'PhabricatorOAuthServer' => 'Phobject', 'PhabricatorOAuthServerAccessToken' => 'PhabricatorOAuthServerDAO', 'PhabricatorOAuthServerApplication' => 'PhabricatorApplication', - 'PhabricatorOAuthServerAuthController' => 'PhabricatorAuthController', + 'PhabricatorOAuthServerAuthController' => 'PhabricatorOAuthServerController', 'PhabricatorOAuthServerAuthorizationCode' => 'PhabricatorOAuthServerDAO', 'PhabricatorOAuthServerAuthorizationsSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorOAuthServerClient' => array( @@ -6394,7 +6394,7 @@ phutil_register_library_map(array( 'PhabricatorOAuthServerScope' => 'Phobject', 'PhabricatorOAuthServerTestCase' => 'PhabricatorTestCase', 'PhabricatorOAuthServerTestController' => 'PhabricatorOAuthServerController', - 'PhabricatorOAuthServerTokenController' => 'PhabricatorAuthController', + 'PhabricatorOAuthServerTokenController' => 'PhabricatorOAuthServerController', 'PhabricatorObjectHandle' => array( 'Phobject', 'PhabricatorPolicyInterface', diff --git a/src/applications/fund/phortune/FundBackerProduct.php b/src/applications/fund/phortune/FundBackerProduct.php index 79ffc559bb..679a1a41d2 100644 --- a/src/applications/fund/phortune/FundBackerProduct.php +++ b/src/applications/fund/phortune/FundBackerProduct.php @@ -21,10 +21,15 @@ final class FundBackerProduct extends PhortuneProductImplementation { public function getName(PhortuneProduct $product) { $initiative = $this->getInitiative(); - return pht( - 'Fund %s %s', - $initiative->getMonogram(), - $initiative->getName()); + + if (!$initiative) { + return pht('Fund '); + } else { + return pht( + 'Fund %s %s', + $initiative->getMonogram(), + $initiative->getName()); + } } public function getPriceAsCurrency(PhortuneProduct $product) { diff --git a/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php b/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php index 4ff83b4d5f..d473e1557a 100644 --- a/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php +++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php @@ -1,20 +1,15 @@ getViewer(); - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); - - $server = new PhabricatorOAuthServer(); - $client_phid = $request->getStr('client_id'); - $scope = $request->getStr('scope'); - $redirect_uri = $request->getStr('redirect_uri'); + $server = new PhabricatorOAuthServer(); + $client_phid = $request->getStr('client_id'); + $scope = $request->getStr('scope'); + $redirect_uri = $request->getStr('redirect_uri'); $response_type = $request->getStr('response_type'); // state is an opaque value the client sent us for their own purposes @@ -30,6 +25,19 @@ final class PhabricatorOAuthServerAuthController phutil_tag('strong', array(), 'client_id'))); } + // We require that users must be able to see an OAuth application + // in order to authorize it. This allows an application's visibility + // policy to be used to restrict authorized users. + try { + $client = id(new PhabricatorOAuthServerClientQuery()) + ->setViewer($viewer) + ->withPHIDs(array($client_phid)) + ->executeOne(); + } catch (PhabricatorPolicyException $ex) { + $ex->setContext(self::CONTEXT_AUTHORIZE); + throw $ex; + } + $server->setUser($viewer); $is_authorized = false; $authorization = null; @@ -39,24 +47,6 @@ final class PhabricatorOAuthServerAuthController // one giant try / catch around all the exciting database stuff so we // can return a 'server_error' response if something goes wrong! try { - try { - $client = id(new PhabricatorOAuthServerClientQuery()) - ->setViewer($viewer) - ->withPHIDs(array($client_phid)) - ->executeOne(); - } catch (PhabricatorPolicyException $ex) { - // We require that users must be able to see an OAuth application - // in order to authorize it. This allows an application's visibility - // policy to be used to restrict authorized users. - - // None of the OAuth error responses are a perfect fit for this, but - // 'invalid_client' seems closest. - return $this->buildErrorResponse( - 'invalid_client', - pht('Not Authorized'), - pht('You are not authorized to authenticate.')); - } - if (!$client) { return $this->buildErrorResponse( 'invalid_request', @@ -211,6 +201,7 @@ final class PhabricatorOAuthServerAuthController } else { $desired_scopes = $scope; } + if (!PhabricatorOAuthServerScope::validateScopesDict($desired_scopes)) { return $this->buildErrorResponse( 'invalid_scope', @@ -236,8 +227,8 @@ final class PhabricatorOAuthServerAuthController 'error_description' => $cancel_msg, )); - $dialog = id(new AphrontDialogView()) - ->setUser($viewer) + return $this->newDialog() + ->setShortTitle(pht('Authorize Access')) ->setTitle(pht('Authorize "%s"?', $name)) ->setSubmitURI($request->getRequestURI()->getPath()) ->setWidth(AphrontDialogView::WIDTH_FORM) @@ -250,23 +241,18 @@ final class PhabricatorOAuthServerAuthController ->appendChild($form->buildLayoutView()) ->addSubmitButton(pht('Authorize Access')) ->addCancelButton((string)$cancel_uri, pht('Do Not Authorize')); - - return id(new AphrontDialogResponse())->setDialog($dialog); } private function buildErrorResponse($code, $title, $message) { $viewer = $this->getRequest()->getUser(); - $dialog = id(new AphrontDialogView()) - ->setUser($viewer) + return $this->newDialog() ->setTitle(pht('OAuth: %s', $title)) ->appendParagraph($message) ->appendParagraph( pht('OAuth Error Code: %s', phutil_tag('tt', array(), $code))) ->addCancelButton('/', pht('Alas!')); - - return id(new AphrontDialogResponse())->setDialog($dialog); } diff --git a/src/applications/oauthserver/controller/PhabricatorOAuthServerController.php b/src/applications/oauthserver/controller/PhabricatorOAuthServerController.php index 22388c8766..00be8a1e8d 100644 --- a/src/applications/oauthserver/controller/PhabricatorOAuthServerController.php +++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerController.php @@ -3,58 +3,13 @@ abstract class PhabricatorOAuthServerController extends PhabricatorController { - public function buildStandardPageResponse($view, array $data) { - $user = $this->getRequest()->getUser(); - $page = $this->buildStandardPageView(); - $page->setApplicationName(pht('OAuth Server')); - $page->setBaseURI('/oauthserver/'); - $page->setTitle(idx($data, 'title')); + const CONTEXT_AUTHORIZE = 'oauthserver.authorize'; - $nav = new AphrontSideNavFilterView(); - $nav->setBaseURI(new PhutilURI('/oauthserver/')); - $nav->addLabel(pht('Clients')); - $nav->addFilter('client/create', pht('Create Client')); - foreach ($this->getExtraClientFilters() as $filter) { - $nav->addFilter($filter['url'], $filter['label']); - } - $nav->addFilter('client', pht('My Clients')); - $nav->selectFilter($this->getFilter(), 'clientauthorization'); - - $nav->appendChild($view); - - $page->appendChild($nav); - - $response = new AphrontWebpageResponse(); - return $response->setContent($page->render()); + protected function buildApplicationCrumbs() { + // We're specifically not putting an "OAuth Server" application crumb + // on these pages because it doesn't make sense to send users there on + // the auth workflows. + return new PHUICrumbsView(); } - protected function getFilter() { - return 'clientauthorization'; - } - - protected function getExtraClientFilters() { - return array(); - } - - protected function getHighlightPHIDs() { - $phids = array(); - $request = $this->getRequest(); - $edited = $request->getStr('edited'); - $new = $request->getStr('new'); - if ($edited) { - $phids[$edited] = $edited; - } - if ($new) { - $phids[$new] = $new; - } - return $phids; - } - - protected function buildErrorView($error_message) { - $error = new PHUIInfoView(); - $error->setSeverity(PHUIInfoView::SEVERITY_ERROR); - $error->setTitle($error_message); - - return $error; - } } diff --git a/src/applications/oauthserver/controller/PhabricatorOAuthServerTestController.php b/src/applications/oauthserver/controller/PhabricatorOAuthServerTestController.php index d95fdf7d43..e5966518f5 100644 --- a/src/applications/oauthserver/controller/PhabricatorOAuthServerTestController.php +++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerTestController.php @@ -3,26 +3,13 @@ final class PhabricatorOAuthServerTestController extends PhabricatorOAuthServerController { - private $id; - - public function shouldRequireLogin() { - return true; - } - - public function willProcessRequest(array $data) { - $this->id = $data['id']; - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); - - $panels = array(); - $results = array(); + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); + $id = $request->getURIData('id'); $client = id(new PhabricatorOAuthServerClientQuery()) ->setViewer($viewer) - ->withIDs(array($this->id)) + ->withIDs(array($id)) ->executeOne(); if (!$client) { return new Aphront404Response(); @@ -37,16 +24,13 @@ final class PhabricatorOAuthServerTestController ->withClientPHIDs(array($client->getPHID())) ->executeOne(); if ($authorization) { - $dialog = id(new AphrontDialogView()) - ->setUser($viewer) + return $this->newDialog() ->setTitle(pht('Already Authorized')) ->appendParagraph( pht( 'You have already authorized this application to access your '. 'account.')) ->addCancelButton($view_uri, pht('Close')); - - return id(new AphrontDialogResponse())->setDialog($dialog); } if ($request->isFormPost()) { @@ -65,8 +49,7 @@ final class PhabricatorOAuthServerTestController // TODO: It would be nice to put scope options in this dialog, maybe? - $dialog = id(new AphrontDialogView()) - ->setUser($viewer) + return $this->newDialog() ->setTitle(pht('Authorize Application?')) ->appendParagraph( pht( @@ -75,7 +58,5 @@ final class PhabricatorOAuthServerTestController phutil_tag('strong', array(), $client->getName()))) ->addCancelButton($view_uri) ->addSubmitButton(pht('Authorize Application')); - - return id(new AphrontDialogResponse())->setDialog($dialog); } } diff --git a/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php b/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php index aa9b8f71a0..7e35574f61 100644 --- a/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php +++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php @@ -1,7 +1,7 @@ getRequest(); - $grant_type = $request->getStr('grant_type'); - $code = $request->getStr('code'); - $redirect_uri = $request->getStr('redirect_uri'); - $client_phid = $request->getStr('client_id'); + public function handleRequest(AphrontRequest $request) { + $grant_type = $request->getStr('grant_type'); + $code = $request->getStr('code'); + $redirect_uri = $request->getStr('redirect_uri'); + $client_phid = $request->getStr('client_id'); $client_secret = $request->getStr('client_secret'); - $response = new PhabricatorOAuthResponse(); - $server = new PhabricatorOAuthServer(); + $response = new PhabricatorOAuthResponse(); + $server = new PhabricatorOAuthServer(); + if ($grant_type != 'authorization_code') { $response->setError('unsupported_grant_type'); $response->setErrorDescription( @@ -32,11 +32,13 @@ final class PhabricatorOAuthServerTokenController 'authorization_code')); return $response; } + if (!$code) { $response->setError('invalid_request'); $response->setErrorDescription(pht('Required parameter code missing.')); return $response; } + if (!$client_phid) { $response->setError('invalid_request'); $response->setErrorDescription( @@ -45,6 +47,7 @@ final class PhabricatorOAuthServerTokenController 'client_id')); return $response; } + if (!$client_secret) { $response->setError('invalid_request'); $response->setErrorDescription( @@ -53,6 +56,7 @@ final class PhabricatorOAuthServerTokenController 'client_secret')); return $response; } + // one giant try / catch around all the exciting database stuff so we // can return a 'server_error' response if something goes wrong! try { diff --git a/src/applications/policy/exception/PhabricatorPolicyException.php b/src/applications/policy/exception/PhabricatorPolicyException.php index 782e1350f0..7d58666d65 100644 --- a/src/applications/policy/exception/PhabricatorPolicyException.php +++ b/src/applications/policy/exception/PhabricatorPolicyException.php @@ -6,6 +6,9 @@ final class PhabricatorPolicyException extends Exception { private $rejection; private $capabilityName; private $moreInfo = array(); + private $objectPHID; + private $context; + private $capability; public function setTitle($title) { $this->title = $title; @@ -43,4 +46,31 @@ final class PhabricatorPolicyException extends Exception { return $this->moreInfo; } + public function setObjectPHID($object_phid) { + $this->objectPHID = $object_phid; + return $this; + } + + public function getObjectPHID() { + return $this->objectPHID; + } + + public function setContext($context) { + $this->context = $context; + return $this; + } + + public function getContext() { + return $this->context; + } + + public function setCapability($capability) { + $this->capability = $capability; + return $this; + } + + public function getCapability() { + return $this->capability; + } + } diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php index dcff36d338..f9be22ad52 100644 --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -589,7 +589,9 @@ final class PhabricatorPolicyFilter extends Phobject { $exception = id(new PhabricatorPolicyException($full_message)) ->setTitle($access_denied) + ->setObjectPHID($object->getPHID()) ->setRejection($rejection) + ->setCapability($capability) ->setCapabilityName($capability_name) ->setMoreInfo($details); @@ -710,6 +712,11 @@ final class PhabricatorPolicyFilter extends Phobject { $objects = $policy->getRuleObjects(); $action = null; foreach ($policy->getRules() as $rule) { + if (!is_array($rule)) { + // Reject, this policy rule is invalid. + return false; + } + $rule_object = idx($objects, idx($rule, 'rule')); if (!$rule_object) { // Reject, this policy has a bogus rule. @@ -831,7 +838,9 @@ final class PhabricatorPolicyFilter extends Phobject { $exception = id(new PhabricatorPolicyException($full_message)) ->setTitle($access_denied) - ->setRejection($rejection); + ->setObjectPHID($object->getPHID()) + ->setRejection($rejection) + ->setCapability(PhabricatorPolicyCapability::CAN_VIEW); throw $exception; } diff --git a/src/applications/policy/storage/PhabricatorPolicy.php b/src/applications/policy/storage/PhabricatorPolicy.php index c23858b0a8..93afb35dc9 100644 --- a/src/applications/policy/storage/PhabricatorPolicy.php +++ b/src/applications/policy/storage/PhabricatorPolicy.php @@ -321,6 +321,12 @@ final class PhabricatorPolicy $classes = array(); foreach ($this->getRules() as $rule) { + if (!is_array($rule)) { + // This rule is invalid. We'll reject it later, but don't need to + // extract anything from it for now. + continue; + } + $class = idx($rule, 'rule'); try { if (class_exists($class)) { From 4428a25a7cb488fa8905e3b61850ddf41e77bb47 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Thu, 3 Sep 2015 10:53:16 -0700 Subject: [PATCH 27/35] Minor Ponder Comment tweaks Summary: Makes the New Comment, See Comments more obviously placed to find. Test Plan: Review new CSS, answer question, comment, etc. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D14043 --- resources/celerity/map.php | 4 ++-- .../ponder/view/PonderFooterView.php | 4 ++-- .../css/application/ponder/ponder-view.css | 19 +++++++++++-------- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index a076cf7814..a58be05060 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -93,7 +93,7 @@ return array( 'rsrc/css/application/policy/policy-edit.css' => '815c66f7', 'rsrc/css/application/policy/policy-transaction-detail.css' => '82100a43', 'rsrc/css/application/policy/policy.css' => '957ea14c', - 'rsrc/css/application/ponder/ponder-view.css' => 'bef48f86', + 'rsrc/css/application/ponder/ponder-view.css' => '7b0df4da', 'rsrc/css/application/projects/project-icon.css' => '4e3eaa5a', 'rsrc/css/application/releeph/releeph-core.css' => '9b3c5733', 'rsrc/css/application/releeph/releeph-preview-branch.css' => 'b7a6f4a5', @@ -811,7 +811,7 @@ return array( 'policy-css' => '957ea14c', 'policy-edit-css' => '815c66f7', 'policy-transaction-detail-css' => '82100a43', - 'ponder-view-css' => 'bef48f86', + 'ponder-view-css' => '7b0df4da', 'project-icon-css' => '4e3eaa5a', 'raphael-core' => '51ee6b43', 'raphael-g' => '40dde778', diff --git a/src/applications/ponder/view/PonderFooterView.php b/src/applications/ponder/view/PonderFooterView.php index b1f346b147..26e35ec45a 100644 --- a/src/applications/ponder/view/PonderFooterView.php +++ b/src/applications/ponder/view/PonderFooterView.php @@ -37,7 +37,7 @@ final class PonderFooterView extends AphrontTagView { if ($this->count == 0) { $icon = id(new PHUIIconView()) - ->setIconFont('fa-plus-circle msr'); + ->setIconFont('fa-comments msr'); $text = pht('Add a Comment'); } else { $icon = id(new PHUIIconView()) @@ -78,7 +78,7 @@ final class PonderFooterView extends AphrontTagView { $actions[] = $hide_action; $actions[] = $show_action; - return array($this->actions, $actions); + return array($actions, $this->actions); } } diff --git a/webroot/rsrc/css/application/ponder/ponder-view.css b/webroot/rsrc/css/application/ponder/ponder-view.css index 39d5c2a8f2..f1182d531c 100644 --- a/webroot/rsrc/css/application/ponder/ponder-view.css +++ b/webroot/rsrc/css/application/ponder/ponder-view.css @@ -54,30 +54,33 @@ } .ponder-footer-view { - margin: 0 4px -4px; - text-align: right; + margin: 0 0 -4px; + text-align: left; } .ponder-footer-view .ponder-footer-action { padding: 4px 8px; - margin-left: 8px; - color: {$bluetext}; + margin-right: 8px; + color: {$anchor}; display: inline-block; background-color: rgba(71, 87, 120, 0.06); - font-size: {$smallerfontsize}; } .ponder-footer-view .ponder-footer-action.ponder-footer-action-helpful { background-color: {$lightyellow}; + color: {$bluetext}; +} + +.ponder-footer-view .ponder-footer-action.ponder-footer-action-helpful + .phui-icon-view { + color: {$bluetext}; } .ponder-footer-view .ponder-footer-action .phui-icon-view { - color: {$bluetext}; - font-size: {$smallerfontsize}; + color: {$anchor}; } .ponder-footer-view a:hover { text-decoration: none; - color: {$darkbluetext}; background-color: rgba(71, 87, 120, 0.10); } From 1e1551d970cbfe2b90536ecd5dd1cbc907ab8b50 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Thu, 3 Sep 2015 10:55:17 -0700 Subject: [PATCH 28/35] Add CCs to Phriction Edit page Summary: Fixes T4099. Allows prepopulating CCs when building Phriction pages. Test Plan: Add notchad, remove notchad. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T4099 Differential Revision: https://secure.phabricator.com/D14042 --- .../controller/PhrictionEditController.php | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/applications/phriction/controller/PhrictionEditController.php b/src/applications/phriction/controller/PhrictionEditController.php index 546524123e..65c4fcf062 100644 --- a/src/applications/phriction/controller/PhrictionEditController.php +++ b/src/applications/phriction/controller/PhrictionEditController.php @@ -109,6 +109,8 @@ final class PhrictionEditController $notes = null; $title = $content->getTitle(); $overwrite = false; + $v_cc = PhabricatorSubscribersQuery::loadSubscribersForPHID( + $document->getPHID()); if ($request->isFormPost()) { @@ -118,6 +120,7 @@ final class PhrictionEditController $current_version = $request->getInt('contentVersion'); $v_view = $request->getStr('viewPolicy'); $v_edit = $request->getStr('editPolicy'); + $v_cc = $request->getArr('cc'); $xactions = array(); $xactions[] = id(new PhrictionTransaction()) @@ -132,6 +135,9 @@ final class PhrictionEditController $xactions[] = id(new PhrictionTransaction()) ->setTransactionType(PhabricatorTransactions::TYPE_EDIT_POLICY) ->setNewValue($v_edit); + $xactions[] = id(new PhrictionTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS) + ->setNewValue(array('=' => $v_cc)); $editor = id(new PhrictionTransactionEditor()) ->setActor($viewer) @@ -222,6 +228,13 @@ final class PhrictionEditController ->setName('content') ->setID('document-textarea') ->setUser($viewer)) + ->appendControl( + id(new AphrontFormTokenizerControl()) + ->setLabel(pht('Subscribers')) + ->setName('cc') + ->setValue($v_cc) + ->setUser($viewer) + ->setDatasource(new PhabricatorMetaMTAMailableDatasource())) ->appendChild( id(new AphrontFormPolicyControl()) ->setName('viewPolicy') From 42ed5241204d05dfaa4edfb6018ef69aeead1c1a Mon Sep 17 00:00:00 2001 From: Chad Little Date: Thu, 3 Sep 2015 11:03:21 -0700 Subject: [PATCH 29/35] Break long strings in all remarkup areas Summary: Fixes T9264. I'm surprised this hasn't come up before, but any long string or URL in remarkup will overflow and cause remarkup areas to scroll. Prefer breaking these words. Test Plan: Review a timeline feed in Differential and a Ponder answer. {F768105} Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T9264 Differential Revision: https://secure.phabricator.com/D14018 --- resources/celerity/map.php | 6 +++--- webroot/rsrc/css/core/remarkup.css | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index a58be05060..c1f7fef804 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -7,7 +7,7 @@ */ return array( 'names' => array( - 'core.pkg.css' => '994de4ed', + 'core.pkg.css' => '31e4c86b', 'core.pkg.js' => '47dc9ebb', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '2de124c9', @@ -104,7 +104,7 @@ return array( 'rsrc/css/application/tokens/tokens.css' => '3d0f239e', 'rsrc/css/application/uiexample/example.css' => '528b19de', 'rsrc/css/core/core.css' => 'a76cefc9', - 'rsrc/css/core/remarkup.css' => '73fc4395', + 'rsrc/css/core/remarkup.css' => 'ef286a6e', 'rsrc/css/core/syntax.css' => '9fd11da8', 'rsrc/css/core/z-index.css' => '57ddcaa2', 'rsrc/css/diviner/diviner-shared.css' => '5a337049', @@ -737,7 +737,7 @@ return array( 'phabricator-object-selector-css' => '85ee8ce6', 'phabricator-phtize' => 'd254d646', 'phabricator-prefab' => '6920d200', - 'phabricator-remarkup-css' => '73fc4395', + 'phabricator-remarkup-css' => 'ef286a6e', 'phabricator-search-results-css' => '7dea472c', 'phabricator-shaped-request' => '7cbe244b', 'phabricator-side-menu-view-css' => 'bec2458e', diff --git a/webroot/rsrc/css/core/remarkup.css b/webroot/rsrc/css/core/remarkup.css index cba6ba0e98..207b69618c 100644 --- a/webroot/rsrc/css/core/remarkup.css +++ b/webroot/rsrc/css/core/remarkup.css @@ -4,6 +4,7 @@ .phabricator-remarkup { line-height: 1.51em; + word-break: break-word; } .phabricator-remarkup p { From 29399849c0edfc4bf4867d3605800f2caf5cf23a Mon Sep 17 00:00:00 2001 From: Chad Little Date: Thu, 3 Sep 2015 11:06:39 -0700 Subject: [PATCH 30/35] Scroll on overflow of property list (mobile) Summary: Fixes T9314. Functionally phui-status-list should get moved off a table, but that's another day. This catches many other possible issues. Test Plan: Review changes on a narrow browser. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T9314 Differential Revision: https://secure.phabricator.com/D14036 --- resources/celerity/map.php | 6 +++--- webroot/rsrc/css/phui/phui-property-list-view.css | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index c1f7fef804..f2aa401fc0 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -7,7 +7,7 @@ */ return array( 'names' => array( - 'core.pkg.css' => '31e4c86b', + 'core.pkg.css' => 'eb8c668d', 'core.pkg.js' => '47dc9ebb', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '2de124c9', @@ -141,7 +141,7 @@ return array( 'rsrc/css/phui/phui-object-item-list-view.css' => 'ab1bf393', 'rsrc/css/phui/phui-pager.css' => 'bea33d23', 'rsrc/css/phui/phui-pinboard-view.css' => '2495140e', - 'rsrc/css/phui/phui-property-list-view.css' => '318d4dea', + 'rsrc/css/phui/phui-property-list-view.css' => '03904f6b', 'rsrc/css/phui/phui-remarkup-preview.css' => '867f85b3', 'rsrc/css/phui/phui-spacing.css' => '042804d6', 'rsrc/css/phui/phui-status.css' => '888cedb8', @@ -794,7 +794,7 @@ return array( 'phui-object-item-list-view-css' => 'ab1bf393', 'phui-pager-css' => 'bea33d23', 'phui-pinboard-view-css' => '2495140e', - 'phui-property-list-view-css' => '318d4dea', + 'phui-property-list-view-css' => '03904f6b', 'phui-remarkup-preview-css' => '867f85b3', 'phui-spacing-css' => '042804d6', 'phui-status-list-view-css' => '888cedb8', diff --git a/webroot/rsrc/css/phui/phui-property-list-view.css b/webroot/rsrc/css/phui/phui-property-list-view.css index e59906a6a3..803d572421 100644 --- a/webroot/rsrc/css/phui/phui-property-list-view.css +++ b/webroot/rsrc/css/phui/phui-property-list-view.css @@ -167,6 +167,7 @@ width: auto; border: none; float: none; + overflow: auto; } .phui-property-list-actions { From cd2f9786bf8e2b5907a84cdd67fae71a208ac862 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 3 Sep 2015 12:15:30 -0700 Subject: [PATCH 31/35] Shuffle various parts of the config edit UI around Summary: Fixes T9339. - Don't show edit control for locked config at all. - Don't show a "Cancel" button either. - Change "Value" label to "Database Value" for non-custom config. - Highlight effective value. - Move examples under current state. - Tweak some formatting. Test Plan: {F777878} Reviewers: chad, avivey Reviewed By: chad, avivey Subscribers: avivey Maniphest Tasks: T9339 Differential Revision: https://secure.phabricator.com/D14054 --- resources/celerity/map.php | 18 +++--- .../PhabricatorConfigEditController.php | 55 +++++++++++-------- .../config/option/PhabricatorConfigOption.php | 6 +- .../css/application/config/config-options.css | 5 ++ 4 files changed, 50 insertions(+), 34 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index f2aa401fc0..3d1b1bf11b 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -7,7 +7,7 @@ */ return array( 'names' => array( - 'core.pkg.css' => 'eb8c668d', + 'core.pkg.css' => '994de4ed', 'core.pkg.js' => '47dc9ebb', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '2de124c9', @@ -40,7 +40,7 @@ return array( 'rsrc/css/application/calendar/calendar-icon.css' => 'c69aa59f', 'rsrc/css/application/chatlog/chatlog.css' => 'd295b020', 'rsrc/css/application/conduit/conduit-api.css' => '7bc725c4', - 'rsrc/css/application/config/config-options.css' => '7fedf08b', + 'rsrc/css/application/config/config-options.css' => '0ede4c9b', 'rsrc/css/application/config/config-template.css' => '8e6c6fcd', 'rsrc/css/application/config/config-welcome.css' => '6abd79be', 'rsrc/css/application/config/setup-issue.css' => 'db7e9c40', @@ -93,7 +93,7 @@ return array( 'rsrc/css/application/policy/policy-edit.css' => '815c66f7', 'rsrc/css/application/policy/policy-transaction-detail.css' => '82100a43', 'rsrc/css/application/policy/policy.css' => '957ea14c', - 'rsrc/css/application/ponder/ponder-view.css' => '7b0df4da', + 'rsrc/css/application/ponder/ponder-view.css' => 'bef48f86', 'rsrc/css/application/projects/project-icon.css' => '4e3eaa5a', 'rsrc/css/application/releeph/releeph-core.css' => '9b3c5733', 'rsrc/css/application/releeph/releeph-preview-branch.css' => 'b7a6f4a5', @@ -104,7 +104,7 @@ return array( 'rsrc/css/application/tokens/tokens.css' => '3d0f239e', 'rsrc/css/application/uiexample/example.css' => '528b19de', 'rsrc/css/core/core.css' => 'a76cefc9', - 'rsrc/css/core/remarkup.css' => 'ef286a6e', + 'rsrc/css/core/remarkup.css' => '73fc4395', 'rsrc/css/core/syntax.css' => '9fd11da8', 'rsrc/css/core/z-index.css' => '57ddcaa2', 'rsrc/css/diviner/diviner-shared.css' => '5a337049', @@ -141,7 +141,7 @@ return array( 'rsrc/css/phui/phui-object-item-list-view.css' => 'ab1bf393', 'rsrc/css/phui/phui-pager.css' => 'bea33d23', 'rsrc/css/phui/phui-pinboard-view.css' => '2495140e', - 'rsrc/css/phui/phui-property-list-view.css' => '03904f6b', + 'rsrc/css/phui/phui-property-list-view.css' => '318d4dea', 'rsrc/css/phui/phui-remarkup-preview.css' => '867f85b3', 'rsrc/css/phui/phui-spacing.css' => '042804d6', 'rsrc/css/phui/phui-status.css' => '888cedb8', @@ -505,7 +505,7 @@ return array( 'calendar-icon-css' => 'c69aa59f', 'changeset-view-manager' => '58562350', 'conduit-api-css' => '7bc725c4', - 'config-options-css' => '7fedf08b', + 'config-options-css' => '0ede4c9b', 'config-welcome-css' => '6abd79be', 'conpherence-durable-column-view' => '86396117', 'conpherence-menu-css' => 'f99fee4c', @@ -737,7 +737,7 @@ return array( 'phabricator-object-selector-css' => '85ee8ce6', 'phabricator-phtize' => 'd254d646', 'phabricator-prefab' => '6920d200', - 'phabricator-remarkup-css' => 'ef286a6e', + 'phabricator-remarkup-css' => '73fc4395', 'phabricator-search-results-css' => '7dea472c', 'phabricator-shaped-request' => '7cbe244b', 'phabricator-side-menu-view-css' => 'bec2458e', @@ -794,7 +794,7 @@ return array( 'phui-object-item-list-view-css' => 'ab1bf393', 'phui-pager-css' => 'bea33d23', 'phui-pinboard-view-css' => '2495140e', - 'phui-property-list-view-css' => '03904f6b', + 'phui-property-list-view-css' => '318d4dea', 'phui-remarkup-preview-css' => '867f85b3', 'phui-spacing-css' => '042804d6', 'phui-status-list-view-css' => '888cedb8', @@ -811,7 +811,7 @@ return array( 'policy-css' => '957ea14c', 'policy-edit-css' => '815c66f7', 'policy-transaction-detail-css' => '82100a43', - 'ponder-view-css' => '7b0df4da', + 'ponder-view-css' => 'bef48f86', 'project-icon-css' => '4e3eaa5a', 'raphael-core' => '51ee6b43', 'raphael-g' => '40dde778', diff --git a/src/applications/config/controller/PhabricatorConfigEditController.php b/src/applications/config/controller/PhabricatorConfigEditController.php index 90547f41f7..370ba01f37 100644 --- a/src/applications/config/controller/PhabricatorConfigEditController.php +++ b/src/applications/config/controller/PhabricatorConfigEditController.php @@ -122,7 +122,7 @@ final class PhabricatorConfigEditController ->appendChild(phutil_tag('p', array(), $msg)); } - if ($option->getHidden()) { + if ($option->getHidden() || $option->getLocked()) { $control = null; } else { $control = $this->renderControl( @@ -164,14 +164,20 @@ final class PhabricatorConfigEditController $form ->appendChild($control); - $submit_control = id(new AphrontFormSubmitControl()) - ->addCancelButton($done_uri); if (!$option->getLocked()) { - $submit_control->setValue(pht('Save Config Entry')); + $form->appendChild( + id(new AphrontFormSubmitControl()) + ->addCancelButton($done_uri) + ->setValue(pht('Save Config Entry'))); } - $form->appendChild($submit_control); + if (!$option->getHidden()) { + $form->appendChild( + id(new AphrontFormMarkupControl()) + ->setLabel(pht('Current Configuration')) + ->setValue($this->renderDefaults($option, $config_entry))); + } $examples = $this->renderExamples($option); if ($examples) { @@ -181,13 +187,6 @@ final class PhabricatorConfigEditController ->setValue($examples)); } - if (!$option->getHidden()) { - $form->appendChild( - id(new AphrontFormMarkupControl()) - ->setLabel(pht('Default')) - ->setValue($this->renderDefaults($option, $config_entry))); - } - $title = pht('Edit %s', $key); $short = pht('Edit'); @@ -438,16 +437,12 @@ final class PhabricatorConfigEditController } $control - ->setLabel(pht('Value')) + ->setLabel(pht('Database Value')) ->setError($e_value) ->setValue($display_value) ->setName('value'); } - if ($option->getLocked()) { - $control->setDisabled(true); - } - return $control; } @@ -501,25 +496,41 @@ final class PhabricatorConfigEditController phutil_tag('th', array(), pht('Source')), phutil_tag('th', array(), pht('Value')), )); + + $is_effective_value = true; foreach ($stack as $key => $source) { + $row_classes = array( + 'column-labels', + ); + $value = $source->getKeys( array( $option->getKey(), )); if (!array_key_exists($option->getKey(), $value)) { - $value = phutil_tag('em', array(), pht('(empty)')); + $value = phutil_tag('em', array(), pht('(No Value Configured)')); } else { $value = $this->getDisplayValue( $option, $entry, $value[$option->getKey()]); + + if ($is_effective_value) { + $is_effective_value = false; + $row_classes[] = 'config-options-effective-value'; + } } - $table[] = phutil_tag('tr', array('class' => 'column-labels'), array( - phutil_tag('th', array(), $source->getName()), - phutil_tag('td', array(), $value), - )); + $table[] = phutil_tag( + 'tr', + array( + 'class' => implode(' ', $row_classes), + ), + array( + phutil_tag('th', array(), $source->getName()), + phutil_tag('td', array(), $value), + )); } require_celerity_resource('config-options-css'); diff --git a/src/applications/config/option/PhabricatorConfigOption.php b/src/applications/config/option/PhabricatorConfigOption.php index e1d71416e7..d10c450192 100644 --- a/src/applications/config/option/PhabricatorConfigOption.php +++ b/src/applications/config/option/PhabricatorConfigOption.php @@ -76,9 +76,9 @@ final class PhabricatorConfigOption } return pht( 'This configuration is locked and can not be edited from the web '. - 'interface. Use `%s` in `%s` to edit it.', - './bin/config', - 'phabricator/'); + 'interface. Use %s in %s to edit it.', + phutil_tag('tt', array(), './bin/config'), + phutil_tag('tt', array(), 'phabricator/')); } public function addExample($value, $description) { diff --git a/webroot/rsrc/css/application/config/config-options.css b/webroot/rsrc/css/application/config/config-options.css index c7b6d55512..67c005094c 100644 --- a/webroot/rsrc/css/application/config/config-options.css +++ b/webroot/rsrc/css/application/config/config-options.css @@ -49,3 +49,8 @@ .config-options-current-value span { color: {$greytext}; } + +.config-options-effective-value, +.config-option-table .config-options-effective-value th { + background: {$lightyellow}; +} From 7641c9c7bc012487c24b7f15708e82c613aa1015 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Fri, 4 Sep 2015 10:34:25 -0700 Subject: [PATCH 32/35] Build LauncherButton for PHUIObjectItemView Summary: There are a handful of places I've been wanting to use a button here. Adds that ability and uses in app launcher. Test Plan: Test Applicatons->Launcher at desktop, mobile, tablet breakpoints {F780453} Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D14059 --- resources/celerity/map.php | 18 ++++----- .../meta/query/PhabricatorAppSearchEngine.php | 39 +++++++++---------- src/view/phui/PHUIObjectItemView.php | 18 +++++++++ .../css/phui/phui-object-item-list-view.css | 13 +++++++ 4 files changed, 58 insertions(+), 30 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 3d1b1bf11b..21e60864f4 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -7,7 +7,7 @@ */ return array( 'names' => array( - 'core.pkg.css' => '994de4ed', + 'core.pkg.css' => 'a40c0b28', 'core.pkg.js' => '47dc9ebb', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '2de124c9', @@ -93,7 +93,7 @@ return array( 'rsrc/css/application/policy/policy-edit.css' => '815c66f7', 'rsrc/css/application/policy/policy-transaction-detail.css' => '82100a43', 'rsrc/css/application/policy/policy.css' => '957ea14c', - 'rsrc/css/application/ponder/ponder-view.css' => 'bef48f86', + 'rsrc/css/application/ponder/ponder-view.css' => '7b0df4da', 'rsrc/css/application/projects/project-icon.css' => '4e3eaa5a', 'rsrc/css/application/releeph/releeph-core.css' => '9b3c5733', 'rsrc/css/application/releeph/releeph-preview-branch.css' => 'b7a6f4a5', @@ -104,7 +104,7 @@ return array( 'rsrc/css/application/tokens/tokens.css' => '3d0f239e', 'rsrc/css/application/uiexample/example.css' => '528b19de', 'rsrc/css/core/core.css' => 'a76cefc9', - 'rsrc/css/core/remarkup.css' => '73fc4395', + 'rsrc/css/core/remarkup.css' => 'ef286a6e', 'rsrc/css/core/syntax.css' => '9fd11da8', 'rsrc/css/core/z-index.css' => '57ddcaa2', 'rsrc/css/diviner/diviner-shared.css' => '5a337049', @@ -138,10 +138,10 @@ return array( 'rsrc/css/phui/phui-info-view.css' => '5b16bac6', 'rsrc/css/phui/phui-list.css' => '125599df', 'rsrc/css/phui/phui-object-box.css' => '407eaf5a', - 'rsrc/css/phui/phui-object-item-list-view.css' => 'ab1bf393', + 'rsrc/css/phui/phui-object-item-list-view.css' => '26c30d3f', 'rsrc/css/phui/phui-pager.css' => 'bea33d23', 'rsrc/css/phui/phui-pinboard-view.css' => '2495140e', - 'rsrc/css/phui/phui-property-list-view.css' => '318d4dea', + 'rsrc/css/phui/phui-property-list-view.css' => '03904f6b', 'rsrc/css/phui/phui-remarkup-preview.css' => '867f85b3', 'rsrc/css/phui/phui-spacing.css' => '042804d6', 'rsrc/css/phui/phui-status.css' => '888cedb8', @@ -737,7 +737,7 @@ return array( 'phabricator-object-selector-css' => '85ee8ce6', 'phabricator-phtize' => 'd254d646', 'phabricator-prefab' => '6920d200', - 'phabricator-remarkup-css' => '73fc4395', + 'phabricator-remarkup-css' => 'ef286a6e', 'phabricator-search-results-css' => '7dea472c', 'phabricator-shaped-request' => '7cbe244b', 'phabricator-side-menu-view-css' => 'bec2458e', @@ -791,10 +791,10 @@ return array( 'phui-inline-comment-view-css' => '0fdb3667', 'phui-list-view-css' => '125599df', 'phui-object-box-css' => '407eaf5a', - 'phui-object-item-list-view-css' => 'ab1bf393', + 'phui-object-item-list-view-css' => '26c30d3f', 'phui-pager-css' => 'bea33d23', 'phui-pinboard-view-css' => '2495140e', - 'phui-property-list-view-css' => '318d4dea', + 'phui-property-list-view-css' => '03904f6b', 'phui-remarkup-preview-css' => '867f85b3', 'phui-spacing-css' => '042804d6', 'phui-status-list-view-css' => '888cedb8', @@ -811,7 +811,7 @@ return array( 'policy-css' => '957ea14c', 'policy-edit-css' => '815c66f7', 'policy-transaction-detail-css' => '82100a43', - 'ponder-view-css' => 'bef48f86', + 'ponder-view-css' => '7b0df4da', 'project-icon-css' => '4e3eaa5a', 'raphael-core' => '51ee6b43', 'raphael-g' => '40dde778', diff --git a/src/applications/meta/query/PhabricatorAppSearchEngine.php b/src/applications/meta/query/PhabricatorAppSearchEngine.php index c67aa78e8a..9dcc0a4399 100644 --- a/src/applications/meta/query/PhabricatorAppSearchEngine.php +++ b/src/applications/meta/query/PhabricatorAppSearchEngine.php @@ -226,39 +226,36 @@ final class PhabricatorAppSearchEngine ), ''); - $description = phutil_tag( - 'div', - array( - 'style' => 'white-space: nowrap; '. - 'overflow: hidden; '. - 'text-overflow: ellipsis;', - ), - $application->getShortDescription()); + $description = $application->getShortDescription(); + + $configure = id(new PHUIButtonView()) + ->setTag('a') + ->setHref('/applications/view/'.get_class($application).'/') + ->setText(pht('Configure')) + ->setColor(PHUIButtonView::GREY); + + $name = $application->getName(); + if ($application->isPrototype()) { + $name = $name.' '.pht('(Prototype)'); + } $item = id(new PHUIObjectItemView()) - ->setHeader($application->getName()) + ->setHeader($name) ->setImageIcon($icon_view) - ->addAttribute($description) - ->addAction( - id(new PHUIListItemView()) - ->setName(pht('Help/Options')) - ->setIcon('fa-cog') - ->setHref('/applications/view/'.get_class($application).'/')); + ->setSubhead($description) + ->setLaunchButton($configure); if ($application->getBaseURI() && $application->isInstalled()) { $item->setHref($application->getBaseURI()); } if (!$application->isInstalled()) { - $item->addIcon('fa-times', pht('Uninstalled')); - } - - if ($application->isPrototype()) { - $item->addIcon('fa-bomb grey', pht('Prototype')); + $item->addAttribute(pht('Uninstalled')); + $item->setDisabled(true); } if (!$application->isFirstParty()) { - $item->addIcon('fa-puzzle-piece', pht('Extension')); + $item->addAttribute(pht('Extension')); } $list->addItem($item); diff --git a/src/view/phui/PHUIObjectItemView.php b/src/view/phui/PHUIObjectItemView.php index 973592a94c..5da85777b0 100644 --- a/src/view/phui/PHUIObjectItemView.php +++ b/src/view/phui/PHUIObjectItemView.php @@ -26,6 +26,7 @@ final class PHUIObjectItemView extends AphrontTagView { private $badge; private $countdownNum; private $countdownNoun; + private $launchButton; const AGE_FRESH = 'fresh'; const AGE_STALE = 'stale'; @@ -260,6 +261,12 @@ final class PHUIObjectItemView extends AphrontTagView { return $this; } + public function setLaunchButton(PHUIButtonView $button) { + $button->setSize(PHUIButtonView::SMALL); + $this->launchButton = $button; + return $this; + } + protected function getTagName() { return 'li'; } @@ -652,6 +659,17 @@ final class PHUIObjectItemView extends AphrontTagView { )); } + if ($this->launchButton) { + $column2 = phutil_tag( + 'div', + array( + 'class' => 'phui-object-item-col2 phui-object-item-launch-button', + ), + array( + $this->launchButton, + )); + } + $table = phutil_tag( 'div', array( diff --git a/webroot/rsrc/css/phui/phui-object-item-list-view.css b/webroot/rsrc/css/phui/phui-object-item-list-view.css index ecaaa32f70..1122b0f7ff 100644 --- a/webroot/rsrc/css/phui/phui-object-item-list-view.css +++ b/webroot/rsrc/css/phui/phui-object-item-list-view.css @@ -747,3 +747,16 @@ ul.phui-object-item-list-view .phui-object-item-selected .phui-object-item-with-image .phui-object-item-content-box { margin-left: 46px; } + +/* - Launcher Button -------------------------------------------------------- */ + +.phui-object-item-col2.phui-object-item-launch-button { + text-align: right; + vertical-align: middle; + padding-right: 4px; +} + +.device-phone .phui-object-item-col2.phui-object-item-launch-button { + padding: 0 8px 8px; + text-align: left; +} From 6f372943dbefea5995c75c20773785e164c94902 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 4 Sep 2015 10:34:32 -0700 Subject: [PATCH 33/35] Add support for temporary files to `file.allocate` Summary: Ref T7148. I can do most of the export stuff by only modifying the Instances codebase, but want to upload all the backups and exports as temporary files and can't currently do this via the API. Make the necessary API changes so that the export workflow can use them when it gets built out. Test Plan: See next diff. Uploaded files with `arc upload --temporary` and saw them upload as temporary files. Reviewers: chad Reviewed By: chad Maniphest Tasks: T7148 Differential Revision: https://secure.phabricator.com/D14055 --- resources/celerity/map.php | 6 +++--- .../files/conduit/FileAllocateConduitAPIMethod.php | 6 ++++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 21e60864f4..cc948cc4f5 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -7,7 +7,7 @@ */ return array( 'names' => array( - 'core.pkg.css' => 'a40c0b28', + 'core.pkg.css' => 'eb8c668d', 'core.pkg.js' => '47dc9ebb', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '2de124c9', @@ -138,7 +138,7 @@ return array( 'rsrc/css/phui/phui-info-view.css' => '5b16bac6', 'rsrc/css/phui/phui-list.css' => '125599df', 'rsrc/css/phui/phui-object-box.css' => '407eaf5a', - 'rsrc/css/phui/phui-object-item-list-view.css' => '26c30d3f', + 'rsrc/css/phui/phui-object-item-list-view.css' => 'ab1bf393', 'rsrc/css/phui/phui-pager.css' => 'bea33d23', 'rsrc/css/phui/phui-pinboard-view.css' => '2495140e', 'rsrc/css/phui/phui-property-list-view.css' => '03904f6b', @@ -791,7 +791,7 @@ return array( 'phui-inline-comment-view-css' => '0fdb3667', 'phui-list-view-css' => '125599df', 'phui-object-box-css' => '407eaf5a', - 'phui-object-item-list-view-css' => '26c30d3f', + 'phui-object-item-list-view-css' => 'ab1bf393', 'phui-pager-css' => 'bea33d23', 'phui-pinboard-view-css' => '2495140e', 'phui-property-list-view-css' => '03904f6b', diff --git a/src/applications/files/conduit/FileAllocateConduitAPIMethod.php b/src/applications/files/conduit/FileAllocateConduitAPIMethod.php index d21c26fe11..ff3a0c80da 100644 --- a/src/applications/files/conduit/FileAllocateConduitAPIMethod.php +++ b/src/applications/files/conduit/FileAllocateConduitAPIMethod.php @@ -17,6 +17,7 @@ final class FileAllocateConduitAPIMethod 'contentLength' => 'int', 'contentHash' => 'optional string', 'viewPolicy' => 'optional string', + 'deleteAfterEpoch' => 'optional int', ); } @@ -39,6 +40,11 @@ final class FileAllocateConduitAPIMethod 'isExplicitUpload' => true, ); + $ttl = $request->getValue('deleteAfterEpoch'); + if ($ttl) { + $properties['ttl'] = $ttl; + } + $file = null; if ($hash) { $file = PhabricatorFile::newFileFromContentHash( From 5dccc14bbf46b0cfaabf1d595c5f446c6e0302c1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 4 Sep 2015 10:34:39 -0700 Subject: [PATCH 34/35] Modularize generation of supplemental login messages Summary: Ref T9346. This mostly allows us to give users additional advice based on which instance they are trying to log in to in the Phacility cluster. It's also slightly more flexible than `auth.login-message` was, and maybe we'll add some more hooks here eventually. This feels like it's a sidegrade in complexity rather than really an improvement, but not too terrible. Test Plan: - Wrote the custom handler in T9346 to replicate old config functionality. - Wrote a smart handler for Phacility that can provide context-sensitive messages based on which OAuth client you're trying to use. See new message box at top (implementation in next diff): {F780375} Reviewers: chad Reviewed By: chad Maniphest Tasks: T9346 Differential Revision: https://secure.phabricator.com/D14057 --- src/__phutil_library_map__.php | 2 ++ .../PhabricatorAuthStartController.php | 20 +++++++++-- .../handler/PhabricatorAuthLoginHandler.php | 36 +++++++++++++++++++ .../PhabricatorExtraConfigSetupCheck.php | 4 +++ ...PhabricatorAuthenticationConfigOptions.php | 8 ----- 5 files changed, 59 insertions(+), 11 deletions(-) create mode 100644 src/applications/auth/handler/PhabricatorAuthLoginHandler.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index aba0b8a1c5..f01d82e40f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1613,6 +1613,7 @@ phutil_register_library_map(array( 'PhabricatorAuthLinkController' => 'applications/auth/controller/PhabricatorAuthLinkController.php', 'PhabricatorAuthListController' => 'applications/auth/controller/config/PhabricatorAuthListController.php', 'PhabricatorAuthLoginController' => 'applications/auth/controller/PhabricatorAuthLoginController.php', + 'PhabricatorAuthLoginHandler' => 'applications/auth/handler/PhabricatorAuthLoginHandler.php', 'PhabricatorAuthManagementCachePKCS8Workflow' => 'applications/auth/management/PhabricatorAuthManagementCachePKCS8Workflow.php', 'PhabricatorAuthManagementLDAPWorkflow' => 'applications/auth/management/PhabricatorAuthManagementLDAPWorkflow.php', 'PhabricatorAuthManagementListFactorsWorkflow' => 'applications/auth/management/PhabricatorAuthManagementListFactorsWorkflow.php', @@ -5458,6 +5459,7 @@ phutil_register_library_map(array( 'PhabricatorAuthLinkController' => 'PhabricatorAuthController', 'PhabricatorAuthListController' => 'PhabricatorAuthProviderConfigController', 'PhabricatorAuthLoginController' => 'PhabricatorAuthController', + 'PhabricatorAuthLoginHandler' => 'Phobject', 'PhabricatorAuthManagementCachePKCS8Workflow' => 'PhabricatorAuthManagementWorkflow', 'PhabricatorAuthManagementLDAPWorkflow' => 'PhabricatorAuthManagementWorkflow', 'PhabricatorAuthManagementListFactorsWorkflow' => 'PhabricatorAuthManagementWorkflow', diff --git a/src/applications/auth/controller/PhabricatorAuthStartController.php b/src/applications/auth/controller/PhabricatorAuthStartController.php index 01578446a5..ff7c18091f 100644 --- a/src/applications/auth/controller/PhabricatorAuthStartController.php +++ b/src/applications/auth/controller/PhabricatorAuthStartController.php @@ -163,8 +163,22 @@ final class PhabricatorAuthStartController $button_columns); } - $login_message = PhabricatorEnv::getEnvConfig('auth.login-message'); - $login_message = phutil_safe_html($login_message); + $handlers = PhabricatorAuthLoginHandler::getAllHandlers(); + + $delegating_controller = $this->getDelegatingController(); + + $header = array(); + foreach ($handlers as $handler) { + $handler = clone $handler; + + $handler->setRequest($request); + + if ($delegating_controller) { + $handler->setDelegatingController($delegating_controller); + } + + $header[] = $handler->getAuthLoginHeaderContent(); + } $invite_message = null; if ($invite) { @@ -178,7 +192,7 @@ final class PhabricatorAuthStartController return $this->buildApplicationPage( array( $crumbs, - $login_message, + $header, $invite_message, $out, ), diff --git a/src/applications/auth/handler/PhabricatorAuthLoginHandler.php b/src/applications/auth/handler/PhabricatorAuthLoginHandler.php new file mode 100644 index 0000000000..eabbf91843 --- /dev/null +++ b/src/applications/auth/handler/PhabricatorAuthLoginHandler.php @@ -0,0 +1,36 @@ +delegatingController = $controller; + return $this; + } + + final public function getDelegatingController() { + return $this->delegatingController; + } + + final public function setRequest(AphrontRequest $request) { + $this->request = $request; + return $this; + } + + final public function getRequest() { + return $this->request; + } + + final public static function getAllHandlers() { + return id(new PhutilClassMapQuery()) + ->setAncestorClass(__CLASS__) + ->execute(); + } +} diff --git a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php index a482f4d073..b73f145033 100644 --- a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php +++ b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php @@ -276,6 +276,10 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck { 'Impersonating users over the API is no longer supported.'), 'feed.public' => pht('The framable public feed is no longer supported.'), + + 'auth.login-message' => pht( + 'This configuration option has been replaced with a modular '. + 'handler. See T9346.'), ); return $ancient_config; diff --git a/src/applications/config/option/PhabricatorAuthenticationConfigOptions.php b/src/applications/config/option/PhabricatorAuthenticationConfigOptions.php index a965d16f42..8092f52ff8 100644 --- a/src/applications/config/option/PhabricatorAuthenticationConfigOptions.php +++ b/src/applications/config/option/PhabricatorAuthenticationConfigOptions.php @@ -73,14 +73,6 @@ final class PhabricatorAuthenticationConfigOptions ->addExample( "yourcompany.com\nmail.yourcompany.com", pht('Valid Setting')), - $this->newOption('auth.login-message', 'string', null) - ->setLocked(true) - ->setSummary(pht('A block of HTML displayed on the login screen.')) - ->setDescription( - pht( - "You can provide an arbitrary block of HTML here, which will ". - "appear on the login screen. Normally, you'd use this to provide ". - "login or registration instructions to users.")), $this->newOption('account.editable', 'bool', true) ->setBoolOptions( array( From 76665f725bc90c5122b4f9f5cf80b75fedbdf885 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 4 Sep 2015 15:11:25 -0700 Subject: [PATCH 35/35] Correct a bad Herald migration Summary: One of the migrations in rPa335004a91 (`20150730.herald.5.sql`) incorrectly swapped "add" and "add blocking" Differential Herald rules. Swap any rules last modified before this patch was applied back. This is the best we can do without possibly overwriting more recent, intentional data. I'll issue some guidance on this in the changelog. Test Plan: - Made a rule, ran patch, no change. - Changed rule modified time to a few months ago, ran patch, saw swap from non-blocking to blocking. Reviewers: chad Reviewed By: chad Differential Revision: https://secure.phabricator.com/D14061 --- .../sql/autopatches/20150904.herald.1.sql | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 resources/sql/autopatches/20150904.herald.1.sql diff --git a/resources/sql/autopatches/20150904.herald.1.sql b/resources/sql/autopatches/20150904.herald.1.sql new file mode 100644 index 0000000000..17f945cc30 --- /dev/null +++ b/resources/sql/autopatches/20150904.herald.1.sql @@ -0,0 +1,52 @@ +/* The "20150730.herald.5.sql" patch incorrectly swapped blocking and + non-blocking "Add Reviewer" rules. This swaps back any rules which + were last modified before the patch was applied. */ + +UPDATE {$NAMESPACE}_herald.herald_action a + JOIN {$NAMESPACE}_herald.herald_rule r + ON a.ruleID = r.id + SET a.action = 'differential.reviewers.blocking.tmp' + WHERE a.action = 'differential.reviewers.add' + AND r.dateModified <= + (SELECT applied FROM {$NAMESPACE}_meta_data.patch_status + WHERE patch = 'phabricator:20150730.herald.5.sql'); + +UPDATE {$NAMESPACE}_herald.herald_action a + JOIN {$NAMESPACE}_herald.herald_rule r + ON a.ruleID = r.id + SET a.action = 'differential.reviewers.add' + WHERE a.action = 'differential.reviewers.blocking' + AND r.dateModified <= + (SELECT applied FROM {$NAMESPACE}_meta_data.patch_status + WHERE patch = 'phabricator:20150730.herald.5.sql'); + +UPDATE {$NAMESPACE}_herald.herald_action a + JOIN {$NAMESPACE}_herald.herald_rule r + ON a.ruleID = r.id + SET a.action = 'differential.reviewers.blocking' + WHERE a.action = 'differential.reviewers.blocking.tmp'; + + +UPDATE {$NAMESPACE}_herald.herald_action a + JOIN {$NAMESPACE}_herald.herald_rule r + ON a.ruleID = r.id + SET a.action = 'differential.reviewers.self.blocking.tmp' + WHERE a.action = 'differential.reviewers.self.add' + AND r.dateModified <= + (SELECT applied FROM {$NAMESPACE}_meta_data.patch_status + WHERE patch = 'phabricator:20150730.herald.5.sql'); + +UPDATE {$NAMESPACE}_herald.herald_action a + JOIN {$NAMESPACE}_herald.herald_rule r + ON a.ruleID = r.id + SET a.action = 'differential.reviewers.self.add' + WHERE a.action = 'differential.reviewers.self.blocking' + AND r.dateModified <= + (SELECT applied FROM {$NAMESPACE}_meta_data.patch_status + WHERE patch = 'phabricator:20150730.herald.5.sql'); + +UPDATE {$NAMESPACE}_herald.herald_action a + JOIN {$NAMESPACE}_herald.herald_rule r + ON a.ruleID = r.id + SET a.action = 'differential.reviewers.self.blocking' + WHERE a.action = 'differential.reviewers.self.blocking.tmp';