[PATCH] PholioMockEmbedView image specification broken

When specifying an image of a mock set (as described in the Remarkup Reference), always the first image of the set is shown. So {M3}, {M3, image=1}, and {M3, image=5} all yield the same result.

Reproduction Instructions

  1. Create a Mock, upload multiple images, save.
  2. Create a new Phriction Wiki page, try embedding the mock you just created using {Mx, image=y} syntax. Perceive how always the first image of the set is shown, regardless which y is chosen.

Phabricator/Arcanist Version

Phabricator as of today (master branch, b2ab18f).

The patch below fixes the problem by adding the missing call to mpull(…, ‘getID’). It also corrects remarkup documentation, so that specifying multiple images using the “12 & 13” syntax isn’t covered anymore (as support for showing multiple images in a carrousel was removed in this commit a couple of years ago). It keeps support for parsing the syntax for backwards compatibility and adds a comment to the code why it’s still in there.


diff --git a/src/applications/pholio/remarkup/PholioRemarkupRule.php b/src/applications/pholio/remarkup/PholioRemarkupRule.php
index 00025b6326..090022c376 100644
--- a/src/applications/pholio/remarkup/PholioRemarkupRule.php
+++ b/src/applications/pholio/remarkup/PholioRemarkupRule.php
@@ -75,6 +75,8 @@ final class PholioRemarkupRule extends PhabricatorObjectRemarkupRule {
       $opts = $parser->parse(substr($options, 1));
       if (isset($opts['image'])) {
+        // PholioMockEmbedView shows only the first image passed.
+        // Keep "&" syntax for backwards compatibility.
         $images = array_unique(
           explode('&', preg_replace('/\s+/', '', $opts['image'])));
diff --git a/src/applications/pholio/view/PholioMockEmbedView.php b/src/applications/pholio/view/PholioMockEmbedView.php
index 38b375b68b..2d5c289c73 100644
--- a/src/applications/pholio/view/PholioMockEmbedView.php
+++ b/src/applications/pholio/view/PholioMockEmbedView.php
@@ -25,7 +25,8 @@ final class PholioMockEmbedView extends AphrontView {
     $thumbnail = null;
     if (!empty($this->images)) {
       $images_to_show = array_intersect_key(
-        $this->mock->getActiveImages(), array_flip($this->images));
+        mpull($mock->getActiveImages(), null, 'getID'),
+        array_flip($this->images));
     $xform = PhabricatorFileTransform::getTransformByKey(
diff --git a/src/docs/user/userguide/remarkup.diviner b/src/docs/user/userguide/remarkup.diviner
index f96c8c7b25..b8bbe1cb8f 100644
--- a/src/docs/user/userguide/remarkup.diviner
+++ b/src/docs/user/userguide/remarkup.diviner
@@ -393,14 +393,13 @@ You can embed a Pholio mock by using braces to refer to it:
-By default the first four images from the mock set are displayed. This behavior
+By default the first image from the mock is are displayed. This behavior
 can be overridden with the **image** option. With the **image** option you can
-provide one or more image IDs to display.
+provide an image ID to display.
-You can set the image (or images) to display like this:
+You can set the image to display like this:
   {M123, image=12345}
-  {M123, image=12345 & 6789}
 == Embedding Pastes