Reduce the amount of weird "static" and "cache" behavior in Pholio query classes

Summary: Depends on D19922. Ref T11351. These query classes have some slightly weird behavior, including `public static function loadImages(...)`. Convert all this stuff into more standard query patterns.

Test Plan: Grepped for callsites, browsed around in Pholio.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T11351

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

src/applications/pholio/engine/PholioMockTimelineEngine.php

@@ -7,10 +7,13 @@ final class PholioMockTimelineEngine
     $viewer = $this->getViewer();
     $object = $this->getObject();
 
-    PholioMockQuery::loadImages(
-      $viewer,
-      array($object),
-      $need_inline_comments = true);
+    $images = id(new PholioImageQuery())
+      ->setViewer($viewer)
+      ->withMocks(array($object))
+      ->needInlineComments(true)
+      ->execute();
+
+    $object->attachImages($images);
 
     return id(new PholioTransactionView())
       ->setMock($object);

src/applications/pholio/query/PholioImageQuery.php

@@ -6,9 +6,9 @@ final class PholioImageQuery
   private $ids;
   private $phids;
   private $mockPHIDs;
+  private $mocks;
 
   private $needInlineComments;
-  private $mockCache = array();
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -20,6 +20,16 @@ final class PholioImageQuery
     return $this;
   }
 
+  public function withMocks(array $mocks) {
+    assert_instances_of($mocks, 'PholioMock');
+
+    $mocks = mpull($mocks, null, 'getPHID');
+    $this->mocks = $mocks;
+    $this->mockPHIDs = array_keys($mocks);
+
+    return $this;
+  }
+
   public function withMockPHIDs(array $mock_phids) {
     $this->mockPHIDs = $mock_phids;
     return $this;
@@ -30,14 +40,6 @@ final class PholioImageQuery
     return $this;
   }
 
-  public function setMockCache($mock_cache) {
-    $this->mockCache = $mock_cache;
-    return $this;
-  }
-  public function getMockCache() {
-    return $this->mockCache;
-  }
-
   public function newResultObject() {
     return new PholioImage();
   }
