From a5a9a5e0027f5dbafd1d12429ae965a07d999ff5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 23 Jan 2020 11:32:11 -0800 Subject: [PATCH] Remove legacy pre-loading of handles from Herald rendering Summary: Ref T13480. When Herald renders rules, it partly uses a very old handle pre-loading mechanism where PHIDs are extracted and loaded upfront. This was obsoleted a long time ago and was pretty shaky even when it worked. Get rid of it to simplify the code a little. Test Plan: Viewed Herald rules rendered into static text with PHID list actions, saw handles. Grepped for all affected methods. Maniphest Tasks: T13480 Differential Revision: https://secure.phabricator.com/D20948 --- .../herald/adapter/HeraldAdapter.php | 107 ++---------------- .../controller/HeraldRuleViewController.php | 5 +- 2 files changed, 13 insertions(+), 99 deletions(-) diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 70f6e3d5cb..5c6f8c9954 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -942,7 +942,6 @@ abstract class HeraldAdapter extends Phobject { public function renderRuleAsText( HeraldRule $rule, - PhabricatorHandleList $handles, PhabricatorUser $viewer) { require_celerity_resource('herald-css'); @@ -973,7 +972,7 @@ abstract class HeraldAdapter extends Phobject { ), array( $icon, - $this->renderConditionAsText($condition, $handles, $viewer), + $this->renderConditionAsText($condition, $viewer), )); } @@ -1004,7 +1003,7 @@ abstract class HeraldAdapter extends Phobject { ), array( $icon, - $this->renderActionAsText($viewer, $action, $handles), + $this->renderActionAsText($viewer, $action), )); } @@ -1018,7 +1017,6 @@ abstract class HeraldAdapter extends Phobject { private function renderConditionAsText( HeraldCondition $condition, - PhabricatorHandleList $handles, PhabricatorUser $viewer) { $field_type = $condition->getFieldName(); @@ -1033,7 +1031,7 @@ abstract class HeraldAdapter extends Phobject { $condition_type = $condition->getFieldCondition(); $condition_name = idx($this->getConditionNameMap(), $condition_type); - $value = $this->renderConditionValueAsText($condition, $handles, $viewer); + $value = $this->renderConditionValueAsText($condition, $viewer); return array( $field_name, @@ -1046,36 +1044,23 @@ abstract class HeraldAdapter extends Phobject { private function renderActionAsText( PhabricatorUser $viewer, - HeraldActionRecord $action, - PhabricatorHandleList $handles) { + HeraldActionRecord $action_record) { - $impl = $this->getActionImplementation($action->getAction()); - if ($impl) { - $impl->setViewer($viewer); + $action_type = $action_record->getAction(); + $action_value = $action_record->getTarget(); - $value = $action->getTarget(); - return $impl->renderActionDescription($value); + $action = $this->getActionImplementation($action_type); + if (!$action) { + return pht('Unknown Action ("%s")', $action_type); } - $rule_global = HeraldRuleTypeConfig::RULE_TYPE_GLOBAL; + $action->setViewer($viewer); - $action_type = $action->getAction(); - - $default = pht('(Unknown Action "%s") equals', $action_type); - - $action_name = idx( - $this->getActionNameMap($rule_global), - $action_type, - $default); - - $target = $this->renderActionTargetAsText($action, $handles); - - return hsprintf(' %s %s', $action_name, $target); + return $action->renderActionDescription($action_value); } private function renderConditionValueAsText( HeraldCondition $condition, - PhabricatorHandleList $handles, PhabricatorUser $viewer) { $field = $this->requireFieldImplementation($condition->getFieldName()); @@ -1086,76 +1071,6 @@ abstract class HeraldAdapter extends Phobject { $condition->getValue()); } - private function renderActionTargetAsText( - HeraldActionRecord $action, - PhabricatorHandleList $handles) { - - // TODO: This should be driven through HeraldAction. - - $target = $action->getTarget(); - if (!is_array($target)) { - $target = array($target); - } - foreach ($target as $index => $val) { - switch ($action->getAction()) { - default: - $handle = $handles->getHandleIfExists($val); - if ($handle) { - $target[$index] = $handle->renderLink(); - } - break; - } - } - $target = phutil_implode_html(', ', $target); - return $target; - } - - /** - * Given a @{class:HeraldRule}, this function extracts all the phids that - * we'll want to load as handles later. - * - * This function performs a somewhat hacky approach to figuring out what - * is and is not a phid - try to get the phid type and if the type is - * *not* unknown assume its a valid phid. - * - * Don't try this at home. Use more strongly typed data at home. - * - * Think of the children. - */ - public static function getHandlePHIDs(HeraldRule $rule) { - $phids = array($rule->getAuthorPHID()); - foreach ($rule->getConditions() as $condition) { - $value = $condition->getValue(); - if (!is_array($value)) { - $value = array($value); - } - foreach ($value as $val) { - if (phid_get_type($val) != - PhabricatorPHIDConstants::PHID_TYPE_UNKNOWN) { - $phids[] = $val; - } - } - } - - foreach ($rule->getActions() as $action) { - $target = $action->getTarget(); - if (!is_array($target)) { - $target = array($target); - } - foreach ($target as $val) { - if (phid_get_type($val) != - PhabricatorPHIDConstants::PHID_TYPE_UNKNOWN) { - $phids[] = $val; - } - } - } - - if ($rule->isObjectRule()) { - $phids[] = $rule->getTriggerObjectPHID(); - } - - return $phids; - } /* -( Applying Effects )--------------------------------------------------- */ diff --git a/src/applications/herald/controller/HeraldRuleViewController.php b/src/applications/herald/controller/HeraldRuleViewController.php index 70a2d20224..49e0fd8e32 100644 --- a/src/applications/herald/controller/HeraldRuleViewController.php +++ b/src/applications/herald/controller/HeraldRuleViewController.php @@ -143,12 +143,11 @@ final class HeraldRuleViewController extends HeraldController { private function buildDescriptionView(HeraldRule $rule) { $viewer = $this->getRequest()->getUser(); $view = id(new PHUIPropertyListView()) - ->setUser($viewer); + ->setViewer($viewer); $adapter = HeraldAdapter::getAdapterForContentType($rule->getContentType()); if ($adapter) { - $handles = $viewer->loadHandles(HeraldAdapter::getHandlePHIDs($rule)); - $rule_text = $adapter->renderRuleAsText($rule, $handles, $viewer); + $rule_text = $adapter->renderRuleAsText($rule, $viewer); $view->addTextContent($rule_text); return $view; }