Make Images in Pholio refer to mocks by PHID instead of ID

Summary:
Ref T11351. In Pholio, we currently use a `mockID`, but a `mockPHID` is generally preferable / more modern / more flexible. In particular, we need PHIDs to load handles and prefer PHIDs when exposing information to the API, and using PHIDs internally makes a bunch of things easier/better/faster and ~nothing harder/worse/slower.

I'll add some inlines about a few things.

Test Plan: Ran migrations, spot-checked database for sanity. Loaded Pholio, saw data unchanged. Created and edited images.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T11351

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

Author: epriestley

resources/sql/autopatches/20181219.pholio.01.imagephid.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_pholio.pholio_image
+  ADD mockPHID VARBINARY(64);

resources/sql/autopatches/20181219.pholio.02.imagemigrate.php

@@ -0,0 +1,35 @@
+<?php
+
+// Old images used a "mockID" instead of a "mockPHID" to reference mocks.
+// Set the "mockPHID" column to the value that corresponds to the "mockID".
+
+$image = new PholioImage();
+$mock = new PholioMock();
+
+$conn = $image->establishConnection('w');
+$iterator = new LiskRawMigrationIterator($conn, $image->getTableName());
+
+foreach ($iterator as $image_row) {
+  if ($image_row['mockPHID']) {
+    continue;
+  }
+
+  $mock_id = $image_row['mockID'];
+
+  $mock_row = queryfx_one(
+    $conn,
+    'SELECT phid FROM %R WHERE id = %d',
+    $mock,
+    $mock_id);
+
+  if (!$mock_row) {
+    continue;
+  }
+
+  queryfx(
+    $conn,
+    'UPDATE %R SET mockPHID = %s WHERE id = %d',
+    $image,
+    $mock_row['phid'],
+    $image_row['id']);
+}

resources/sql/autopatches/20181219.pholio.03.imageid.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_pholio.pholio_image
+  DROP mockID;

src/applications/pholio/editor/PholioMockEditor.php

