Skip to content

Commit dd206a5

Browse files
author
epriestley
committed
Viewerize ArcBundle file loading callbacks
Summary: Ref T603. Clean these up and move them to a single place. Test Plan: - Downloaded a raw diff. - Enabled "attach diffs", created a revision, got an email with a diff. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T603 Differential Revision: https://secure.phabricator.com/D7179
1 parent 13dae05 commit dd206a5

File tree

4 files changed

+43
-25
lines changed

4 files changed

+43
-25
lines changed

src/__phutil_library_map__.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1185,6 +1185,7 @@
11851185
'PhabricatorFeedStoryStatus' => 'applications/feed/story/PhabricatorFeedStoryStatus.php',
11861186
'PhabricatorFeedStoryTypeConstants' => 'applications/feed/constants/PhabricatorFeedStoryTypeConstants.php',
11871187
'PhabricatorFile' => 'applications/files/storage/PhabricatorFile.php',
1188+
'PhabricatorFileBundleLoader' => 'applications/files/query/PhabricatorFileBundleLoader.php',
11881189
'PhabricatorFileCommentController' => 'applications/files/controller/PhabricatorFileCommentController.php',
11891190
'PhabricatorFileController' => 'applications/files/controller/PhabricatorFileController.php',
11901191
'PhabricatorFileDAO' => 'applications/files/storage/PhabricatorFileDAO.php',

src/applications/differential/controller/DifferentialRevisionViewController.php

Lines changed: 7 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -875,27 +875,6 @@ private function renderOtherRevisions(array $revisions) {
875875
->appendChild($view);
876876
}
877877

878-
/**
879-
* Straight copy of the loadFileByPhid method in
880-
* @{class:DifferentialReviewRequestMail}.
881-
*
882-
* This is because of the code similarity between the buildPatch method in
883-
* @{class:DifferentialReviewRequestMail} and @{method:buildRawDiffResponse}
884-
* in this class. Both of these methods end up using call_user_func and this
885-
* piece of code is the lucky function.
886-
*
887-
* @return mixed (@{class:PhabricatorFile} if found, null if not)
888-
*/
889-
public function loadFileByPHID($phid) {
890-
// TODO: (T603) Factor this and the other one out.
891-
$file = id(new PhabricatorFile())->loadOneWhere(
892-
'phid = %s',
893-
$phid);
894-
if (!$file) {
895-
return null;
896-
}
897-
return $file->loadFileData();
898-
}
899878

900879
/**
901880
* Note this code is somewhat similar to the buildPatch method in
@@ -912,6 +891,8 @@ private function buildRawDiffResponse(
912891
assert_instances_of($changesets, 'DifferentialChangeset');
913892
assert_instances_of($vs_changesets, 'DifferentialChangeset');
914893

894+
$viewer = $this->getRequest()->getUser();
895+
915896
$engine = new PhabricatorDifferenceEngine();
916897
$generated_changesets = array();
917898
foreach ($changesets as $changeset) {
@@ -954,9 +935,12 @@ private function buildRawDiffResponse(
954935
foreach ($raw_changes as $changedict) {
955936
$changes[] = ArcanistDiffChange::newFromDictionary($changedict);
956937
}
957-
$bundle = ArcanistBundle::newFromChanges($changes);
958938

959-
$bundle->setLoadFileDataCallback(array($this, 'loadFileByPHID'));
939+
$loader = id(new PhabricatorFileBundleLoader())
940+
->setViewer($viewer);
941+
942+
$bundle = ArcanistBundle::newFromChanges($changes);
943+
$bundle->setLoadFileDataCallback(array($loader, 'loadFileData'));
960944

961945
$vcs = $repository ? $repository->getVersionControlSystem() : null;
962946
switch ($vcs) {

src/applications/differential/mail/DifferentialReviewRequestMail.php

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,9 +127,15 @@ private function buildPatch() {
127127
foreach ($raw_changes as $changedict) {
128128
$changes[] = ArcanistDiffChange::newFromDictionary($changedict);
129129
}
130-
$bundle = ArcanistBundle::newFromChanges($changes);
131130

132-
$bundle->setLoadFileDataCallback(array($this, 'loadFileByPHID'));
131+
// TODO: It would be nice to have a real viewer here eventually, but
132+
// in the meantime anyone we're sending mail to can certainly see the
133+
// patch.
134+
$loader = id(new PhabricatorFileBundleLoader())
135+
->setViewer(PhabricatorUser::getOmnipotentUser());
136+
137+
$bundle = ArcanistBundle::newFromChanges($changes);
138+
$bundle->setLoadFileDataCallback(array($loader, 'loadFileData'));
133139

134140
$format = PhabricatorEnv::getEnvConfig('metamta.differential.patch-format');
135141
switch ($format) {
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<?php
2+
3+
/**
4+
* Callback provider for loading @{class@arcanist:ArcanistBundle} file data
5+
* stored in the Files application.
6+
*/
7+
final class PhabricatorFileBundleLoader {
8+
9+
private $viewer;
10+
11+
public function setViewer(PhabricatorUser $viewer) {
12+
$this->viewer = $viewer;
13+
return $this;
14+
}
15+
16+
public function loadFileData($phid) {
17+
$file = id(new PhabricatorFileQuery())
18+
->setViewer($this->viewer)
19+
->withPHIDs(array($phid))
20+
->executeOne();
21+
if (!$file) {
22+
return null;
23+
}
24+
return $file->loadFileData();
25+
}
26+
27+
}

0 commit comments

Comments
 (0)