@@ -76,26 +78,40 @@ final class PholioImageQuery
   protected function willFilterPage(array $images) {
     assert_instances_of($images, 'PholioImage');
 
-    if ($this->getMockCache()) {
-      $mocks = $this->getMockCache();
-    } else {
-      $mock_phids = mpull($images, 'getMockPHID');
+    $mock_phids = array();
+    foreach ($images as $image) {
+      if (!$image->hasMock()) {
+        continue;
+      }
 
-      // DO NOT set needImages to true; recursion results!
-      $mocks = id(new PholioMockQuery())
-        ->setViewer($this->getViewer())
-        ->withPHIDs($mock_phids)
-        ->execute();
-      $mocks = mpull($mocks, null, 'getPHID');
+      $mock_phids[] = $image->getMockPHID();
     }
 
-    foreach ($images as $index => $image) {
-      $mock = idx($mocks, $image->getMockPHID());
-      if ($mock) {
-        $image->attachMock($mock);
+    if ($mock_phids) {
+      if ($this->mocks) {
+        $mocks = $this->mocks;
       } else {
-        // mock is missing or we can't see it
-        unset($images[$index]);
+        $mocks = id(new PholioMockQuery())
+          ->setViewer($this->getViewer())
+          ->withPHIDs($mock_phids)
+          ->execute();
+      }
+
+      $mocks = mpull($mocks, null, 'getPHID');
+
+      foreach ($images as $key => $image) {
+        if (!$image->hasMock()) {
+          continue;
+        }
+
+        $mock = idx($mocks, $image->getMockPHID());
+        if (!$mock) {
+          unset($images[$key]);
+          $this->didRejectResult($image);
+          continue;
+        }
+
+        $image->attachMock($mock);
       }
     }
 

src/applications/pholio/query/PholioMockQuery.php

@@ -58,21 +58,14 @@ final class PholioMockQuery
   }
 
   protected function loadPage() {
-    $mocks = $this->loadStandardPage($this->newResultObject());
-
-    if ($mocks && $this->needImages) {
-      self::loadImages($this->getViewer(), $mocks, $this->needInlineComments);
+    if ($this->needInlineComments && !$this->needImages) {
+      throw new Exception(
+        pht(
+          'You can not query for inline comments without also querying for '.
+          'images.'));
     }
 
-    if ($mocks && $this->needCoverFiles) {
-      $this->loadCoverFiles($mocks);
-    }
-
-    if ($mocks && $this->needTokenCounts) {
-      $this->loadTokenCounts($mocks);
-    }
-
-    return $mocks;
+    return $this->loadStandardPage(new PholioMock());
   }
 
   protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
@@ -109,58 +102,53 @@ final class PholioMockQuery
     return $where;
   }
 
-  public static function loadImages(
-    PhabricatorUser $viewer,
-    array $mocks,
-    $need_inline_comments) {
-    assert_instances_of($mocks, 'PholioMock');
+  protected function didFilterPage(array $mocks) {
+    $viewer = $this->getViewer();
 
-    $mock_map = mpull($mocks, null, 'getPHID');
-    $all_images = id(new PholioImageQuery())
-      ->setViewer($viewer)
-      ->setMockCache($mock_map)
-      ->withMockPHIDs(array_keys($mock_map))
-      ->needInlineComments($need_inline_comments)
-      ->execute();
+    if ($this->needImages) {
+      $images = id(new PholioImageQuery())
+        ->setViewer($viewer)
+        ->withMocks($mocks)
+        ->needInlineComments($this->needInlineComments)
+        ->execute();
 
-    $image_groups = mgroup($all_images, 'getMockPHID');
-
-    foreach ($mocks as $mock) {
-      $mock_images = idx($image_groups, $mock->getPHID(), array());
-      $mock->attachImages($mock_images);
-    }
-  }
-
-  private function loadCoverFiles(array $mocks) {
-    assert_instances_of($mocks, 'PholioMock');
-    $cover_file_phids = mpull($mocks, 'getCoverPHID');
-    $cover_files = id(new PhabricatorFileQuery())
-      ->setViewer($this->getViewer())
-      ->withPHIDs($cover_file_phids)
-      ->execute();
-
-    $cover_files = mpull($cover_files, null, 'getPHID');
-
-    foreach ($mocks as $mock) {
-      $file = idx($cover_files, $mock->getCoverPHID());
-      if (!$file) {
-        $file = PhabricatorFile::loadBuiltin($this->getViewer(), 'missing.png');
+      $image_groups = mgroup($images, 'getMockPHID');
+      foreach ($mocks as $mock) {
+        $images = idx($image_groups, $mock->getPHID(), array());
+        $mock->attachImages($images);
       }
-      $mock->attachCoverFile($file);
     }
-  }
 
-  private function loadTokenCounts(array $mocks) {
-    assert_instances_of($mocks, 'PholioMock');
+    if ($this->needCoverFiles) {
+      $cover_files = id(new PhabricatorFileQuery())
+        ->setViewer($viewer)
+        ->withPHIDs(mpull($mocks, 'getCoverPHID'))
+        ->execute();
+      $cover_files = mpull($cover_files, null, 'getPHID');
 
-    $phids = mpull($mocks, 'getPHID');
-    $counts = id(new PhabricatorTokenCountQuery())
-      ->withObjectPHIDs($phids)
-      ->execute();
-
-    foreach ($mocks as $mock) {
-      $mock->attachTokenCount(idx($counts, $mock->getPHID(), 0));
+      foreach ($mocks as $mock) {
+        $file = idx($cover_files, $mock->getCoverPHID());
+        if (!$file) {
+          $file = PhabricatorFile::loadBuiltin(
+            $viewer,
+            'missing.png');
+        }
+        $mock->attachCoverFile($file);
+      }
     }
+
+    if ($this->needTokenCounts) {
+      $counts = id(new PhabricatorTokenCountQuery())
+        ->withObjectPHIDs(mpull($mocks, 'getPHID'))
+        ->execute();
+
+      foreach ($mocks as $mock) {
+        $token_count = idx($counts, $mock->getPHID(), 0);
+        $mock->attachTokenCount($token_count);
+      }
+    }
+
+    return $mocks;
   }
 
   public function getQueryApplicationClass() {

src/applications/pholio/view/PholioMockImagesView.php

@@ -110,7 +110,7 @@ final class PholioMockImagesView extends AphrontView {
       );
     }
 
-    $ids = mpull($mock->getActiveImages(), 'getID');
+    $ids = mpull($mock->getActiveImages(), null, 'getID');
     if ($this->imageID && isset($ids[$this->imageID])) {
       $selected_id = $this->imageID;
     } else {