Duplicated summary item when linking to content with the MediaThumbnailFormatter

This issue is still in Drupal 9.4.x.

I'm changing this to "Normal" because in my experience, site builders are already confused when configuring Media reference fields to display. This bug only makes it more confusing.

The posted patch looks reasonable, but we'll need tests too.

Here's a fail test. There doesn't appear to be a ton of test coverage for stuff like this. I was hoping to just add a little to an existing test in media, but I didn't find any obvious candidates. In this new test class, I closely imitated ResponsiveImageFieldUiTest from the responsive_image module.

I'm also attaching a patch essentially adds the fix from #2. I've added a comment and made some stylistic changes, but it's essentially the same fix.

Arg, I missed changing the @group annotation when I copied over the test from responsive_image. That didn't cause any problems running the new test locally, but the bot doesn't like that one bit!

Here are those same patches but with the @group annotation on the new test fixed.

Well, the namespace is wrong. Sorry. Trying again.

Update: I got the namespace wrong because I was copying off of a strange test. I put in an issue here: #3248816: ResponsiveImageFieldUiTest.php should be moved to tests directory

Finally got it!

I've also updated the IS to include the original screenshot from @mottihoresh.

Applied patch #12 and the issue is fixed.

Before Patch:

After Patch:

RTBC +1

I just turned this into an MR because the media folks tend to prefer that. I applied the patch from #12 and broken it into two commits to start the MR.

+++ b/core/modules/media/src/Plugin/Field/FieldFormatter/MediaThumbnailFormatter.php
@@ -113,14 +113,11 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
+    if ($this->getSetting('image_link') == 'media') {
+      $summary[] = $this->t('Linked to media item');

please replace == with ===

Thanks, @andypost. I updated the MR.

I'm so confused about what branch things are supposed to target right now. Should I update the MR to target 9.3.x?

I'm picking this one back up with a different approach to testing. Instead of that bulky JS test, here's a Kernel test. This has the same fix as was in the MR.

That extra failure in each patch above is a known "random" fail: #3315753: Random fail in Drupal\Tests\media_library\FunctionalJavascript\WidgetUploadTest

The real failure to note in the fail patch looks perfect:

1) Drupal\Tests\media\Kernel\MediaThumbnailFormatterTest::testSettingsSummary with data set "content" (array('content'), array('Original image', 'Linked to content', 'Image loading: lazy'))
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
     0 => 'Original image'
     1 => 'Linked to content'
     2 => 'Image loading: lazy'
+    3 => 'Linked to content'
 )

This failure is exactly what we see in the IS and it is fixed by 3078030-26.patch.

Setting back to NR since this only got bumped due to the random fails.

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200, following Review a patch or merge require as a guide.

Tried replicating this in 10.1 but did not see the issue.

Moving to NW if the steps to reproduce need to be updated.

This is still an issue in 10.1. The steps to reproduce in the IS look correct to me. The tests indicate the issue still exists as well. The key is selecting "Link image to content" in the field ui.

I'll throw this back to NR.

Thanks for the quick reply.

I'll retest this.

Running the last patch against 10.1 one more time to see if that FunctionJavascript test passes.

Good idea, @smustgrave. I just re-queued the fail test in #26 as well, just to make sure it fails on the very latest.

Fail test in #26 still failing correctly on 10.1.

I think this looks like a really straightforward fix, and the test coverage is solid.

1. +++ b/core/modules/media/tests/src/Kernel/MediaThumbnailFormatterTest.php
   @@ -0,0 +1,78 @@
   +   * @dataProvider providerTestSettingsSummary()
   

   Nit: I don't know if data providers are normally supposed to have the () after the function name...? But hey, if it works, it's not a big deal.

2. +++ b/core/modules/media/tests/src/Kernel/MediaThumbnailFormatterTest.php
   @@ -0,0 +1,78 @@
   +  public function testSettingsSummary($settings, $expected): void {
   

   These parameters should be type hinted as arrays, and documented in the method doc comment.

3. +++ b/core/modules/media/tests/src/Kernel/MediaThumbnailFormatterTest.php
   @@ -0,0 +1,78 @@
   +    /** @var \Drupal\Core\Field\FormatterPluginManager $formatter_manager  */
   +    $formatter_manager = \Drupal::service('plugin.manager.field.formatter');
   +    $formatter = $formatter_manager->createInstance('media_thumbnail', $configuration);
   

   I feel like this is a bit lower-level than we should do.

   I'd suggest we use the entity display system instead, which would allow us to respect the underlying abstractions:

   $display = \Drupal::service('entity_display.repository')->getViewDisplay('media', $this->testMediaType->id());
   
   $display->setComponent($source_field_name, ['type' => 'media_thumbnail', 'settings' => $settings]);
   $formatter = $display->getRenderer($source_field_name);
   

4. +++ b/core/modules/media/tests/src/Kernel/MediaThumbnailFormatterTest.php
   @@ -0,0 +1,78 @@
   +    $summary = $formatter->settingsSummary();
   +    // The summary is an array of TranslatableText objects. Iterate over it
   +    // to get the strings.
   +    $summary_strings = [];
   +    foreach ($summary as $value) {
   +      $summary_strings[] = $value->__toString();
   +    }
   +    $this->assertSame($expected, $summary_strings);
   

   This is a bit verbose, and we probably shouldn't call __toString() directly.

   How about this instead?

   $actual_summary = array_map('strval', $formatter->settingsSummary());
   $this->assertSame($expected, $actual_summary);
   

   Also, for clarity's sake, maybe $expected should be renamed to $expected_summary.

5. +++ b/core/modules/media/tests/src/Kernel/MediaThumbnailFormatterTest.php
   @@ -0,0 +1,78 @@
   +      'content' => [
   +        [
   +          'image_link' => 'content',
   +        ],
   +        [
   +          'Original image',
   +          'Linked to content',
   +          'Image loading: lazy',
   +        ],
   +      ],
   +      'media' => [
   +        [
   +          'image_link' => 'media',
   +        ],
   +        [
   +          'Original image',
   +          'Image loading: lazy',
   +          'Linked to media item',
   +        ],
   +      ],
   +      'nothing' => [
   +        [
   +          'image_link' => '',
   +        ],
   +        [
   +          'Original image',
   +          'Image loading: lazy',
   +        ],
   +      ],
   +    ];
   

   The keys 'content', 'media', and 'nothing' aren't the most descriptive. Could these be "link to content", "link to media", etc.?

Thank you for all the suggestions in #35, @phenaproxima. I followed the spirit of 35.3 but I had to create a new entity bundle with a media reference field in order to use the view display API. Since media_thumbnail can only be used on media reference fields, what I was doing didn't make sense anyway!

Since there are so many changes in the test, I attaching a new test-only (aka FAIL) patch.

That was painful. These should be better.

Cleaning up some of the files some

Reviewing #37

FAIL.patch shows the issue
Confirmed it manually also following the steps in the issue summary
Applying the fix did address the issue and the comments in the patch help explain why.

Random Migrate failure again.

Those look like unrelated "random" fails in #42. Flipping back to RTBC.

Unrelated failure in Javascript.Drupal\FunctionalJavascriptTests\Tests\JSWebAssertTest

Back to RTBC

Adding issue credits

Committed to 10.1.x and backported to 10.0.x and 9.5.x

Thanks folks