@@ -91,7 +91,7 @@ protected function applyFinalEffects(
 
     $images = $this->getNewImages();
     foreach ($images as $image) {
-      $image->setMockID($object->getID());
+      $image->setMockPHID($object->getPHID());
       $image->save();
     }
 

src/applications/pholio/lipsum/PhabricatorPholioMockTestDataGenerator.php

@@ -65,7 +65,7 @@ public function generateObject() {
       ->setActor($author)
       ->applyTransactions($mock, $transactions);
     foreach ($images as $image) {
-      $image->setMockID($mock->getID());
+      $image->setMockPHID($mock->getPHID());
       $image->save();
     }
 

src/applications/pholio/phid/PholioImagePHIDType.php

@@ -32,13 +32,9 @@ public function loadHandles(
     foreach ($handles as $phid => $handle) {
       $image = $objects[$phid];
 
-      $id = $image->getID();
-      $mock_id = $image->getMockID();
-      $name = $image->getName();
-
-      $handle->setURI("/M{$mock_id}/{$id}/");
-      $handle->setName($name);
-      $handle->setFullName($name);
+      $handle
+        ->setName($image->getName())
+        ->setURI($image->getURI());
     }
   }
 

src/applications/pholio/query/PholioImageQuery.php

@@ -5,8 +5,7 @@ final class PholioImageQuery
 
   private $ids;
   private $phids;
-  private $mockIDs;
-  private $obsolete;
+  private $mockPHIDs;
 
   private $needInlineComments;
   private $mockCache = array();
@@ -21,13 +20,8 @@ public function withPHIDs(array $phids) {
     return $this;
   }
 
-  public function withMockIDs(array $mock_ids) {
-    $this->mockIDs = $mock_ids;
-    return $this;
-  }
-
-  public function withObsolete($obsolete) {
-    $this->obsolete = $obsolete;
+  public function withMockPHIDs(array $mock_phids) {
+    $this->mockPHIDs = $mock_phids;
     return $this;
   }
 
@@ -69,18 +63,11 @@ protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
         $this->phids);
     }
 
-    if ($this->mockIDs !== null) {
+    if ($this->mockPHIDs !== null) {
       $where[] = qsprintf(
         $conn,
-        'mockID IN (%Ld)',
-        $this->mockIDs);
-    }
-
-    if ($this->obsolete !== null) {
-      $where[] = qsprintf(
-        $conn,
-        'isObsolete = %d',
-        $this->obsolete);
+        'mockPHID IN (%Ls)',
+        $this->mockPHIDs);
     }
 
     return $where;
@@ -92,16 +79,18 @@ protected function willFilterPage(array $images) {
     if ($this->getMockCache()) {
       $mocks = $this->getMockCache();
     } else {
-      $mock_ids = mpull($images, 'getMockID');
+      $mock_phids = mpull($images, 'getMockPHID');
+
       // DO NOT set needImages to true; recursion results!
       $mocks = id(new PholioMockQuery())
         ->setViewer($this->getViewer())
-        ->withIDs($mock_ids)
+        ->withPHIDs($mock_phids)
         ->execute();
-      $mocks = mpull($mocks, null, 'getID');
+      $mocks = mpull($mocks, null, 'getPHID');
     }
+
     foreach ($images as $index => $image) {
-      $mock = idx($mocks, $image->getMockID());
+      $mock = idx($mocks, $image->getMockPHID());
       if ($mock) {
         $image->attachMock($mock);
       } else {

src/applications/pholio/query/PholioMockQuery.php

@@ -115,18 +115,18 @@ public static function loadImages(
     $need_inline_comments) {
     assert_instances_of($mocks, 'PholioMock');
 
-    $mock_map = mpull($mocks, null, 'getID');
+    $mock_map = mpull($mocks, null, 'getPHID');
     $all_images = id(new PholioImageQuery())
       ->setViewer($viewer)
       ->setMockCache($mock_map)
-      ->withMockIDs(array_keys($mock_map))
+      ->withMockPHIDs(array_keys($mock_map))
       ->needInlineComments($need_inline_comments)
       ->execute();
 
-    $image_groups = mgroup($all_images, 'getMockID');
+    $image_groups = mgroup($all_images, 'getMockPHID');
 
     foreach ($mocks as $mock) {
-      $mock_images = idx($image_groups, $mock->getID(), array());
+      $mock_images = idx($image_groups, $mock->getPHID(), array());
       $mock->attachAllImages($mock_images);
       $active_images = mfilter($mock_images, 'getIsObsolete', true);
       $mock->attachImages(msort($active_images, 'getSequence'));

src/applications/pholio/storage/PholioImage.php

@@ -6,7 +6,7 @@ final class PholioImage extends PholioDAO
     PhabricatorExtendedPolicyInterface {
 
   protected $authorPHID;
-  protected $mockID;
+  protected $mockPHID;
   protected $filePHID;
   protected $name;
   protected $description;
@@ -29,22 +29,17 @@ protected function getConfiguration() {
     return array(
       self::CONFIG_AUX_PHID => true,
       self::CONFIG_COLUMN_SCHEMA => array(
-        'mockID' => 'id?',
+        'mockPHID' => 'phid?',
         'name' => 'text128',
         'description' => 'text',
         'sequence' => 'uint32',
         'isObsolete' => 'bool',
         'replacesImagePHID' => 'phid?',
       ),
       self::CONFIG_KEY_SCHEMA => array(
-        'key_phid' => null,
-        'keyPHID' => array(
-          'columns' => array('phid'),
-          'unique' => true,
-        ),
-        'mockID' => array(
-          'columns' => array('mockID', 'isObsolete', 'sequence'),
-        ),
+        // TODO: There should be a key starting with "mockPHID" here at a
+        // minimum, but it's not entirely clear what other columns we should
+        // have as part of the key.
       ),
     ) + parent::getConfiguration();
   }
@@ -71,6 +66,10 @@ public function getMock() {
     return $this->assertAttached($this->mock);
   }
 
+  public function hasMock() {
+    return (bool)$this->getMockPHID();
+  }
+
   public function attachInlineComments(array $inline_comments) {
     assert_instances_of($inline_comments, 'PholioTransactionComment');
     $this->inlineComments = $inline_comments;
@@ -82,6 +81,22 @@ public function getInlineComments() {
     return $this->inlineComments;
   }
 
+  public function getURI() {
+    if ($this->hasMock()) {
+      $mock = $this->getMock();
+
+      $mock_uri = $mock->getURI();
+      $image_id = $this->getID();
+
+      return "{$mock_uri}/{$image_id}/";
+    }
+
+    // For now, standalone images have no URI. We could provide one at some
+    // point, although it's not clear that there's any motivation to do so.
+
+    return null;
+  }
+
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 
@@ -96,7 +111,7 @@ public function getCapabilities() {
   public function getPolicy($capability) {
     // If the image is attached to a mock, we use an extended policy to match
     // the mock's permissions.
-    if ($this->getMockID()) {
+    if ($this->hasMock()) {
       return PhabricatorPolicies::getMostOpenPolicy();
     }
 
@@ -113,7 +128,7 @@ public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
 
 
   public function getExtendedPolicy($capability, PhabricatorUser $viewer) {
-    if ($this->getMockID()) {
+    if ($this->hasMock()) {
       return array(
         array(
           $this->getMock(),

src/applications/pholio/storage/PholioMock.php

@@ -259,7 +259,7 @@ public function destroyObjectPermanently(
     $this->openTransaction();
       $images = id(new PholioImageQuery())
         ->setViewer($engine->getViewer())
-        ->withMockIDs(array($this->getID()))
+        ->withMockIDs(array($this->getPHID()))
         ->execute();
       foreach ($images as $image) {
         $image->delete();