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.

Stop using loadRelationships in differential related herald adapters

Summary:
Used `DifferentialRevisionQuery` with the relevant `need*()` calls in the test controller.
And started assuming the revision has reviewers and CC phids in `HeraldDifferentialRevisionAdapter`.

Test Plan:
Added herald rules that use revisions (one for revisions another for commit) and reviewers.
Created, accepted and landed a revision that matched the rules and checked all rules were applied.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T1279

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

Conflicts:
src/applications/herald/adapter/HeraldCommitAdapter.php
src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php
src/applications/herald/controller/HeraldTestConsoleController.php

authored by

Juan Pablo Civile and committed by
epriestley 562da7e9 c35b93d9

+21 -5
+13 -3
src/applications/herald/adapter/HeraldCommitAdapter.php
··· 221 221 $data = $this->commitData; 222 222 $revision_id = $data->getCommitDetail('differential.revisionID'); 223 223 if ($revision_id) { 224 - // TODO: (T603) Herald policy stuff. 225 - $revision = id(new DifferentialRevision())->load($revision_id); 224 + // NOTE: The Herald rule owner might not actually have access to 225 + // the revision, and can control which revision a commit is 226 + // associated with by putting text in the commit message. However, 227 + // the rules they can write against revisions don't actually expose 228 + // anything interesting, so it seems reasonable to load unconditionally 229 + // here. 230 + 231 + $revision = id(new DifferentialRevisionQuery()) 232 + ->withIDs(array($revision_id)) 233 + ->setViewer(PhabricatorUser::getOmnipotentUser()) 234 + ->needRelationships(true) 235 + ->needReviewerStatus(true) 236 + ->executeOne(); 226 237 if ($revision) { 227 - $revision->loadRelationships(); 228 238 $this->affectedRevision = $revision; 229 239 } 230 240 }
+8 -2
src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php
··· 62 62 public static function newLegacyAdapter( 63 63 DifferentialRevision $revision, 64 64 DifferentialDiff $diff) { 65 - 66 65 $object = new HeraldDifferentialRevisionAdapter(); 67 66 68 - $revision->loadRelationships(); 67 + // Reload the revision to pick up relationship information. 68 + $revision = id(new DifferentialRevisionQuery()) 69 + ->withIDs(array($revision->getID())) 70 + ->setViewer(PhabricatorUser::getOmnipotentUser()) 71 + ->needRelationships(true) 72 + ->needReviewerStatus(true) 73 + ->executeOne(); 74 + 69 75 $object->revision = $revision; 70 76 $object->diff = $diff; 71 77