From 55532b3f748939c4c4c38b5fe0976c35ae6ebcb6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 9 Mar 2021 14:38:03 -0800 Subject: [PATCH 01/13] Add a very basic "auditors" attachment to "differential.commit.search" Summary: Ref T13631. For now, this only shows the auditor PHID. The current status constants could use some cleanup before they're exposed. Test Plan: Queried with "auditors" attachment, saw basic auditor information. Maniphest Tasks: T13631 Differential Revision: https://secure.phabricator.com/D21597 --- src/__phutil_library_map__.php | 2 ++ ...iffusionAuditorsSearchEngineAttachment.php | 33 +++++++++++++++++++ .../storage/PhabricatorRepositoryCommit.php | 5 ++- 3 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 src/applications/diffusion/engineextension/DiffusionAuditorsSearchEngineAttachment.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 8c6c45c326..226adfdc69 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -724,6 +724,7 @@ phutil_register_library_map(array( 'DiffusionAuditorsAddAuditorsHeraldAction' => 'applications/diffusion/herald/DiffusionAuditorsAddAuditorsHeraldAction.php', 'DiffusionAuditorsAddSelfHeraldAction' => 'applications/diffusion/herald/DiffusionAuditorsAddSelfHeraldAction.php', 'DiffusionAuditorsHeraldAction' => 'applications/diffusion/herald/DiffusionAuditorsHeraldAction.php', + 'DiffusionAuditorsSearchEngineAttachment' => 'applications/diffusion/engineextension/DiffusionAuditorsSearchEngineAttachment.php', 'DiffusionBlameConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionBlameConduitAPIMethod.php', 'DiffusionBlameController' => 'applications/diffusion/controller/DiffusionBlameController.php', 'DiffusionBlameQuery' => 'applications/diffusion/query/blame/DiffusionBlameQuery.php', @@ -6843,6 +6844,7 @@ phutil_register_library_map(array( 'DiffusionAuditorsAddAuditorsHeraldAction' => 'DiffusionAuditorsHeraldAction', 'DiffusionAuditorsAddSelfHeraldAction' => 'DiffusionAuditorsHeraldAction', 'DiffusionAuditorsHeraldAction' => 'HeraldAction', + 'DiffusionAuditorsSearchEngineAttachment' => 'PhabricatorSearchEngineAttachment', 'DiffusionBlameConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', 'DiffusionBlameController' => 'DiffusionController', 'DiffusionBlameQuery' => 'DiffusionQuery', diff --git a/src/applications/diffusion/engineextension/DiffusionAuditorsSearchEngineAttachment.php b/src/applications/diffusion/engineextension/DiffusionAuditorsSearchEngineAttachment.php new file mode 100644 index 0000000000..a244241449 --- /dev/null +++ b/src/applications/diffusion/engineextension/DiffusionAuditorsSearchEngineAttachment.php @@ -0,0 +1,33 @@ +needAuditRequests(true); + } + + public function getAttachmentForObject($object, $data, $spec) { + $auditors = $object->getAudits(); + + $list = array(); + foreach ($auditors as $auditor) { + $list[] = array( + 'auditorPHID' => $auditor->getAuditorPHID(), + ); + } + + return array( + 'auditors' => $list, + ); + } + +} diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index 6870ca2025..f0c7d6abf8 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -945,7 +945,10 @@ final class PhabricatorRepositoryCommit } public function getConduitSearchAttachments() { - return array(); + return array( + id(new DiffusionAuditorsSearchEngineAttachment()) + ->setAttachmentKey('auditors'), + ); } From 2636d84d0c24cbcf62fa70d4d0ed112844854e13 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 9 Mar 2021 15:11:28 -0800 Subject: [PATCH 02/13] Remove very old Audit status constants and AuditRequest data Summary: Ref T13631. See that task for discussion. - "NONE": Probably never used? - "CC": Obsoleted by subscribers. - "AUDIT_NOT_REQUIRED": For Owners packages, obsoleted by edges. - "CLOSED": For "Close Audit", obsoleted by "Request Verification". Test Plan: - Grepped for constants, browsed Diffusion. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13631 Differential Revision: https://secure.phabricator.com/D21598 --- .../20210309.auditors.01.status.sql | 5 ++++ .../PhabricatorAuditStatusConstants.php | 18 --------------- .../audit/editor/PhabricatorAuditEditor.php | 7 ------ .../controller/DiffusionCommitController.php | 4 ---- .../herald/DiffusionAuditorsHeraldAction.php | 4 +--- .../DiffusionCommitAuditorsHeraldField.php | 6 +++-- .../PhabricatorRepositoryAuditRequest.php | 23 ------------------- 7 files changed, 10 insertions(+), 57 deletions(-) create mode 100644 resources/sql/autopatches/20210309.auditors.01.status.sql diff --git a/resources/sql/autopatches/20210309.auditors.01.status.sql b/resources/sql/autopatches/20210309.auditors.01.status.sql new file mode 100644 index 0000000000..731ce3ca44 --- /dev/null +++ b/resources/sql/autopatches/20210309.auditors.01.status.sql @@ -0,0 +1,5 @@ +UPDATE {$NAMESPACE}_repository.repository_auditrequest + SET auditStatus = 'accepted' WHERE auditStatus = 'closed'; + +DELETE FROM {$NAMESPACE}_repository.repository_auditrequest + WHERE auditStatus IN ('', 'cc', 'audit-not-required'); diff --git a/src/applications/audit/constants/PhabricatorAuditStatusConstants.php b/src/applications/audit/constants/PhabricatorAuditStatusConstants.php index 920cfd875e..54eb332124 100644 --- a/src/applications/audit/constants/PhabricatorAuditStatusConstants.php +++ b/src/applications/audit/constants/PhabricatorAuditStatusConstants.php @@ -2,27 +2,19 @@ final class PhabricatorAuditStatusConstants extends Phobject { - const NONE = ''; - const AUDIT_NOT_REQUIRED = 'audit-not-required'; const AUDIT_REQUIRED = 'audit-required'; const CONCERNED = 'concerned'; const ACCEPTED = 'accepted'; const AUDIT_REQUESTED = 'requested'; const RESIGNED = 'resigned'; - const CLOSED = 'closed'; - const CC = 'cc'; public static function getStatusNameMap() { $map = array( - self::NONE => pht('Not Applicable'), - self::AUDIT_NOT_REQUIRED => pht('Audit Not Required'), self::AUDIT_REQUIRED => pht('Audit Required'), self::CONCERNED => pht('Concern Raised'), self::ACCEPTED => pht('Accepted'), self::AUDIT_REQUESTED => pht('Audit Requested'), self::RESIGNED => pht('Resigned'), - self::CLOSED => pht('Closed'), - self::CC => pht("Was CC'd"), ); return $map; @@ -51,12 +43,6 @@ final class PhabricatorAuditStatusConstants extends Phobject { case self::ACCEPTED: $color = 'green'; break; - case self::AUDIT_NOT_REQUIRED: - $color = 'blue'; - break; - case self::CLOSED: - $color = 'dark'; - break; case self::RESIGNED: $color = 'grey'; break; @@ -69,9 +55,6 @@ final class PhabricatorAuditStatusConstants extends Phobject { public static function getStatusIcon($code) { switch ($code) { - case self::AUDIT_NOT_REQUIRED: - $icon = PHUIStatusItemView::ICON_OPEN; - break; case self::AUDIT_REQUIRED: case self::AUDIT_REQUESTED: $icon = PHUIStatusItemView::ICON_WARNING; @@ -80,7 +63,6 @@ final class PhabricatorAuditStatusConstants extends Phobject { $icon = PHUIStatusItemView::ICON_REJECT; break; case self::ACCEPTED: - case self::CLOSED: $icon = PHUIStatusItemView::ICON_ACCEPT; break; case self::RESIGNED: diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index 98c0d92624..81317fd47e 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -179,7 +179,6 @@ final class PhabricatorAuditEditor $object->attachAudits($commit->getAudits()); $status_concerned = PhabricatorAuditStatusConstants::CONCERNED; - $status_closed = PhabricatorAuditStatusConstants::CLOSED; $status_resigned = PhabricatorAuditStatusConstants::RESIGNED; $status_accepted = PhabricatorAuditStatusConstants::ACCEPTED; $status_concerned = PhabricatorAuditStatusConstants::CONCERNED; @@ -491,12 +490,6 @@ final class PhabricatorAuditEditor } foreach ($object->getAudits() as $audit) { - if (!$audit->isInteresting()) { - // Don't send mail to uninteresting auditors, like packages which - // own this code but which audits have not triggered for. - continue; - } - if (!$audit->isResigned()) { $phids[] = $audit->getAuditorPHID(); } diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index 473e8911ac..6037781edc 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -598,10 +598,6 @@ final class DiffusionCommitController extends DiffusionController { $other_requests = array(); foreach ($audit_requests as $audit_request) { - if (!$audit_request->isInteresting()) { - continue; - } - if ($audit_request->isUser()) { $user_requests[] = $audit_request; } else { diff --git a/src/applications/diffusion/herald/DiffusionAuditorsHeraldAction.php b/src/applications/diffusion/herald/DiffusionAuditorsHeraldAction.php index 4ec23b1faa..bd75799f03 100644 --- a/src/applications/diffusion/herald/DiffusionAuditorsHeraldAction.php +++ b/src/applications/diffusion/herald/DiffusionAuditorsHeraldAction.php @@ -38,9 +38,7 @@ abstract class DiffusionAuditorsHeraldAction $current = array(); foreach ($auditors as $auditor) { - if ($auditor->isInteresting()) { - $current[] = $auditor->getAuditorPHID(); - } + $current[] = $auditor->getAuditorPHID(); } $allowed_types = array( diff --git a/src/applications/diffusion/herald/DiffusionCommitAuditorsHeraldField.php b/src/applications/diffusion/herald/DiffusionCommitAuditorsHeraldField.php index 8deee3417e..5f0da133f8 100644 --- a/src/applications/diffusion/herald/DiffusionCommitAuditorsHeraldField.php +++ b/src/applications/diffusion/herald/DiffusionCommitAuditorsHeraldField.php @@ -22,9 +22,11 @@ final class DiffusionCommitAuditorsHeraldField $phids = array(); foreach ($audits as $audit) { - if ($audit->isActiveAudit()) { - $phids[] = $audit->getAuditorPHID(); + if ($audit->isResigned()) { + continue; } + + $phids[] = $audit->getAuditorPHID(); } return $phids; diff --git a/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php b/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php index e05820c825..1165b03a3f 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php +++ b/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php @@ -49,29 +49,6 @@ final class PhabricatorRepositoryAuditRequest return $this->assertAttached($this->commit); } - public function isActiveAudit() { - switch ($this->getAuditStatus()) { - case PhabricatorAuditStatusConstants::NONE: - case PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED: - case PhabricatorAuditStatusConstants::RESIGNED: - case PhabricatorAuditStatusConstants::CLOSED: - case PhabricatorAuditStatusConstants::CC: - return false; - } - - return true; - } - - public function isInteresting() { - switch ($this->getAuditStatus()) { - case PhabricatorAuditStatusConstants::NONE: - case PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED: - return false; - } - - return true; - } - public function isResigned() { switch ($this->getAuditStatus()) { case PhabricatorAuditStatusConstants::RESIGNED: From ac2f5a10469a1607e5b349bc04c0a7ae89b921a6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 10 Mar 2021 08:54:21 -0800 Subject: [PATCH 03/13] Modernize and clean up "PhabricatorAuditStatusConstants" Summary: Ref T13631. Move "PhabricatorAuditStatusConstants" to a more modern object ("PhabricatorAuditRequestStatus"). Expose the status value via Conduit. Test Plan: - Ran `bin/audit delete`. - Viewed a commit with auditors in the web UI. - Grepped for affected symbols. - Called Conduit with the "auditors" attachment, saw auditor statuses. Maniphest Tasks: T13631 Differential Revision: https://secure.phabricator.com/D21599 --- src/__phutil_library_map__.php | 4 +- .../PhabricatorAuditRequestStatus.php | 80 +++++++++++++++++ .../PhabricatorAuditStatusConstants.php | 90 ------------------- .../audit/editor/PhabricatorAuditEditor.php | 5 -- ...abricatorAuditManagementDeleteWorkflow.php | 4 +- .../controller/DiffusionCommitController.php | 9 +- ...sionDoorkeeperCommitFeedStoryPublisher.php | 6 +- ...iffusionAuditorsSearchEngineAttachment.php | 3 + .../diffusion/herald/HeraldCommitAdapter.php | 4 +- ...fusionCommitRequiredActionResultBucket.php | 6 +- .../DiffusionCommitAcceptTransaction.php | 2 +- .../DiffusionCommitAuditTransaction.php | 16 ++-- .../DiffusionCommitAuditorsTransaction.php | 2 +- .../DiffusionCommitConcernTransaction.php | 2 +- .../DiffusionCommitResignTransaction.php | 2 +- .../PhabricatorRepositoryAuditRequest.php | 11 ++- .../storage/PhabricatorRepositoryCommit.php | 8 +- 17 files changed, 121 insertions(+), 133 deletions(-) create mode 100644 src/applications/audit/constants/PhabricatorAuditRequestStatus.php delete mode 100644 src/applications/audit/constants/PhabricatorAuditStatusConstants.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 226adfdc69..065c588ddb 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2305,7 +2305,7 @@ phutil_register_library_map(array( 'PhabricatorAuditManagementDeleteWorkflow' => 'applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php', 'PhabricatorAuditManagementWorkflow' => 'applications/audit/management/PhabricatorAuditManagementWorkflow.php', 'PhabricatorAuditReplyHandler' => 'applications/audit/mail/PhabricatorAuditReplyHandler.php', - 'PhabricatorAuditStatusConstants' => 'applications/audit/constants/PhabricatorAuditStatusConstants.php', + 'PhabricatorAuditRequestStatus' => 'applications/audit/constants/PhabricatorAuditRequestStatus.php', 'PhabricatorAuditSynchronizeManagementWorkflow' => 'applications/audit/management/PhabricatorAuditSynchronizeManagementWorkflow.php', 'PhabricatorAuditTransaction' => 'applications/audit/storage/PhabricatorAuditTransaction.php', 'PhabricatorAuditTransactionComment' => 'applications/audit/storage/PhabricatorAuditTransactionComment.php', @@ -8651,7 +8651,7 @@ phutil_register_library_map(array( 'PhabricatorAuditManagementDeleteWorkflow' => 'PhabricatorAuditManagementWorkflow', 'PhabricatorAuditManagementWorkflow' => 'PhabricatorManagementWorkflow', 'PhabricatorAuditReplyHandler' => 'PhabricatorApplicationTransactionReplyHandler', - 'PhabricatorAuditStatusConstants' => 'Phobject', + 'PhabricatorAuditRequestStatus' => 'Phobject', 'PhabricatorAuditSynchronizeManagementWorkflow' => 'PhabricatorAuditManagementWorkflow', 'PhabricatorAuditTransaction' => 'PhabricatorModularTransaction', 'PhabricatorAuditTransactionComment' => array( diff --git a/src/applications/audit/constants/PhabricatorAuditRequestStatus.php b/src/applications/audit/constants/PhabricatorAuditRequestStatus.php new file mode 100644 index 0000000000..e3fb188a14 --- /dev/null +++ b/src/applications/audit/constants/PhabricatorAuditRequestStatus.php @@ -0,0 +1,80 @@ +key = $status; + return $result; + } + + public function getIconIcon() { + return $this->getMapProperty('icon'); + } + + public function getIconColor() { + return $this->getMapProperty('icon.color'); + } + + public function getStatusName() { + $name = $this->getMapProperty('name'); + if ($name !== null) { + return $name; + } + + return pht('Unknown Audit Request Status ("%s")', $this->key); + } + + public function getStatusValue() { + return $this->key; + } + + public function isResigned() { + return ($this->key === self::RESIGNED); + } + + private function getMapProperty($key, $default = null) { + $map = self::newStatusMap(); + $spec = idx($map, $this->key, array()); + return idx($spec, $key, $default); + } + + private static function newStatusMap() { + return array( + self::AUDIT_REQUIRED => array( + 'name' => pht('Audit Required'), + 'icon' => 'fa-exclamation-circle', + 'icon.color' => 'orange', + ), + self::AUDIT_REQUESTED => array( + 'name' => pht('Audit Requested'), + 'icon' => 'fa-exclamation-circle', + 'icon.color' => 'orange', + ), + self::CONCERNED => array( + 'name' => pht('concern Raised'), + 'icon' => 'fa-times-circle', + 'icon.color' => 'red', + ), + self::ACCEPTED => array( + 'name' => pht('Accepted'), + 'icon' => 'fa-check-circle', + 'icon.color' => 'green', + ), + self::RESIGNED => array( + 'name' => pht('Resigned'), + 'icon' => 'fa-times', + 'icon.color' => 'grey', + ), + ); + } + +} diff --git a/src/applications/audit/constants/PhabricatorAuditStatusConstants.php b/src/applications/audit/constants/PhabricatorAuditStatusConstants.php deleted file mode 100644 index 54eb332124..0000000000 --- a/src/applications/audit/constants/PhabricatorAuditStatusConstants.php +++ /dev/null @@ -1,90 +0,0 @@ - pht('Audit Required'), - self::CONCERNED => pht('Concern Raised'), - self::ACCEPTED => pht('Accepted'), - self::AUDIT_REQUESTED => pht('Audit Requested'), - self::RESIGNED => pht('Resigned'), - ); - - return $map; - } - - public static function getActionRequiredStatusConstants() { - return array( - self::AUDIT_REQUIRED, - self::AUDIT_REQUESTED, - ); - } - - public static function getStatusName($code) { - return idx(self::getStatusNameMap(), $code, pht('Unknown')); - } - - public static function getStatusColor($code) { - switch ($code) { - case self::CONCERNED: - $color = 'red'; - break; - case self::AUDIT_REQUIRED: - case self::AUDIT_REQUESTED: - $color = 'orange'; - break; - case self::ACCEPTED: - $color = 'green'; - break; - case self::RESIGNED: - $color = 'grey'; - break; - default: - $color = 'bluegrey'; - break; - } - return $color; - } - - public static function getStatusIcon($code) { - switch ($code) { - case self::AUDIT_REQUIRED: - case self::AUDIT_REQUESTED: - $icon = PHUIStatusItemView::ICON_WARNING; - break; - case self::CONCERNED: - $icon = PHUIStatusItemView::ICON_REJECT; - break; - case self::ACCEPTED: - $icon = PHUIStatusItemView::ICON_ACCEPT; - break; - case self::RESIGNED: - $icon = 'fa-times'; - break; - default: - $icon = PHUIStatusItemView::ICON_QUESTION; - break; - } - return $icon; - } - - public static function getOpenStatusConstants() { - return array( - self::AUDIT_REQUIRED, - self::AUDIT_REQUESTED, - self::CONCERNED, - ); - } - - public static function isOpenStatus($status) { - return in_array($status, self::getOpenStatusConstants()); - } - -} diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index 81317fd47e..7b97ea515a 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -178,11 +178,6 @@ final class PhabricatorAuditEditor } $object->attachAudits($commit->getAudits()); - $status_concerned = PhabricatorAuditStatusConstants::CONCERNED; - $status_resigned = PhabricatorAuditStatusConstants::RESIGNED; - $status_accepted = PhabricatorAuditStatusConstants::ACCEPTED; - $status_concerned = PhabricatorAuditStatusConstants::CONCERNED; - $actor_phid = $this->getActingAsPHID(); $actor_is_author = ($object->getAuthorPHID()) && ($actor_phid == $object->getAuthorPHID()); diff --git a/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php b/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php index cd621ce821..50041330dc 100644 --- a/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php +++ b/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php @@ -153,13 +153,13 @@ final class PhabricatorAuditManagementDeleteWorkflow foreach ($commit_audits as $audit) { $audit_id = $audit->getID(); + $status = $audit->getAuditRequestStatusObject(); $description = sprintf( '%10d %-16s %-16s %s: %s', $audit_id, $handles[$audit->getAuditorPHID()]->getName(), - PhabricatorAuditStatusConstants::getStatusName( - $audit->getAuditStatus()), + $status->getStatusName(), $commit->getRepository()->formatCommitName( $commit->getCommitIdentifier()), trim($commit->getSummary())); diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index 6037781edc..7b893b1e52 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -898,12 +898,13 @@ final class DiffusionCommitController extends DiffusionController { $view = new PHUIStatusListView(); foreach ($audit_requests as $request) { - $code = $request->getAuditStatus(); + $status = $request->getAuditRequestStatusObject(); + $item = new PHUIStatusItemView(); $item->setIcon( - PhabricatorAuditStatusConstants::getStatusIcon($code), - PhabricatorAuditStatusConstants::getStatusColor($code), - PhabricatorAuditStatusConstants::getStatusName($code)); + $status->getIconIcon(), + $status->getIconColor(), + $status->getStatusName()); $auditor_phid = $request->getAuditorPHID(); $target = $viewer->renderHandle($auditor_phid); diff --git a/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php b/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php index 088e5dc71a..8f1362860d 100644 --- a/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php +++ b/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php @@ -98,9 +98,9 @@ final class DiffusionDoorkeeperCommitFeedStoryPublisher } switch ($status) { - case PhabricatorAuditStatusConstants::AUDIT_REQUIRED: - case PhabricatorAuditStatusConstants::AUDIT_REQUESTED: - case PhabricatorAuditStatusConstants::CONCERNED: + case PhabricatorAuditRequestStatus::AUDIT_REQUIRED: + case PhabricatorAuditRequestStatus::AUDIT_REQUESTED: + case PhabricatorAuditRequestStatus::CONCERNED: $active += array_fuse($request_phids); break; default: diff --git a/src/applications/diffusion/engineextension/DiffusionAuditorsSearchEngineAttachment.php b/src/applications/diffusion/engineextension/DiffusionAuditorsSearchEngineAttachment.php index a244241449..1dfcbcc05b 100644 --- a/src/applications/diffusion/engineextension/DiffusionAuditorsSearchEngineAttachment.php +++ b/src/applications/diffusion/engineextension/DiffusionAuditorsSearchEngineAttachment.php @@ -20,8 +20,11 @@ final class DiffusionAuditorsSearchEngineAttachment $list = array(); foreach ($auditors as $auditor) { + $status = $auditor->getAuditRequestStatusObject(); + $list[] = array( 'auditorPHID' => $auditor->getAuditorPHID(), + 'status' => $status->getStatusValue(), ); } diff --git a/src/applications/diffusion/herald/HeraldCommitAdapter.php b/src/applications/diffusion/herald/HeraldCommitAdapter.php index 59508d77fb..b8ebf9c022 100644 --- a/src/applications/diffusion/herald/HeraldCommitAdapter.php +++ b/src/applications/diffusion/herald/HeraldCommitAdapter.php @@ -166,8 +166,8 @@ final class HeraldCommitAdapter public function loadAuditNeededPackages() { if ($this->auditNeededPackages === null) { $status_arr = array( - PhabricatorAuditStatusConstants::AUDIT_REQUIRED, - PhabricatorAuditStatusConstants::CONCERNED, + PhabricatorAuditRequestStatus::AUDIT_REQUIRED, + PhabricatorAuditRequestStatus::CONCERNED, ); $requests = id(new PhabricatorRepositoryAuditRequest()) ->loadAllWhere( diff --git a/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php b/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php index 25984c93e1..0c5bdbced7 100644 --- a/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php +++ b/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php @@ -90,7 +90,7 @@ final class DiffusionCommitRequiredActionResultBucket $objects = $this->objects; $has_concern = array( - PhabricatorAuditStatusConstants::CONCERNED, + PhabricatorAuditRequestStatus::CONCERNED, ); $has_concern = array_fuse($has_concern); @@ -119,8 +119,8 @@ final class DiffusionCommitRequiredActionResultBucket $objects = $this->objects; $should_audit = array( - PhabricatorAuditStatusConstants::AUDIT_REQUIRED, - PhabricatorAuditStatusConstants::AUDIT_REQUESTED, + PhabricatorAuditRequestStatus::AUDIT_REQUIRED, + PhabricatorAuditRequestStatus::AUDIT_REQUESTED, ); $should_audit = array_fuse($should_audit); diff --git a/src/applications/diffusion/xaction/DiffusionCommitAcceptTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitAcceptTransaction.php index 5ade7f3513..9cd42eab36 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitAcceptTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitAcceptTransaction.php @@ -31,7 +31,7 @@ final class DiffusionCommitAcceptTransaction } public function applyExternalEffects($object, $value) { - $status = PhabricatorAuditStatusConstants::ACCEPTED; + $status = PhabricatorAuditRequestStatus::ACCEPTED; $actor = $this->getActor(); $this->applyAuditorEffect($object, $actor, $value, $status); } diff --git a/src/applications/diffusion/xaction/DiffusionCommitAuditTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitAuditTransaction.php index c80301e6ee..b6657b73f2 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitAuditTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitAuditTransaction.php @@ -21,12 +21,12 @@ abstract class DiffusionCommitAuditTransaction PhabricatorRepositoryCommit $commit, PhabricatorUser $viewer) { - // This omits various inactive states like "Resigned" and "Not Required". + // This omits inactive states; currently just "Resigned". $active = array( - PhabricatorAuditStatusConstants::AUDIT_REQUIRED, - PhabricatorAuditStatusConstants::CONCERNED, - PhabricatorAuditStatusConstants::ACCEPTED, - PhabricatorAuditStatusConstants::AUDIT_REQUESTED, + PhabricatorAuditRequestStatus::AUDIT_REQUIRED, + PhabricatorAuditRequestStatus::CONCERNED, + PhabricatorAuditRequestStatus::ACCEPTED, + PhabricatorAuditRequestStatus::AUDIT_REQUESTED, ); $active = array_fuse($active); @@ -42,7 +42,7 @@ abstract class DiffusionCommitAuditTransaction $commit, $viewer, array( - PhabricatorAuditStatusConstants::ACCEPTED, + PhabricatorAuditRequestStatus::ACCEPTED, )); } @@ -53,7 +53,7 @@ abstract class DiffusionCommitAuditTransaction $commit, $viewer, array( - PhabricatorAuditStatusConstants::CONCERNED, + PhabricatorAuditRequestStatus::CONCERNED, )); } @@ -117,7 +117,7 @@ abstract class DiffusionCommitAuditTransaction $map = array(); - $with_authority = ($status != PhabricatorAuditStatusConstants::RESIGNED); + $with_authority = ($status != PhabricatorAuditRequestStatus::RESIGNED); if ($with_authority) { foreach ($audits as $audit) { if ($commit->hasAuditAuthority($actor, $audit, $acting_phid)) { diff --git a/src/applications/diffusion/xaction/DiffusionCommitAuditorsTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitAuditorsTransaction.php index 9ef51b9e90..15b44543e9 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitAuditorsTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitAuditorsTransaction.php @@ -16,7 +16,7 @@ final class DiffusionCommitAuditorsTransaction $auditors = $this->generateOldValue($object); $old_auditors = $auditors; - $request_status = PhabricatorAuditStatusConstants::AUDIT_REQUESTED; + $request_status = PhabricatorAuditRequestStatus::AUDIT_REQUESTED; $rem = idx($value, '-', array()); foreach ($rem as $phid) { diff --git a/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php index ffd084412e..8f2744f21d 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php @@ -37,7 +37,7 @@ final class DiffusionCommitConcernTransaction } public function applyExternalEffects($object, $value) { - $status = PhabricatorAuditStatusConstants::CONCERNED; + $status = PhabricatorAuditRequestStatus::CONCERNED; $actor = $this->getActor(); $this->applyAuditorEffect($object, $actor, $value, $status); } diff --git a/src/applications/diffusion/xaction/DiffusionCommitResignTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitResignTransaction.php index 8adc8346dc..1f5a4b94b0 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitResignTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitResignTransaction.php @@ -36,7 +36,7 @@ final class DiffusionCommitResignTransaction } public function applyExternalEffects($object, $value) { - $status = PhabricatorAuditStatusConstants::RESIGNED; + $status = PhabricatorAuditRequestStatus::RESIGNED; $actor = $this->getActor(); $this->applyAuditorEffect($object, $actor, $value, $status); } diff --git a/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php b/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php index 1165b03a3f..606a0656e0 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php +++ b/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php @@ -50,14 +50,13 @@ final class PhabricatorRepositoryAuditRequest } public function isResigned() { - switch ($this->getAuditStatus()) { - case PhabricatorAuditStatusConstants::RESIGNED: - return true; - } - - return false; + return $this->getAuditRequestStatusObject()->isResigned(); } + public function getAuditRequestStatusObject() { + $status = $this->getAuditStatus(); + return PhabricatorAuditRequestStatus::newForStatus($status); + } /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index f0c7d6abf8..31413ea0c2 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -312,14 +312,14 @@ final class PhabricatorRepositoryCommit foreach ($requests as $request) { switch ($request->getAuditStatus()) { - case PhabricatorAuditStatusConstants::AUDIT_REQUIRED: - case PhabricatorAuditStatusConstants::AUDIT_REQUESTED: + case PhabricatorAuditRequestStatus::AUDIT_REQUIRED: + case PhabricatorAuditRequestStatus::AUDIT_REQUESTED: $any_need = true; break; - case PhabricatorAuditStatusConstants::ACCEPTED: + case PhabricatorAuditRequestStatus::ACCEPTED: $any_accept = true; break; - case PhabricatorAuditStatusConstants::CONCERNED: + case PhabricatorAuditRequestStatus::CONCERNED: $any_concern = true; break; } From 404b55ce573a1548af73c23f6de96537cf22e244 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 10 Mar 2021 09:33:49 -0800 Subject: [PATCH 04/13] Give audit statuses API constants that match their UI strings Summary: Ref T13631. These strings were a little inconsistent; make them more consistent. Test Plan: Called `diffusion.commit.search` with the appropriate attachment, saw slightly more consistent statuses. Maniphest Tasks: T13631 Differential Revision: https://secure.phabricator.com/D21600 --- .../audit/constants/PhabricatorAuditRequestStatus.php | 11 ++++++++++- .../DiffusionAuditorsSearchEngineAttachment.php | 2 +- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/applications/audit/constants/PhabricatorAuditRequestStatus.php b/src/applications/audit/constants/PhabricatorAuditRequestStatus.php index e3fb188a14..ea0c496df0 100644 --- a/src/applications/audit/constants/PhabricatorAuditRequestStatus.php +++ b/src/applications/audit/constants/PhabricatorAuditRequestStatus.php @@ -37,6 +37,10 @@ final class PhabricatorAuditRequestStatus extends Phobject { return $this->key; } + public function getStatusValueForConduit() { + return $this->getMapProperty('value.conduit'); + } + public function isResigned() { return ($this->key === self::RESIGNED); } @@ -53,26 +57,31 @@ final class PhabricatorAuditRequestStatus extends Phobject { 'name' => pht('Audit Required'), 'icon' => 'fa-exclamation-circle', 'icon.color' => 'orange', + 'value.conduit' => 'audit-required', ), self::AUDIT_REQUESTED => array( 'name' => pht('Audit Requested'), 'icon' => 'fa-exclamation-circle', 'icon.color' => 'orange', + 'value.conduit' => 'audit-requested', ), self::CONCERNED => array( - 'name' => pht('concern Raised'), + 'name' => pht('Concern Raised'), 'icon' => 'fa-times-circle', 'icon.color' => 'red', + 'value.conduit' => 'concern-raised', ), self::ACCEPTED => array( 'name' => pht('Accepted'), 'icon' => 'fa-check-circle', 'icon.color' => 'green', + 'value.conduit' => 'accepted', ), self::RESIGNED => array( 'name' => pht('Resigned'), 'icon' => 'fa-times', 'icon.color' => 'grey', + 'value.conduit' => 'resigned', ), ); } diff --git a/src/applications/diffusion/engineextension/DiffusionAuditorsSearchEngineAttachment.php b/src/applications/diffusion/engineextension/DiffusionAuditorsSearchEngineAttachment.php index 1dfcbcc05b..6023e8b678 100644 --- a/src/applications/diffusion/engineextension/DiffusionAuditorsSearchEngineAttachment.php +++ b/src/applications/diffusion/engineextension/DiffusionAuditorsSearchEngineAttachment.php @@ -24,7 +24,7 @@ final class DiffusionAuditorsSearchEngineAttachment $list[] = array( 'auditorPHID' => $auditor->getAuditorPHID(), - 'status' => $status->getStatusValue(), + 'status' => $status->getStatusValueForConduit(), ); } From afdef332fb20836385fda6fa060e3be6bb9986ff Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 10 Mar 2021 10:05:57 -0800 Subject: [PATCH 05/13] Allow "transaction.search" to be called on an object type Summary: Ref T13631. This supports a more robust version of "poll for updates by using dateModified window queries" that uses transactions as a logical clock. This is particularly relevant for commits, since they don't have a "dateModified" at time of writing. Test Plan: - Queried for transactions by type and object. - Issued various invalid transaction queries, got appropriate errors. Maniphest Tasks: T13631 Differential Revision: https://secure.phabricator.com/D21601 --- .../TransactionSearchConduitAPIMethod.php | 124 +++++++++++++----- 1 file changed, 92 insertions(+), 32 deletions(-) diff --git a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php index bc0311eae4..aaeee3aea7 100644 --- a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php +++ b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php @@ -8,7 +8,9 @@ final class TransactionSearchConduitAPIMethod } public function getMethodDescription() { - return pht('Read transactions and comments for an object.'); + return pht( + 'Read transactions and comments for a particular object '. + 'or an entire object type.'); } public function getMethodDocumentation() { @@ -23,6 +25,26 @@ One common reason to call this method is that you're implmenting a webhook and just received a notification that an object has changed. See the Webhooks documentation for more detailed discussion of this use case. +One Object Type at a Time +========================= + +This API method can query transactions for any type of object which supports +transactions, but only one type of object can be queried per call. For example: +you can retrieve transactions affecting Tasks, or you can retrieve transactions +affecting Revisions, but a single call can not retrieve both. + +This is a technical limitation arising because (among other reasons) there is +no global ordering on transactions. + +To find transactions for a specific object (like a particular task), pass the +object PHID or an appropriate object identifier (like `T123`) as an +`objectIdentifier`. + +To find all transactions for an object type, pass the object type constant as +an `objectType`. For example, the correct identifier for tasks is `TASK`. (You +can quickly find an unknown type constant by looking at the PHID of an object +of that type.) + Constraints =========== @@ -64,8 +86,9 @@ EOREMARKUP protected function defineParamTypes() { return array( - 'objectIdentifier' => 'phid|string', - 'constraints' => 'map', + 'objectIdentifier' => 'optional phid|string', + 'objectType' => 'optional string', + 'constraints' => 'optional map', ) + $this->getPagerParamTypes(); } @@ -81,43 +104,19 @@ EOREMARKUP $viewer = $request->getUser(); $pager = $this->newPager($request); - $object_name = $request->getValue('objectIdentifier', null); - if (!strlen($object_name)) { - throw new Exception( - pht( - 'When calling "transaction.search", you must provide an object to '. - 'retrieve transactions for.')); - } - - $object = id(new PhabricatorObjectQuery()) - ->setViewer($viewer) - ->withNames(array($object_name)) - ->executeOne(); - if (!$object) { - throw new Exception( - pht( - 'No object "%s" exists.', - $object_name)); - } - - if (!($object instanceof PhabricatorApplicationTransactionInterface)) { - throw new Exception( - pht( - 'Object "%s" (of type "%s") does not implement "%s", so '. - 'transactions can not be loaded for it.', - $object_name, - get_class($object), - 'PhabricatorApplicationTransactionInterface')); - } + $object = $this->loadTemplateObject($request); $xaction_query = PhabricatorApplicationTransactionQuery::newQueryForObject( $object); $xaction_query ->needHandles(false) - ->withObjectPHIDs(array($object->getPHID())) ->setViewer($viewer); + if ($object->getPHID()) { + $xaction_query->withObjectPHIDs(array($object->getPHID())); + } + $constraints = $request->getValue('constraints', array()); $xaction_query = $this->applyConstraints($constraints, $xaction_query); @@ -355,4 +354,65 @@ EOREMARKUP ); } + private function loadTemplateObject(ConduitAPIRequest $request) { + $viewer = $request->getUser(); + + $object_identifier = $request->getValue('objectIdentifier'); + $object_type = $request->getValue('objectType'); + + $has_identifier = ($object_identifier !== null); + $has_type = ($object_type !== null); + + if (!$has_type && !$has_identifier) { + throw new Exception( + pht( + 'Calls to "transaction.search" must specify either an "objectType" '. + 'or an "objectIdentifier"')); + } else if ($has_type && $has_identifier) { + throw new Exception( + pht( + 'Calls to "transaction.search" must not specify both an '. + '"objectType" and an "objectIdentifier".')); + } + + if ($has_type) { + $all_types = PhabricatorPHIDType::getAllTypes(); + + if (!isset($all_types[$object_type])) { + ksort($all_types); + throw new Exception( + pht( + 'In call to "transaction.search", specified "objectType" ("%s") '. + 'is unknown. Valid object types are: %s.', + $object_type, + implode(', ', array_keys($all_types)))); + } + + $object = $all_types[$object_type]->newObject(); + } else { + $object = id(new PhabricatorObjectQuery()) + ->setViewer($viewer) + ->withNames(array($object_identifier)) + ->executeOne(); + if (!$object) { + throw new Exception( + pht( + 'In call to "transaction.search", specified "objectIdentifier" '. + '("%s") does not exist.', + $object_identifier)); + } + } + + if (!($object instanceof PhabricatorApplicationTransactionInterface)) { + throw new Exception( + pht( + 'In call to "transaction.search", selected object (of type "%s") '. + 'does not implement "%s", so transactions can not be loaded for it.', + get_class($object), + 'PhabricatorApplicationTransactionInterface')); + } + + return $object; + } + } From 4cff4dc68b6e23b2557a994b71325f508e60af6d Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 10 Mar 2021 11:43:15 -0800 Subject: [PATCH 06/13] Interpret search tokens in the for "_..." as substring search Summary: Ref T13632. Users searching for `__FILE__`, etc., almost certainly mean to perform a substring search. Test Plan: Added tests and made them pass. Searched for various tokens, saw compiler interpretation in UI. Maniphest Tasks: T13632 Differential Revision: https://secure.phabricator.com/D21602 --- .../compiler/PhutilSearchQueryCompiler.php | 23 +++++++++++++++---- .../PhutilSearchQueryCompilerTestCase.php | 14 +++++++++++ 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/src/applications/search/compiler/PhutilSearchQueryCompiler.php b/src/applications/search/compiler/PhutilSearchQueryCompiler.php index 8f99682f37..c3f93e16c9 100644 --- a/src/applications/search/compiler/PhutilSearchQueryCompiler.php +++ b/src/applications/search/compiler/PhutilSearchQueryCompiler.php @@ -284,11 +284,24 @@ final class PhutilSearchQueryCompiler $operator = self::OPERATOR_AND; break; case '': - // See T12995. If this query term contains Chinese, Japanese or - // Korean characters, treat the term as a substring term by default. - // These languages do not separate words with spaces, so the term - // search mode is normally useless. - if ($enable_functions && !$is_quoted && phutil_utf8_is_cjk($value)) { + $use_substring = false; + + if ($enable_functions && !$is_quoted) { + // See T12995. If this query term contains Chinese, Japanese or + // Korean characters, treat the term as a substring term by default. + // These languages do not separate words with spaces, so the term + // search mode is normally useless. + if (phutil_utf8_is_cjk($value)) { + $use_substring = true; + } else if (phutil_preg_match('/^_/', $value)) { + // See T13632. Assume users searching for any term that begins + // with an undescore intend to perform substring search if they + // don't provide an explicit search function. + $use_substring = true; + } + } + + if ($use_substring) { $operator = self::OPERATOR_SUBSTRING; } else { $operator = self::OPERATOR_AND; diff --git a/src/applications/search/compiler/__tests__/PhutilSearchQueryCompilerTestCase.php b/src/applications/search/compiler/__tests__/PhutilSearchQueryCompilerTestCase.php index 8576e800c7..4dc41d9734 100644 --- a/src/applications/search/compiler/__tests__/PhutilSearchQueryCompilerTestCase.php +++ b/src/applications/search/compiler/__tests__/PhutilSearchQueryCompilerTestCase.php @@ -205,6 +205,20 @@ final class PhutilSearchQueryCompilerTestCase 'xyz', ), ), + + // See T12995. Interpret CJK tokens as substring queries since these + // languages do not use spaces as word separators. + "\xE7\x8C\xAB" => array( + array(null, $op_sub, "\xE7\x8C\xAB"), + ), + + // See T13632. Interpret tokens that begin with "_" as substring tokens + // if no function is specified. + '_x _y_ "_z_"' => array( + array(null, $op_sub, '_x'), + array(null, $op_sub, '_y_'), + array(null, $op_and, '_z_'), + ), ); $this->assertCompileFunctionQueries($function_tests); From 0815891e4263e726c9701610c2eb9bf4db92d4a2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 11 Mar 2021 10:31:25 -0800 Subject: [PATCH 07/13] Fix an error when users receive notifications about objects they can no longer see Summary: Ref T13623. The change in D21577 could lead to a case where we try to access stories the user can't see. Move the story-loading piece to "willFilterPage()" to make our way thorugh this. Test Plan: - Made FeedStory return nothing to simulate invisible notifications, loaded page. - Before: index access fatal. - After: clean "no notifications". - Loaded notifications normally, saw normal notifications. Maniphest Tasks: T13623 Differential Revision: https://secure.phabricator.com/D21603 --- .../query/PhabricatorNotificationQuery.php | 63 ++++++++++++------- 1 file changed, 39 insertions(+), 24 deletions(-) diff --git a/src/applications/notification/query/PhabricatorNotificationQuery.php b/src/applications/notification/query/PhabricatorNotificationQuery.php index 4ceca2e1d4..69eb41695b 100644 --- a/src/applications/notification/query/PhabricatorNotificationQuery.php +++ b/src/applications/notification/query/PhabricatorNotificationQuery.php @@ -63,25 +63,7 @@ final class PhabricatorNotificationQuery $this->buildWhereClause($conn), $this->buildLimitClause($conn)); - // See T13623. Although most queries for notifications return unique - // stories, this isn't a guarantee. - $story_map = ipull($data, null, 'chronologicalKey'); - - $stories = PhabricatorFeedStory::loadAllFromRows( - $story_map, - $this->getViewer()); - $stories = mpull($stories, null, 'getChronologicalKey'); - - $results = array(); - foreach ($data as $row) { - $story_key = $row['chronologicalKey']; - $has_viewed = $row['hasViewed']; - - $results[] = id(clone $stories[$story_key]) - ->setHasViewed($has_viewed); - } - - return $results; + return $data; } protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { @@ -111,14 +93,47 @@ final class PhabricatorNotificationQuery return $where; } - protected function willFilterPage(array $stories) { - foreach ($stories as $key => $story) { - if (!$story->isVisibleInNotifications()) { - unset($stories[$key]); + protected function willFilterPage(array $rows) { + // See T13623. The policy model here is outdated and awkward. + + // Users may have notifications about objects they can no longer see. + // Two ways this can arise: destroy an object; or change an object's + // view policy to exclude a user. + + // "PhabricatorFeedStory::loadAllFromRows()" does its own policy filtering. + // This doesn't align well with modern query sequencing, but we should be + // able to get away with it by loading here. + + // See T13623. Although most queries for notifications return unique + // stories, this isn't a guarantee. + $story_map = ipull($rows, null, 'chronologicalKey'); + + $viewer = $this->getViewer(); + $stories = PhabricatorFeedStory::loadAllFromRows($story_map, $viewer); + $stories = mpull($stories, null, 'getChronologicalKey'); + + $results = array(); + foreach ($rows as $row) { + $story_key = $row['chronologicalKey']; + $has_viewed = $row['hasViewed']; + + if (!isset($stories[$story_key])) { + // NOTE: We can't call "didRejectResult()" here because we don't have + // a policy object to pass. + continue; } + + $story = id(clone $stories[$story_key]) + ->setHasViewed($has_viewed); + + if (!$story->isVisibleInNotifications()) { + continue; + } + + $results[] = $story; } - return $stories; + return $results; } protected function getDefaultOrderVector() { From 4484946cfd29d1d3f61c03825d812601038fa75c Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 11 Mar 2021 11:48:55 -0800 Subject: [PATCH 08/13] In JSON DocumentEngine, preserve the distinction between "{}" and "[]" Summary: Ref T13635. Currently, the JSON DocumentEngine uses "phutil_json_decode()", but this can confuse "{}" and "[]". Be more careful about how the JSON value is decoded, to preserve the distinction. Test Plan: {F8520479} Maniphest Tasks: T13635 Differential Revision: https://secure.phabricator.com/D21605 --- src/__phutil_library_map__.php | 2 ++ .../document/PhabricatorJSONDocumentEngine.php | 18 ++++++++++++++++++ ...habricatorDocumentEngineParserException.php | 4 ++++ 3 files changed, 24 insertions(+) create mode 100644 src/applications/files/document/exception/PhabricatorDocumentEngineParserException.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 065c588ddb..52e5f46bbf 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3209,6 +3209,7 @@ phutil_register_library_map(array( 'PhabricatorDocumentEngineBlock' => 'applications/files/diff/PhabricatorDocumentEngineBlock.php', 'PhabricatorDocumentEngineBlockDiff' => 'applications/files/diff/PhabricatorDocumentEngineBlockDiff.php', 'PhabricatorDocumentEngineBlocks' => 'applications/files/diff/PhabricatorDocumentEngineBlocks.php', + 'PhabricatorDocumentEngineParserException' => 'applications/files/document/exception/PhabricatorDocumentEngineParserException.php', 'PhabricatorDocumentRef' => 'applications/files/document/PhabricatorDocumentRef.php', 'PhabricatorDocumentRenderingEngine' => 'applications/files/document/render/PhabricatorDocumentRenderingEngine.php', 'PhabricatorDoorkeeperApplication' => 'applications/doorkeeper/application/PhabricatorDoorkeeperApplication.php', @@ -9707,6 +9708,7 @@ phutil_register_library_map(array( 'PhabricatorDocumentEngineBlock' => 'Phobject', 'PhabricatorDocumentEngineBlockDiff' => 'Phobject', 'PhabricatorDocumentEngineBlocks' => 'Phobject', + 'PhabricatorDocumentEngineParserException' => 'Exception', 'PhabricatorDocumentRef' => 'Phobject', 'PhabricatorDocumentRenderingEngine' => 'Phobject', 'PhabricatorDoorkeeperApplication' => 'PhabricatorApplication', diff --git a/src/applications/files/document/PhabricatorJSONDocumentEngine.php b/src/applications/files/document/PhabricatorJSONDocumentEngine.php index 683f1746e6..42f4469ee6 100644 --- a/src/applications/files/document/PhabricatorJSONDocumentEngine.php +++ b/src/applications/files/document/PhabricatorJSONDocumentEngine.php @@ -31,6 +31,17 @@ final class PhabricatorJSONDocumentEngine try { $data = phutil_json_decode($raw_data); + // See T13635. "phutil_json_decode()" always turns JSON into a PHP array, + // and we lose the distinction between "{}" and "[]". This distinction is + // important when rendering a document. + $data = json_decode($raw_data, false); + if (!$data) { + throw new PhabricatorDocumentEngineParserException( + pht( + 'Failed to "json_decode(...)" JSON document after successfully '. + 'decoding it with "phutil_json_decode(...).')); + } + if (preg_match('/^\s*\[/', $raw_data)) { $content = id(new PhutilJSON())->encodeAsList($data); } else { @@ -47,6 +58,13 @@ final class PhabricatorJSONDocumentEngine 'This document is not valid JSON: %s', $ex->getMessage())); + $content = $raw_data; + } catch (PhabricatorDocumentEngineParserException $ex) { + $message = $this->newMessage( + pht( + 'Unable to parse this document as JSON: %s', + $ex->getMessage())); + $content = $raw_data; } diff --git a/src/applications/files/document/exception/PhabricatorDocumentEngineParserException.php b/src/applications/files/document/exception/PhabricatorDocumentEngineParserException.php new file mode 100644 index 0000000000..b664d942ad --- /dev/null +++ b/src/applications/files/document/exception/PhabricatorDocumentEngineParserException.php @@ -0,0 +1,4 @@ + Date: Thu, 11 Mar 2021 13:07:36 -0800 Subject: [PATCH 09/13] Improve routing of "/robots.txt", "/favicon.ico", "/status/", and 404 on custom Sites Summary: Fixes T12919. Fixes T13636. Prior to this change, some well-known resource paths don't route on sites like ResourceSite. - `/robots.txt`: Make it route on ResourceSite and just deny the whole site. - `/favicon.ico`: Make it route on ResourceSite. - `/status/`: Make it route on ResourceSite. - 404: Make it render a 404 on ResourceSite. Test Plan: - Visited all URIs on ResourceSite, got sensible responses. - Visited all URIs on main site. - Visited 404 while logged out, got login page. Maniphest Tasks: T13636, T12919 Differential Revision: https://secure.phabricator.com/D21606 --- src/__phutil_library_map__.php | 6 +++++ .../AphrontApplicationConfiguration.php | 9 +++---- src/aphront/response/Aphront404Response.php | 9 ++++++- src/aphront/site/AphrontSite.php | 2 +- src/aphront/site/PhabricatorPlatformSite.php | 4 +++ .../PhabricatorAuthMainMenuBarExtension.php | 8 +++++- .../controller/Phabricator404Controller.php | 7 +++++- .../PhabricatorPlatform404Controller.php | 10 ++++++++ .../PhabricatorSystemApplication.php | 10 +++++++- .../PhabricatorRobotsController.php | 22 +++++----------- .../PhabricatorRobotsPlatformController.php | 25 +++++++++++++++++++ .../PhabricatorRobotsResourceController.php | 17 +++++++++++++ 12 files changed, 103 insertions(+), 26 deletions(-) create mode 100644 src/applications/base/controller/PhabricatorPlatform404Controller.php create mode 100644 src/applications/system/controller/PhabricatorRobotsPlatformController.php create mode 100644 src/applications/system/controller/PhabricatorRobotsResourceController.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 52e5f46bbf..9f02ad2c41 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4253,6 +4253,7 @@ phutil_register_library_map(array( 'PhabricatorPhurlURLViewController' => 'applications/phurl/controller/PhabricatorPhurlURLViewController.php', 'PhabricatorPinnedApplicationsSetting' => 'applications/settings/setting/PhabricatorPinnedApplicationsSetting.php', 'PhabricatorPirateEnglishTranslation' => 'infrastructure/internationalization/translation/PhabricatorPirateEnglishTranslation.php', + 'PhabricatorPlatform404Controller' => 'applications/base/controller/PhabricatorPlatform404Controller.php', 'PhabricatorPlatformSite' => 'aphront/site/PhabricatorPlatformSite.php', 'PhabricatorPointsEditField' => 'applications/transactions/editfield/PhabricatorPointsEditField.php', 'PhabricatorPointsFact' => 'applications/fact/fact/PhabricatorPointsFact.php', @@ -4689,6 +4690,8 @@ phutil_register_library_map(array( 'PhabricatorResetPasswordUserLogType' => 'applications/people/userlog/PhabricatorResetPasswordUserLogType.php', 'PhabricatorResourceSite' => 'aphront/site/PhabricatorResourceSite.php', 'PhabricatorRobotsController' => 'applications/system/controller/PhabricatorRobotsController.php', + 'PhabricatorRobotsPlatformController' => 'applications/system/controller/PhabricatorRobotsPlatformController.php', + 'PhabricatorRobotsResourceController' => 'applications/system/controller/PhabricatorRobotsResourceController.php', 'PhabricatorS3FileStorageEngine' => 'applications/files/engine/PhabricatorS3FileStorageEngine.php', 'PhabricatorSMSAuthFactor' => 'applications/auth/factor/PhabricatorSMSAuthFactor.php', 'PhabricatorSQLPatchList' => 'infrastructure/storage/patch/PhabricatorSQLPatchList.php', @@ -10914,6 +10917,7 @@ phutil_register_library_map(array( 'PhabricatorPhurlURLViewController' => 'PhabricatorPhurlController', 'PhabricatorPinnedApplicationsSetting' => 'PhabricatorInternalSetting', 'PhabricatorPirateEnglishTranslation' => 'PhutilTranslation', + 'PhabricatorPlatform404Controller' => 'PhabricatorController', 'PhabricatorPlatformSite' => 'PhabricatorSite', 'PhabricatorPointsEditField' => 'PhabricatorEditField', 'PhabricatorPointsFact' => 'PhabricatorFact', @@ -11471,6 +11475,8 @@ phutil_register_library_map(array( 'PhabricatorResetPasswordUserLogType' => 'PhabricatorUserLogType', 'PhabricatorResourceSite' => 'PhabricatorSite', 'PhabricatorRobotsController' => 'PhabricatorController', + 'PhabricatorRobotsPlatformController' => 'PhabricatorRobotsController', + 'PhabricatorRobotsResourceController' => 'PhabricatorRobotsController', 'PhabricatorS3FileStorageEngine' => 'PhabricatorFileStorageEngine', 'PhabricatorSMSAuthFactor' => 'PhabricatorAuthFactor', 'PhabricatorSQLPatchList' => 'Phobject', diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php index 31b3db9a7f..9e22ca7f47 100644 --- a/src/aphront/configuration/AphrontApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontApplicationConfiguration.php @@ -32,10 +32,6 @@ final class AphrontApplicationConfiguration return $request; } - public function build404Controller() { - return array(new Phabricator404Controller(), array()); - } - public function buildRedirectController($uri, $external) { return array( new PhabricatorRedirectController(), @@ -504,7 +500,10 @@ final class AphrontApplicationConfiguration return array($result, array()); } - return $this->build404Controller(); + throw new Exception( + pht( + 'Aphront site ("%s") failed to build a 404 controller.', + get_class($site))); } /** diff --git a/src/aphront/response/Aphront404Response.php b/src/aphront/response/Aphront404Response.php index ea98e9102b..5f2e9f1115 100644 --- a/src/aphront/response/Aphront404Response.php +++ b/src/aphront/response/Aphront404Response.php @@ -10,10 +10,17 @@ final class Aphront404Response extends AphrontHTMLResponse { $request = $this->getRequest(); $viewer = $request->getViewer(); + // See T13636. Note that this response may be served from a Site other than + // the primary PlatformSite. For now, always link to the PlatformSite. + + // (This may not be the best possible place to send users who are currently + // on "real" sites, like the BlogSite.) + $return_uri = PhabricatorEnv::getURI('/'); + $dialog = id(new AphrontDialogView()) ->setViewer($viewer) ->setTitle(pht('404 Not Found')) - ->addCancelButton('/', pht('Return to Charted Waters')) + ->addCancelButton($return_uri, pht('Return to Charted Waters')) ->appendParagraph( pht( 'You arrive at your destination, but there is nothing here.')) diff --git a/src/aphront/site/AphrontSite.php b/src/aphront/site/AphrontSite.php index 9eb053b03b..85bf42fe6c 100644 --- a/src/aphront/site/AphrontSite.php +++ b/src/aphront/site/AphrontSite.php @@ -10,7 +10,7 @@ abstract class AphrontSite extends Phobject { abstract public function getRoutingMaps(); public function new404Controller(AphrontRequest $request) { - return null; + return new Phabricator404Controller(); } protected function isHostMatch($host, array $uris) { diff --git a/src/aphront/site/PhabricatorPlatformSite.php b/src/aphront/site/PhabricatorPlatformSite.php index 0973758bcd..a3193e65d1 100644 --- a/src/aphront/site/PhabricatorPlatformSite.php +++ b/src/aphront/site/PhabricatorPlatformSite.php @@ -50,4 +50,8 @@ final class PhabricatorPlatformSite extends PhabricatorSite { return $maps; } + public function new404Controller(AphrontRequest $request) { + return new PhabricatorPlatform404Controller(); + } + } diff --git a/src/applications/auth/extension/PhabricatorAuthMainMenuBarExtension.php b/src/applications/auth/extension/PhabricatorAuthMainMenuBarExtension.php index d49a447df6..042a6816dd 100644 --- a/src/applications/auth/extension/PhabricatorAuthMainMenuBarExtension.php +++ b/src/applications/auth/extension/PhabricatorAuthMainMenuBarExtension.php @@ -39,7 +39,13 @@ final class PhabricatorAuthMainMenuBarExtension private function buildLoginMenu() { $controller = $this->getController(); - $uri = new PhutilURI('/auth/start/'); + // See T13636. This button may be rendered by the 404 controller on sites + // other than the primary PlatformSite. Link the button to the primary + // site. + + $uri = '/auth/start/'; + $uri = PhabricatorEnv::getURI($uri); + $uri = new PhutilURI($uri); if ($controller) { $path = $controller->getRequest()->getPath(); $uri->replaceQueryParam('next', $path); diff --git a/src/applications/base/controller/Phabricator404Controller.php b/src/applications/base/controller/Phabricator404Controller.php index 1aa785a040..f181f5badd 100644 --- a/src/applications/base/controller/Phabricator404Controller.php +++ b/src/applications/base/controller/Phabricator404Controller.php @@ -1,6 +1,11 @@ 'PhabricatorStatusController', '/debug/' => 'PhabricatorDebugController', '/favicon.ico' => 'PhabricatorFaviconController', - '/robots.txt' => 'PhabricatorRobotsController', + '/robots.txt' => 'PhabricatorRobotsPlatformController', '/services/' => array( 'encoding/' => 'PhabricatorSystemSelectEncodingController', 'highlight/' => 'PhabricatorSystemSelectHighlightController', @@ -38,4 +38,12 @@ final class PhabricatorSystemApplication extends PhabricatorApplication { ); } + public function getResourceRoutes() { + return array( + '/status/' => 'PhabricatorStatusController', + '/favicon.ico' => 'PhabricatorFaviconController', + '/robots.txt' => 'PhabricatorRobotsResourceController', + ); + } + } diff --git a/src/applications/system/controller/PhabricatorRobotsController.php b/src/applications/system/controller/PhabricatorRobotsController.php index 18be8862a8..c8b73fd947 100644 --- a/src/applications/system/controller/PhabricatorRobotsController.php +++ b/src/applications/system/controller/PhabricatorRobotsController.php @@ -1,26 +1,13 @@ newRobotsRules(); // Add a small crawl delay (number of seconds between requests) for spiders // which respect it. The intent here is to prevent spiders from affecting @@ -36,4 +23,7 @@ final class PhabricatorRobotsController extends PhabricatorController { ->setCacheDurationInSeconds(phutil_units('2 hours in seconds')) ->setCanCDN(true); } + + abstract protected function newRobotsRules(); + } diff --git a/src/applications/system/controller/PhabricatorRobotsPlatformController.php b/src/applications/system/controller/PhabricatorRobotsPlatformController.php new file mode 100644 index 0000000000..5132e94e87 --- /dev/null +++ b/src/applications/system/controller/PhabricatorRobotsPlatformController.php @@ -0,0 +1,25 @@ + Date: Thu, 11 Mar 2021 13:47:23 -0800 Subject: [PATCH 10/13] Improve routing of "/robots.txt", "/favicon.ico", and "/status/" on Short and Blog sites Summary: Ref T13636. Add routing for "/robots.txt", "favicon.ico", and "/status/" on the ShortSite and BlogSite. Test Plan: Visted all resources (and 404 pages) on Short and Blog sites, and Platform site. Maniphest Tasks: T13636 Differential Revision: https://secure.phabricator.com/D21607 --- src/__phutil_library_map__.php | 10 +++++++--- .../PhabricatorPhameApplication.php | 6 +++++- .../PhabricatorPhurlApplication.php | 4 ++++ .../robots/PhabricatorRobotsBlogController.php | 17 +++++++++++++++++ .../PhabricatorRobotsController.php | 7 ------- .../PhabricatorRobotsPlatformController.php | 7 +++++++ .../PhabricatorRobotsResourceController.php | 1 + .../PhabricatorRobotsShortController.php | 18 ++++++++++++++++++ 8 files changed, 59 insertions(+), 11 deletions(-) create mode 100644 src/applications/system/controller/robots/PhabricatorRobotsBlogController.php rename src/applications/system/controller/{ => robots}/PhabricatorRobotsController.php (55%) rename src/applications/system/controller/{ => robots}/PhabricatorRobotsPlatformController.php (66%) rename src/applications/system/controller/{ => robots}/PhabricatorRobotsResourceController.php (90%) create mode 100644 src/applications/system/controller/robots/PhabricatorRobotsShortController.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 9f02ad2c41..3cae40d75e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4689,9 +4689,11 @@ phutil_register_library_map(array( 'PhabricatorRequestExceptionHandler' => 'aphront/handler/PhabricatorRequestExceptionHandler.php', 'PhabricatorResetPasswordUserLogType' => 'applications/people/userlog/PhabricatorResetPasswordUserLogType.php', 'PhabricatorResourceSite' => 'aphront/site/PhabricatorResourceSite.php', - 'PhabricatorRobotsController' => 'applications/system/controller/PhabricatorRobotsController.php', - 'PhabricatorRobotsPlatformController' => 'applications/system/controller/PhabricatorRobotsPlatformController.php', - 'PhabricatorRobotsResourceController' => 'applications/system/controller/PhabricatorRobotsResourceController.php', + 'PhabricatorRobotsBlogController' => 'applications/system/controller/robots/PhabricatorRobotsBlogController.php', + 'PhabricatorRobotsController' => 'applications/system/controller/robots/PhabricatorRobotsController.php', + 'PhabricatorRobotsPlatformController' => 'applications/system/controller/robots/PhabricatorRobotsPlatformController.php', + 'PhabricatorRobotsResourceController' => 'applications/system/controller/robots/PhabricatorRobotsResourceController.php', + 'PhabricatorRobotsShortController' => 'applications/system/controller/robots/PhabricatorRobotsShortController.php', 'PhabricatorS3FileStorageEngine' => 'applications/files/engine/PhabricatorS3FileStorageEngine.php', 'PhabricatorSMSAuthFactor' => 'applications/auth/factor/PhabricatorSMSAuthFactor.php', 'PhabricatorSQLPatchList' => 'infrastructure/storage/patch/PhabricatorSQLPatchList.php', @@ -11474,9 +11476,11 @@ phutil_register_library_map(array( 'PhabricatorRequestExceptionHandler' => 'AphrontRequestExceptionHandler', 'PhabricatorResetPasswordUserLogType' => 'PhabricatorUserLogType', 'PhabricatorResourceSite' => 'PhabricatorSite', + 'PhabricatorRobotsBlogController' => 'PhabricatorRobotsController', 'PhabricatorRobotsController' => 'PhabricatorController', 'PhabricatorRobotsPlatformController' => 'PhabricatorRobotsController', 'PhabricatorRobotsResourceController' => 'PhabricatorRobotsController', + 'PhabricatorRobotsShortController' => 'PhabricatorRobotsController', 'PhabricatorS3FileStorageEngine' => 'PhabricatorFileStorageEngine', 'PhabricatorSMSAuthFactor' => 'PhabricatorAuthFactor', 'PhabricatorSQLPatchList' => 'Phobject', diff --git a/src/applications/phame/application/PhabricatorPhameApplication.php b/src/applications/phame/application/PhabricatorPhameApplication.php index 9b85a827c1..cd2eb4b487 100644 --- a/src/applications/phame/application/PhabricatorPhameApplication.php +++ b/src/applications/phame/application/PhabricatorPhameApplication.php @@ -70,7 +70,11 @@ final class PhabricatorPhameApplication extends PhabricatorApplication { } public function getBlogRoutes() { - return $this->getLiveRoutes(); + return $this->getLiveRoutes() + array( + '/status/' => 'PhabricatorStatusController', + '/favicon.ico' => 'PhabricatorFaviconController', + '/robots.txt' => 'PhabricatorRobotsBlogController', + ); } private function getLiveRoutes() { diff --git a/src/applications/phurl/application/PhabricatorPhurlApplication.php b/src/applications/phurl/application/PhabricatorPhurlApplication.php index ebff17186b..14ffda77f1 100644 --- a/src/applications/phurl/application/PhabricatorPhurlApplication.php +++ b/src/applications/phurl/application/PhabricatorPhurlApplication.php @@ -55,6 +55,10 @@ final class PhabricatorPhurlApplication extends PhabricatorApplication { public function getShortRoutes() { return array( + '/status/' => 'PhabricatorStatusController', + '/favicon.ico' => 'PhabricatorFaviconController', + '/robots.txt' => 'PhabricatorRobotsShortController', + '/u/(?P[^/]+)' => 'PhabricatorPhurlShortURLController', '.*' => 'PhabricatorPhurlShortURLDefaultController', ); diff --git a/src/applications/system/controller/robots/PhabricatorRobotsBlogController.php b/src/applications/system/controller/robots/PhabricatorRobotsBlogController.php new file mode 100644 index 0000000000..3b26acd6ee --- /dev/null +++ b/src/applications/system/controller/robots/PhabricatorRobotsBlogController.php @@ -0,0 +1,17 @@ +newRobotsRules(); - // Add a small crawl delay (number of seconds between requests) for spiders - // which respect it. The intent here is to prevent spiders from affecting - // performance for users. The possible cost is slower indexing, but that - // seems like a reasonable tradeoff, since most Phabricator installs are - // probably not hugely concerned about cutting-edge SEO. - $out[] = 'Crawl-delay: 1'; - $content = implode("\n", $out)."\n"; return id(new AphrontPlainTextResponse()) diff --git a/src/applications/system/controller/PhabricatorRobotsPlatformController.php b/src/applications/system/controller/robots/PhabricatorRobotsPlatformController.php similarity index 66% rename from src/applications/system/controller/PhabricatorRobotsPlatformController.php rename to src/applications/system/controller/robots/PhabricatorRobotsPlatformController.php index 5132e94e87..b4a3c4fa37 100644 --- a/src/applications/system/controller/PhabricatorRobotsPlatformController.php +++ b/src/applications/system/controller/robots/PhabricatorRobotsPlatformController.php @@ -19,6 +19,13 @@ final class PhabricatorRobotsPlatformController $out[] = 'Disallow: /diffusion/'; $out[] = 'Disallow: /source/'; + // Add a small crawl delay (number of seconds between requests) for spiders + // which respect it. The intent here is to prevent spiders from affecting + // performance for users. The possible cost is slower indexing, but that + // seems like a reasonable tradeoff, since most Phabricator installs are + // probably not hugely concerned about cutting-edge SEO. + $out[] = 'Crawl-delay: 1'; + return $out; } diff --git a/src/applications/system/controller/PhabricatorRobotsResourceController.php b/src/applications/system/controller/robots/PhabricatorRobotsResourceController.php similarity index 90% rename from src/applications/system/controller/PhabricatorRobotsResourceController.php rename to src/applications/system/controller/robots/PhabricatorRobotsResourceController.php index da0db4fe95..367d080c5d 100644 --- a/src/applications/system/controller/PhabricatorRobotsResourceController.php +++ b/src/applications/system/controller/robots/PhabricatorRobotsResourceController.php @@ -10,6 +10,7 @@ final class PhabricatorRobotsResourceController $out[] = 'User-Agent: *'; $out[] = 'Disallow: /'; + $out[] = 'Crawl-delay: 1'; return $out; } diff --git a/src/applications/system/controller/robots/PhabricatorRobotsShortController.php b/src/applications/system/controller/robots/PhabricatorRobotsShortController.php new file mode 100644 index 0000000000..efc906b168 --- /dev/null +++ b/src/applications/system/controller/robots/PhabricatorRobotsShortController.php @@ -0,0 +1,18 @@ + Date: Thu, 11 Mar 2021 14:28:41 -0800 Subject: [PATCH 11/13] Provide more help around GRANT errors, particularly for missing TEMPORARY TABLE permission Summary: Fixes T13622. Figuring out what permissions we have seems difficult, so address this a bit more narrowly: - Make the "access denied" error message a bit more helpful. - Tailor error handling for the "CREATE TEMPORARY TABLE" statement. Test Plan: - Created a new user, granted them "SELECT ON *.*" but not "CREATE TEMPORARY TABLE", ran `bin/storage upgrade --force --apply phabricator:20210215.changeset.02.phid-populate.php`. - Before: fairly opaque error. - After: fairly useful error. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13622 Differential Revision: https://secure.phabricator.com/D21608 --- .../20210215.changeset.02.phid-populate.php | 20 +++++++++++++------ .../AphrontBaseMySQLDatabaseConnection.php | 11 +++++++++- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/resources/sql/autopatches/20210215.changeset.02.phid-populate.php b/resources/sql/autopatches/20210215.changeset.02.phid-populate.php index 2f35a18172..93f886c7b0 100644 --- a/resources/sql/autopatches/20210215.changeset.02.phid-populate.php +++ b/resources/sql/autopatches/20210215.changeset.02.phid-populate.php @@ -11,12 +11,20 @@ $chunk_size = 4096; $temporary_table = 'tmp_20210215_changeset_id_map'; -queryfx( - $conn, - 'CREATE TEMPORARY TABLE %T ( - changeset_id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, - changeset_phid VARBINARY(64) NOT NULL)', - $temporary_table); +try { + queryfx( + $conn, + 'CREATE TEMPORARY TABLE %T ( + changeset_id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + changeset_phid VARBINARY(64) NOT NULL)', + $temporary_table); +} catch (AphrontAccessDeniedQueryException $ex) { + throw new PhutilProxyException( + pht( + 'Failed to "CREATE TEMPORARY TABLE". You may need to "GRANT" the '. + 'current MySQL user this permission.'), + $ex); +} $table_iterator = id(new LiskRawMigrationIterator($conn, $table_name)) ->setPageSize($chunk_size); diff --git a/src/infrastructure/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php b/src/infrastructure/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php index 6faf10e2c6..db52eebe8e 100644 --- a/src/infrastructure/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php +++ b/src/infrastructure/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php @@ -347,7 +347,16 @@ abstract class AphrontBaseMySQLDatabaseConnection case 1142: // Access denied to table case 1143: // Access denied to column case 1227: // Access denied (e.g., no SUPER for SHOW SLAVE STATUS). - throw new AphrontAccessDeniedQueryException($message); + + // See T13622. Try to help users figure out that this is a GRANT + // problem. + + $more = pht( + 'This error usually indicates that you need to "GRANT" the '. + 'MySQL user additional permissions. See "GRANT" in the MySQL '. + 'manual for help.'); + + throw new AphrontAccessDeniedQueryException("{$message}\n\n{$more}"); case 1045: // Access denied (auth) throw new AphrontInvalidCredentialsQueryException($message); case 1146: // No such table From 4b529e6009285a7f8a850e6b1f35e1bc2ea4c30c Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 12 Mar 2021 09:06:48 -0800 Subject: [PATCH 12/13] Fix a followup notification paging error with partial objects Summary: Ref T13623. In D21603, I made the "partial object" this query returns a raw row, which paging keys can no longer be extracted from properly. Test Plan: Paged notifications to page 2, no longer saw an error. Maniphest Tasks: T13623 Differential Revision: https://secure.phabricator.com/D21609 --- .../notification/query/PhabricatorNotificationQuery.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/notification/query/PhabricatorNotificationQuery.php b/src/applications/notification/query/PhabricatorNotificationQuery.php index 69eb41695b..2cd97f25c8 100644 --- a/src/applications/notification/query/PhabricatorNotificationQuery.php +++ b/src/applications/notification/query/PhabricatorNotificationQuery.php @@ -181,7 +181,7 @@ final class PhabricatorNotificationQuery protected function newPagingMapFromPartialObject($object) { return array( - 'key' => $object->getChronologicalKey(), + 'key' => $object['chronologicalKey'], ); } From b11c6fcacd8bceafc2a2f223037952dc817f4d7f Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 12 Mar 2021 09:12:09 -0800 Subject: [PATCH 13/13] Clarify the behavior of "audit.can-author-close-audit" Summary: Ref T13631. This option has a behavior other than the behavior implied by the name and documented. Document the correct behavior, at least. This can likely be removed after T10574. Test Plan: Read config option help in Config. Maniphest Tasks: T13631 Differential Revision: https://secure.phabricator.com/D21610 --- .../config/PhabricatorDiffusionConfigOptions.php | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/applications/diffusion/config/PhabricatorDiffusionConfigOptions.php b/src/applications/diffusion/config/PhabricatorDiffusionConfigOptions.php index a0c2277f6e..be889b4cc4 100644 --- a/src/applications/diffusion/config/PhabricatorDiffusionConfigOptions.php +++ b/src/applications/diffusion/config/PhabricatorDiffusionConfigOptions.php @@ -61,16 +61,22 @@ final class PhabricatorDiffusionConfigOptions ->setDescription(pht('Hard byte limit on including patches in email.')), $this->newOption('metamta.diffusion.time-limit', 'int', 60) ->setDescription(pht('Hard time limit on generating patches.')), + $this->newOption( 'audit.can-author-close-audit', 'bool', false) ->setBoolOptions( array( - pht('Enable Closing Audits'), - pht('Disable Closing Audits'), + pht('Enable Self-Accept'), + pht('Disable Self-Accept'), )) - ->setDescription(pht('Controls whether Author can Close Audits.')), + ->setDescription( + pht( + 'Allows the author of a commit to be an auditor and accept their '. + 'own commits. Note that this behavior is different from the '. + 'behavior implied by the name of the option: long ago, it did '. + 'something else.')), $this->newOption('bugtraq.url', 'string', null) ->addExample('https://bugs.php.net/%BUGID%', pht('PHP bugs'))