Page MenuHomePhabricator
Files F15199422

D19223.diff
No OneTemporary
Actions

D19223.diff
View Options

diff --git a/src/applications/files/query/PhabricatorFileQuery.php b/src/applications/files/query/PhabricatorFileQuery.php
--- a/src/applications/files/query/PhabricatorFileQuery.php
+++ b/src/applications/files/query/PhabricatorFileQuery.php
@@ -152,46 +152,74 @@
}
protected function loadPage() {
- $files = $this->loadStandardPage(new PhabricatorFile());
+ $files = $this->loadStandardPage($this->newResultObject());
if (!$files) {
return $files;
}
- $viewer = $this->getViewer();
- $is_omnipotent = $viewer->isOmnipotent();
-
- // We need to load attached objects to perform policy checks for files.
- // First, load the edges.
+ // Figure out which files we need to load attached objects for. In most
+ // cases, we need to load attached objects to perform policy checks for
+ // files.
- $edge_type = PhabricatorFileHasObjectEdgeType::EDGECONST;
- $file_phids = mpull($files, 'getPHID');
- $edges = id(new PhabricatorEdgeQuery())
- ->withSourcePHIDs($file_phids)
- ->withEdgeTypes(array($edge_type))
- ->execute();
-
- $object_phids = array();
+ // However, in some special cases where we know files will always be
+ // visible, we skip this. See T8478 and T13106.
+ $need_objects = array();
foreach ($files as $file) {
- $phids = array_keys($edges[$file->getPHID()][$edge_type]);
- $file->attachObjectPHIDs($phids);
+ $always_visible = false;
if ($file->getIsProfileImage()) {
- // If this is a profile image, don't bother loading related files.
- // It will always be visible, and we can get into trouble if we try
- // to load objects and end up stuck in a cycle. See T8478.
- continue;
+ $always_visible = true;
}
- if ($is_omnipotent) {
- // If the viewer is omnipotent, we don't need to load the associated
- // objects either since they can certainly see the object. Skipping
- // this can improve performance and prevent cycles.
+ if ($file->isBuiltin()) {
+ $always_visible = true;
+ }
+
+ if ($always_visible) {
+ // We just treat these files as though they aren't attached to
+ // anything. This saves a query in common cases when we're loading
+ // profile images or builtins. We could be slightly more nuanced
+ // about this and distinguish between "not attached to anything" and
+ // "might be attached but policy checks don't need to care".
+ $file->attachObjectPHIDs(array());
continue;
}
- foreach ($phids as $phid) {
- $object_phids[$phid] = true;
+ $need_objects[] = $file;
+ }
+
+ $viewer = $this->getViewer();
+ $is_omnipotent = $viewer->isOmnipotent();
+
+ // If we have any files left which do need objects, load the edges now.
+ $object_phids = array();
+ if ($need_objects) {
+ $edge_type = PhabricatorFileHasObjectEdgeType::EDGECONST;
+ $file_phids = mpull($need_objects, 'getPHID');
+
+ $edges = id(new PhabricatorEdgeQuery())
+ ->withSourcePHIDs($file_phids)
+ ->withEdgeTypes(array($edge_type))
+ ->execute();
+
+ foreach ($need_objects as $file) {
+ $phids = array_keys($edges[$file->getPHID()][$edge_type]);
+ $file->attachObjectPHIDs($phids);
+
+ if ($is_omnipotent) {
+ // If the viewer is omnipotent, we don't need to load the associated
+ // objects either since the viewer can certainly see the object.
+ // Skipping this can improve performance and prevent cycles. This
+ // could possibly become part of the profile/builtin code above which
+ // short circuits attacment policy checks in cases where we know them
+ // to be unnecessary.
+ continue;
+ }
+
+ foreach ($phids as $phid) {
+ $object_phids[$phid] = true;
+ }
}
}
@@ -203,7 +231,7 @@
$xforms = id(new PhabricatorTransformedFile())->loadAllWhere(
'transformedPHID IN (%Ls)',
- $file_phids);
+ mpull($files, 'getPHID'));
$xform_phids = mpull($xforms, 'getOriginalPHID', 'getTransformedPHID');
foreach ($xform_phids as $derived_phid => $original_phid) {
$object_phids[$original_phid] = true;

File Metadata

Mime Type
text/plain
Expires
Mon, Feb 24, 8:38 AM (13 h, 54 m)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7189356
Default Alt Text
D19223.diff (4 KB)

Event Timeline

Phabricator · Built by Phacility