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;