recaptime.dev / phorge
@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
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Fix a Herald issue where testing commits against rules with revision-related conditions would fail

Summary:
Fixes T11610. Clean up some sketchy old code from long ago.

If you had rules that use conditions like "Accepted revision exists" and ran them in the test console, we'd never load the "CommitData" and fatal.

Instead, load CommitData in `newTestAdapter()` and generally make these pathways a little more modern.

Test Plan:
- Wrote an "Accepted Revision Exists" rule.
- Ran a commit in the test console.
- Before patch, got fatal from T11610.
- After patch, got clean test result.
- Also pushed a commit and reviewed the transcript to make sure the rule ran properly.

Reviewers: joshuaspence, chad

Reviewed By: chad

Maniphest Tasks: T11610

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

epriestley 8d048f06 17cd119b

+27 -34
+1 -2
src/applications/audit/editor/PhabricatorAuditEditor.php
··· 886 886 protected function buildHeraldAdapter( 887 887 PhabricatorLiskDAO $object, 888 888 array $xactions) { 889 - 890 889 return id(new HeraldCommitAdapter()) 891 - ->setCommit($object); 890 + ->setObject($object); 892 891 } 893 892 894 893 protected function didApplyHeraldRules(
+1 -1
src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php
··· 29 29 'Test rules which run when a revision is created or updated.'); 30 30 } 31 31 32 - public function newTestAdapter($object) { 32 + public function newTestAdapter(PhabricatorUser $viewer, $object) { 33 33 return self::newLegacyAdapter( 34 34 $object, 35 35 $object->loadActiveDiff());
+21 -29
src/applications/diffusion/herald/HeraldCommitAdapter.php
··· 8 8 protected $revision; 9 9 10 10 protected $commit; 11 - protected $commitData; 12 11 private $commitDiff; 13 12 14 13 protected $affectedPaths; ··· 33 32 public function getAdapterTestDescription() { 34 33 return pht( 35 34 'Test rules which run after a commit is discovered and imported.'); 35 + } 36 + 37 + public function newTestAdapter(PhabricatorUser $viewer, $object) { 38 + $object = id(new DiffusionCommitQuery()) 39 + ->setViewer($viewer) 40 + ->withPHIDs(array($object->getPHID())) 41 + ->needCommitData(true) 42 + ->executeOne(); 43 + if (!$object) { 44 + throw new Exception( 45 + pht( 46 + 'Failed to reload commit ("%s") to fetch commit data.', 47 + $object->getPHID())); 48 + } 49 + 50 + return id(clone $this) 51 + ->setObject($object); 36 52 } 37 53 38 54 protected function initializeNewAdapter() { ··· 100 116 return pht('This rule can trigger for **repositories** and **projects**.'); 101 117 } 102 118 103 - public function setCommit(PhabricatorRepositoryCommit $commit) { 104 - $viewer = PhabricatorUser::getOmnipotentUser(); 105 - 106 - $repository = id(new PhabricatorRepositoryQuery()) 107 - ->setViewer($viewer) 108 - ->withIDs(array($commit->getRepositoryID())) 109 - ->executeOne(); 110 - if (!$repository) { 111 - throw new Exception(pht('Unable to load repository!')); 112 - } 113 - 114 - $data = id(new PhabricatorRepositoryCommitData())->loadOneWhere( 115 - 'commitID = %d', 116 - $commit->getID()); 117 - if (!$data) { 118 - throw new Exception(pht('Unable to load commit data!')); 119 - } 120 - 121 - $this->commit = clone $commit; 122 - $this->commit->attachRepository($repository); 123 - $this->commit->attachCommitData($data); 124 - 125 - $this->commitData = $data; 126 - 127 - return $this; 128 - } 129 - 130 119 public function getHeraldName() { 131 120 return $this->commit->getMonogram(); 132 121 } ··· 171 160 public function loadDifferentialRevision() { 172 161 if ($this->affectedRevision === null) { 173 162 $this->affectedRevision = false; 174 - $data = $this->commitData; 163 + 164 + $commit = $this->getObject(); 165 + $data = $commit->getCommitData(); 166 + 175 167 $revision_id = $data->getCommitDetail('differential.revisionID'); 176 168 if ($revision_id) { 177 169 // NOTE: The Herald rule owner might not actually have access to
+1 -1
src/applications/herald/adapter/HeraldAdapter.php
··· 224 224 return $this->isTestAdapterForObject($object); 225 225 } 226 226 227 - public function newTestAdapter($object) { 227 + public function newTestAdapter(PhabricatorUser $viewer, $object) { 228 228 return id(clone $this) 229 229 ->setObject($object); 230 230 }
+3 -1
src/applications/herald/controller/HeraldTestConsoleController.php
··· 142 142 143 143 if ($request->isFormPost() && $adapter_key) { 144 144 if (isset($can_select[$adapter_key])) { 145 - $adapter = $can_select[$adapter_key]->newTestAdapter($object); 145 + $adapter = $can_select[$adapter_key]->newTestAdapter( 146 + $viewer, 147 + $object); 146 148 $this->setTestAdapter($adapter); 147 149 return null; 148 150 }