@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator

Add "requestedObjectPHID" to ReleephRequest

Summary:
Ref T3551. Currently, ReleephRequests don't have a direct concept of the //object// being requested. You can request `D123`, but that is just a convenient way to write `rXyyyy`.

When the UI wants to display information about a revision, it deduces it by examining the commit.

This is primarily an attack on T3551, so we don't need to load <commit -> edge -> revision> (in an ad-hoc way) to get revisions. Instead, when you request a revision we keep track of it and can load it directly later.

Later, this will let us do more things: for example, if you request a branch, we can automatically update the commits (as GitHub does), etc. (Repository branches will need PHIDs first, of course.)

This adds and populates the column but doesn't use it yet. The second part of the migration could safely be run while Phabricator is up, although even for Facebook this table is probably quite small.

Test Plan:
- Ran migration.
- Verified existing requests associated sensibly.
- Created a new commit request.
- Created a new revision request.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3551

Differential Revision: https://secure.phabricator.com/D8822

+112 -3
+5
resources/sql/autopatches/20140420.rel.1.objectphid.sql
··· 1 + ALTER TABLE {$NAMESPACE}_releeph.releeph_request 2 + ADD COLUMN requestedObjectPHID VARCHAR(64) COLLATE utf8_bin NOT NULL; 3 + 4 + ALTER TABLE {$NAMESPACE}_releeph.releeph_request 5 + ADD KEY `key_requestedObject` (requestedObjectPHID);
+45
resources/sql/autopatches/20140420.rel.2.objectmig.php
··· 1 + <?php 2 + 3 + $pull_table = new ReleephRequest(); 4 + $table_name = $pull_table->getTableName(); 5 + $conn_w = $pull_table->establishConnection('w'); 6 + 7 + echo "Setting object PHIDs for requests...\n"; 8 + foreach (new LiskMigrationIterator($pull_table) as $pull) { 9 + $id = $pull->getID(); 10 + 11 + echo "Migrating pull request {$id}...\n"; 12 + if ($pull->getRequestedObjectPHID()) { 13 + // We already have a valid PHID, so skip this request. 14 + continue; 15 + } 16 + 17 + $commit_phids = $pull->getCommitPHIDs(); 18 + if (count($commit_phids) != 1) { 19 + // At the time this migration was written, all requests had exactly one 20 + // commit. If a request has more than one, we don't have the information 21 + // we need to process it. 22 + continue; 23 + } 24 + 25 + $commit_phid = head($commit_phids); 26 + 27 + $revision_phids = PhabricatorEdgeQuery::loadDestinationPHIDs( 28 + $commit_phid, 29 + PhabricatorEdgeConfig::TYPE_COMMIT_HAS_DREV); 30 + 31 + if ($revision_phids) { 32 + $object_phid = head($revision_phids); 33 + } else { 34 + $object_phid = $commit_phid; 35 + } 36 + 37 + queryfx( 38 + $conn_w, 39 + 'UPDATE %T SET requestedObjectPHID = %s WHERE id = %d', 40 + $table_name, 41 + $object_phid, 42 + $id); 43 + } 44 + 45 + echo "Done.\n";
+21 -1
src/applications/releeph/commitfinder/ReleephCommitFinder.php
··· 4 4 5 5 private $releephProject; 6 6 private $user; 7 + private $objectPHID; 7 8 8 9 public function setUser(PhabricatorUser $user) { 9 10 $this->user = $user; ··· 18 19 return $this; 19 20 } 20 21 22 + public function getRequestedObjectPHID() { 23 + return $this->objectPHID; 24 + } 25 + 21 26 public function fromPartial($partial_string) { 27 + $this->objectPHID = null; 28 + 22 29 // Look for diffs 23 30 $matches = array(); 24 31 if (preg_match('/^D([1-9]\d*)$/', $partial_string, $matches)) { ··· 36 43 "{$partial_string} has no commits associated with it yet."); 37 44 } 38 45 46 + $this->objectPHID = $diff_rev->getPHID(); 47 + 39 48 $commits = id(new PhabricatorRepositoryCommit())->loadAllWhere( 40 49 'phid IN (%Ls) ORDER BY epoch ASC', 41 50 $commit_phids); ··· 43 52 } 44 53 45 54 // Look for a raw commit number, or r<callsign><commit-number>. 46 - $repository = $this->releephProject->loadPhabricatorRepository(); 55 + $repository = $this->releephProject->getRepository(); 47 56 $dr_data = null; 48 57 $matches = array(); 49 58 if (preg_match('/^r(?P<callsign>[A-Z]+)(?P<commit>\w+)$/', ··· 77 86 if (!$phabricator_repository_commit) { 78 87 throw new ReleephCommitFinderException( 79 88 "The commit {$partial_string} doesn't exist in this repository."); 89 + } 90 + 91 + // When requesting a single commit, if it has an associated review we 92 + // imply the review was requested instead. This is always correct for now 93 + // and consistent with the older behavior, although it might not be the 94 + // right rule in the future. 95 + $phids = PhabricatorEdgeQuery::loadDestinationPHIDs( 96 + $phabricator_repository_commit->getPHID(), 97 + PhabricatorEdgeConfig::TYPE_COMMIT_HAS_DREV); 98 + if ($phids) { 99 + $this->objectPHID = head($phids); 80 100 } 81 101 82 102 return $phabricator_repository_commit;
+8 -1
src/applications/releeph/conduit/ConduitAPI_releeph_request_Method.php
··· 49 49 50 50 // Find the requested commit identifiers 51 51 $requested_commits = array(); 52 + $requested_object_phids = array(); 52 53 $things = $request->getValue('things'); 53 54 $finder = id(new ReleephCommitFinder()) 54 55 ->setUser($user) ··· 56 57 foreach ($things as $thing) { 57 58 try { 58 59 $requested_commits[$thing] = $finder->fromPartial($thing); 60 + $object_phid = $finder->getRequestedObjectPHID(); 61 + if (!$object_phid) { 62 + $object_phid = $requested_commits[$thing]->getPHID(); 63 + } 64 + $requested_object_phids[$thing] = $object_phid; 59 65 } catch (ReleephCommitFinderException $ex) { 60 66 throw id(new ConduitException('ERR_NO_MATCHES')) 61 67 ->setErrorDescription($ex->getMessage()); ··· 99 105 $releeph_request = id(new ReleephRequest()) 100 106 ->setRequestUserPHID($user->getPHID()) 101 107 ->setBranchID($releeph_branch->getID()) 102 - ->setInBranch(0); 108 + ->setInBranch(0) 109 + ->setRequestedObjectPHID($requested_object_phids[$thing]); 103 110 104 111 $xactions = array(); 105 112
+11 -1
src/applications/releeph/controller/request/ReleephRequestEditController.php
··· 43 43 $pull = id(new ReleephRequest()) 44 44 ->setRequestUserPHID($viewer->getPHID()) 45 45 ->setBranchID($branch->getID()) 46 - ->setInBranch(0); 46 + ->setInBranch(0) 47 + ->attachBranch($branch); 47 48 48 49 $is_edit = false; 49 50 } ··· 115 116 $e_request_identifier = 'Not parsed yet'; 116 117 $errors[] = "The requested commit hasn't been parsed yet."; 117 118 } 119 + } 120 + 121 + if (!$errors) { 122 + $object_phid = $finder->getRequestedObjectPHID(); 123 + if (!$object_phid) { 124 + $object_phid = $pr_commit->getPHID(); 125 + } 126 + 127 + $pull->setRequestedObjectPHID($object_phid); 118 128 } 119 129 } 120 130
+22
src/applications/releeph/storage/ReleephRequest.php
··· 13 13 protected $pickStatus; 14 14 protected $mailKey; 15 15 16 + /** 17 + * The object which is being requested. Normally this is a commit, but it 18 + * might also be a revision. In the future, it could be a repository branch 19 + * or an external object (like a GitHub pull request). 20 + */ 21 + protected $requestedObjectPHID; 22 + 16 23 // Information about the thing being requested 17 24 protected $requestCommitPHID; 18 25 19 26 // Information about the last commit to the releeph branch 20 27 protected $commitIdentifier; 21 28 protected $commitPHID; 29 + 22 30 23 31 private $customFields = self::ATTACHABLE; 24 32 private $branch = self::ATTACHABLE; ··· 161 169 public function setDetail($key, $value) { 162 170 $this->details[$key] = $value; 163 171 return $this; 172 + } 173 + 174 + 175 + /** 176 + * Get the commit PHIDs this request is requesting. 177 + * 178 + * NOTE: For now, this always returns one PHID. 179 + * 180 + * @return list<phid> Commit PHIDs requested by this request. 181 + */ 182 + public function getCommitPHIDs() { 183 + return array( 184 + $this->requestCommitPHID, 185 + ); 164 186 } 165 187 166 188 public function getReason() {