From d3020af97b92c7799981e1b0a3cf0715e2cf4ac9 Mon Sep 17 00:00:00 2001 From: Anh Nhan Nguyen Date: Thu, 4 Apr 2013 13:10:18 -0700 Subject: [PATCH] Uniformized handle data Summary: This sets more reasonable values for the object handle fields imo. It's not like I ever want to find out what letter to use and then do `substr($handle->getType(), 0, 1).$handle->getID()` to get `D1` each time I use handles. Name: - D1 - T1 - M1 - P1 - etc. Fullname: - D1: Something - T1: Something - etc. In addition, this helps me to reasonable prefill Hovercards in case there is no application-specific event listener. Also deletes `title` and `alternateID` completely. They deserved that. Test Plan: Visited places, nothing broke (We only ever used `$handle->getName()` for users and commits). Tested mail reply handler. Did not test the other way around, but should be fine. Hovercards broken until D5572 (would love to induce a cyclic dependency) Reviewers: epriestley, chad, btrahan Reviewed By: epriestley CC: aran, Korvin Differential Revision: https://secure.phabricator.com/D5571 --- ...entialManiphestTasksFieldSpecification.php | 2 +- .../PhabricatorMailReplyHandler.php | 6 +++- .../phid/PhabricatorObjectHandle.php | 19 ------------ .../handle/PhabricatorObjectHandleData.php | 31 ++++++++++++------- .../examples/PhabricatorUIExample.php | 1 - 5 files changed, 26 insertions(+), 33 deletions(-) diff --git a/src/applications/differential/field/specification/DifferentialManiphestTasksFieldSpecification.php b/src/applications/differential/field/specification/DifferentialManiphestTasksFieldSpecification.php index 2c984dda04..cfcea7208a 100644 --- a/src/applications/differential/field/specification/DifferentialManiphestTasksFieldSpecification.php +++ b/src/applications/differential/field/specification/DifferentialManiphestTasksFieldSpecification.php @@ -115,7 +115,7 @@ final class DifferentialManiphestTasksFieldSpecification $names = array(); foreach ($this->maniphestTasks as $phid) { $handle = $this->getHandle($phid); - $names[] = 'T'.$handle->getAlternateID(); + $names[] = $handle->getName(); } return implode(', ', $names); } diff --git a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php b/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php index 7970aeb571..67d8550edc 100644 --- a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php +++ b/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php @@ -285,9 +285,13 @@ EOBODY; return null; } + $user = head(id(new PhabricatorPeopleQuery()) + ->withPhids(array($handle->getPHID())) + ->execute()); + $receiver = $this->getMailReceiver(); $receiver_id = $receiver->getID(); - $user_id = $handle->getAlternateID(); + $user_id = $user->getID(); $hash = PhabricatorMetaMTAReceivedMail::computeMailHash( $receiver->getMailKey(), $handle->getPHID()); diff --git a/src/applications/phid/PhabricatorObjectHandle.php b/src/applications/phid/PhabricatorObjectHandle.php index 7814b76bcc..1ded1abc27 100644 --- a/src/applications/phid/PhabricatorObjectHandle.php +++ b/src/applications/phid/PhabricatorObjectHandle.php @@ -6,13 +6,10 @@ final class PhabricatorObjectHandle { private $phid; private $type; private $name; - private $email; private $fullName; private $imageURI; private $timestamp; - private $alternateID; private $status = PhabricatorObjectHandleStatus::STATUS_OPEN; - private $title; private $complete; private $disabled; @@ -52,11 +49,6 @@ final class PhabricatorObjectHandle { return $this->status; } - public function setTitle($title) { - $this->title = $title; - return $this; - } - public function setFullName($full_name) { $this->fullName = $full_name; return $this; @@ -96,15 +88,6 @@ final class PhabricatorObjectHandle { return $this->timestamp; } - public function setAlternateID($alternate_id) { - $this->alternateID = $alternate_id; - return $this; - } - - public function getAlternateID() { - return $this->alternateID; - } - public function getTypeName() { static $map = array( PhabricatorPHIDConstants::PHID_TYPE_USER => 'User', @@ -190,7 +173,6 @@ final class PhabricatorObjectHandle { if ($this->status != PhabricatorObjectHandleStatus::STATUS_OPEN) { $class .= ' handle-status-'.$this->status; - $title = (isset($this->title) ? $this->title : $this->status); } if ($this->disabled) { @@ -203,7 +185,6 @@ final class PhabricatorObjectHandle { array( 'href' => $this->getURI(), 'class' => $class, - 'title' => $title, ), $name); } diff --git a/src/applications/phid/handle/PhabricatorObjectHandleData.php b/src/applications/phid/handle/PhabricatorObjectHandleData.php index c08a9b25b9..d94944138f 100644 --- a/src/applications/phid/handle/PhabricatorObjectHandleData.php +++ b/src/applications/phid/handle/PhabricatorObjectHandleData.php @@ -41,6 +41,7 @@ final class PhabricatorObjectHandleData { switch ($type) { case PhabricatorPHIDConstants::PHID_TYPE_USER: + // TODO: Update query + Batch User Images $user_dao = new PhabricatorUser(); $users = $user_dao->loadAllWhere( 'phid in (%Ls)', @@ -55,6 +56,8 @@ final class PhabricatorObjectHandleData { return mpull($commits, null, 'getPHID'); case PhabricatorPHIDConstants::PHID_TYPE_TASK: + // TODO: Update this to ManiphestTaskQuery, //especially// after we have + // policy-awareness $task_dao = new ManiphestTask(); $tasks = $task_dao->loadAllWhere( 'phid IN (%Ls)', @@ -69,6 +72,7 @@ final class PhabricatorObjectHandleData { return mpull($entries, null, 'getPHID'); case PhabricatorPHIDConstants::PHID_TYPE_FILE: + // TODO: Update this to PhabricatorFileQuery $object = new PhabricatorFile(); $files = $object->loadAllWhere('phid IN (%Ls)', $phids); return mpull($files, null, 'getPHID'); @@ -81,6 +85,7 @@ final class PhabricatorObjectHandleData { return mpull($projects, null, 'getPHID'); case PhabricatorPHIDConstants::PHID_TYPE_REPO: + // TODO: Update this to PhabricatorRepositoryQuery $object = new PhabricatorRepository(); $repositories = $object->loadAllWhere('phid in (%Ls)', $phids); return mpull($repositories, null, 'getPHID'); @@ -103,6 +108,7 @@ final class PhabricatorObjectHandleData { return mpull($lists, null, 'getPHID'); case PhabricatorPHIDConstants::PHID_TYPE_DREV: + // TODO: Update this to DifferentialRevisionQuery $revision_dao = new DifferentialRevision(); $revisions = $revision_dao->loadAllWhere( 'phid IN (%Ls)', @@ -110,6 +116,8 @@ final class PhabricatorObjectHandleData { return mpull($revisions, null, 'getPHID'); case PhabricatorPHIDConstants::PHID_TYPE_WIKI: + // TODO: Update this to PhrictionDocumentQuery, already pre-package + // content DAO $document_dao = new PhrictionDocument(); $documents = $document_dao->loadAllWhere( 'phid IN (%Ls)', @@ -260,12 +268,9 @@ final class PhabricatorObjectHandleData { $handle->setURI('/p/'.$user->getUsername().'/'); $handle->setFullName( $user->getUsername().' ('.$user->getRealName().')'); - $handle->setAlternateID($user->getID()); $handle->setComplete(true); if (isset($statuses[$phid])) { $handle->setStatus($statuses[$phid]->getTextStatus()); - $handle->setTitle( - $statuses[$phid]->getTerseSummary($this->viewer)); } $handle->setDisabled($user->getIsDisabled()); @@ -308,7 +313,7 @@ final class PhabricatorObjectHandleData { $handle->setName('Unknown Revision'); } else { $rev = $objects[$phid]; - $handle->setName($rev->getTitle()); + $handle->setName('D'.$rev->getID()); $handle->setURI('/D'.$rev->getID()); $handle->setFullName('D'.$rev->getID().': '.$rev->getTitle()); $handle->setComplete(true); @@ -367,11 +372,10 @@ final class PhabricatorObjectHandleData { $handle->setName('Unknown Task'); } else { $task = $objects[$phid]; - $handle->setName($task->getTitle()); + $handle->setName('T'.$task->getID()); $handle->setURI('/T'.$task->getID()); $handle->setFullName('T'.$task->getID().': '.$task->getTitle()); $handle->setComplete(true); - $handle->setAlternateID($task->getID()); if ($task->getStatus() != ManiphestTaskStatus::STATUS_OPEN) { $closed = PhabricatorObjectHandleStatus::STATUS_CLOSED; $handle->setStatus($closed); @@ -408,7 +412,8 @@ final class PhabricatorObjectHandleData { $handle->setName('Unknown File'); } else { $file = $objects[$phid]; - $handle->setName($file->getName()); + $handle->setName('F'.$file->getID()); + $handle->setFullName('F'.$file->getID().' '.$file->getName()); $handle->setURI($file->getBestURI()); $handle->setComplete(true); } @@ -442,7 +447,7 @@ final class PhabricatorObjectHandleData { $handle->setName('Unknown Repository'); } else { $repository = $objects[$phid]; - $handle->setName($repository->getCallsign()); + $handle->setName('r'.$repository->getCallsign()); $handle->setFullName("r" . $repository->getCallsign() . " (" . $repository->getName() . ")"); $handle->setURI('/diffusion/'.$repository->getCallsign().'/'); @@ -486,6 +491,7 @@ final class PhabricatorObjectHandleData { break; case PhabricatorPHIDConstants::PHID_TYPE_WIKI: + // TODO: Update this $document_dao = new PhrictionDocument(); $content_dao = new PhrictionContent(); @@ -510,6 +516,7 @@ final class PhabricatorObjectHandleData { $info = $documents[$phid]; $handle->setName($info['title']); $handle->setURI(PhrictionDocument::getSlugURI($info['slug'])); + $handle->setFullName($info['title']); $handle->setComplete(true); if ($info['status'] != PhrictionDocumentStatus::STATUS_EXISTS) { $closed = PhabricatorObjectHandleStatus::STATUS_CLOSED; @@ -529,7 +536,9 @@ final class PhabricatorObjectHandleData { $handle->setName('Unknown Ponder Question'); } else { $question = $objects[$phid]; - $handle->setName(phutil_utf8_shorten($question->getTitle(), 60)); + $handle->setName('Q' . $question->getID()); + $handle->setFullName( + phutil_utf8_shorten($question->getTitle(), 60)); $handle->setURI(new PhutilURI('/Q' . $question->getID())); $handle->setComplete(true); } @@ -546,7 +555,7 @@ final class PhabricatorObjectHandleData { $handle->setName('Unknown Paste'); } else { $paste = $objects[$phid]; - $handle->setName($paste->getTitle()); + $handle->setName('P'.$paste->getID()); $handle->setFullName($paste->getFullName()); $handle->setURI('/P'.$paste->getID()); $handle->setComplete(true); @@ -600,7 +609,7 @@ final class PhabricatorObjectHandleData { $handle->setName('Unknown Mock'); } else { $mock = $objects[$phid]; - $handle->setName($mock->getName()); + $handle->setName('M'.$mock->getID()); $handle->setFullName('M'.$mock->getID().': '.$mock->getName()); $handle->setURI('/M'.$mock->getID()); $handle->setComplete(true); diff --git a/src/applications/uiexample/examples/PhabricatorUIExample.php b/src/applications/uiexample/examples/PhabricatorUIExample.php index 7889b1ed86..0afe5447a2 100644 --- a/src/applications/uiexample/examples/PhabricatorUIExample.php +++ b/src/applications/uiexample/examples/PhabricatorUIExample.php @@ -22,7 +22,6 @@ abstract class PhabricatorUIExample { $id = mt_rand(15, 9999); $handle = new PhabricatorObjectHandle(); - $handle->setAlternateID(mt_rand(15, 9999)); $handle->setName($name); $handle->setType($type); $handle->setPHID(PhabricatorPHID::generateNewPHID($type));