Ensure that embedded image media items do not display at unreasonable sizes by default

Nice work! This patch is looking great! It does need some changes (and some questions to be answered by committer-type folk), but overall this is a solid approach.

1. +++ b/core/modules/media/media.module
   @@ -496,3 +499,73 @@ function media_filter_format_edit_form_validate($form, FormStateInterface $form_
   + * @throws \Drupal\Core\Entity\EntityStorageException
   

   We don't need the @throws, because this function never actually throws anything.

2. +++ b/core/modules/media/media.module
   @@ -496,3 +499,73 @@ function media_filter_format_edit_form_validate($form, FormStateInterface $form_
   +  $values = [
   +    'targetEntityType' => 'media',
   +    'bundle' => $type->id(),
   +    'mode' => 'embed',
   +    'status' => TRUE,
   +  ];
   +  $display = EntityViewDisplay::create($values);
   

   Nit: $values does not need to be its own variable.

3. +++ b/core/modules/media/media.module
   @@ -496,3 +499,73 @@ function media_filter_format_edit_form_validate($form, FormStateInterface $form_
   +  foreach (array_keys($display->getComponents()) as $name) {
   +    $display->removeComponent($name);
   +  }
   

   A quicker way to do this without looping: $display->set('content', []).

4. +++ b/core/modules/media/media.module
   @@ -496,3 +499,73 @@ function media_filter_format_edit_form_validate($form, FormStateInterface $form_
   +  // Expose the thumbnail component with the default image style included with
   +  // the media module.
   +  $display->setComponent('thumbnail', [
   +    'type' => 'image',
   +    'label' => 'hidden',
   +    'settings' => [
   +      'image_style' => 'media_embed',
   +      'image_link' => '',
   +    ],
   +  ]);
   

   So...I'm not sure why we would want to expose the thumbnail in an embedded view mode. For a YouTube video, for example, we'd want to show the video itself (the iframe). What we should do here instead is take advantage of MediaSourceInterface::prepareViewDisplay().

   It makes sense for Media Library to configure the thumbnail, since that's what's normally displayed in the media library. But for an embed, we should use the defaults provided by prepareViewDisplay().

5. +++ b/core/modules/media/media.module
   @@ -496,3 +499,73 @@ function media_filter_format_edit_form_validate($form, FormStateInterface $form_
   +  return (bool) $display->save();
   

   I know this was copied and pasted from Media Library, so this is not your fault at all. But looking at it with fresh eyes, this line makes no actual sense :)

   If $display->save() fails, it throws an exception. If it succeeds, it returns SAVED_NEW or SAVED_UDPATED, both of which cast to boolean TRUE. So in other words, this function will always return TRUE. Let's just remove this line here, and open a follow-up to fix it in Media Library.

6. +++ b/core/modules/media/media.module
   @@ -496,3 +499,73 @@ function media_filter_format_edit_form_validate($form, FormStateInterface $form_
   +/**
   + * Implements hook_form_alter().
   + */
   +function media_form_alter(array &$form, FormStateInterface $form_state, $form_id) {
   

   I was going to suggest using hook_form_BASE_FORM_ID_alter() instead, but I see this is following what Media Library already does.

7. +++ b/core/modules/media/media.module
   @@ -496,3 +499,73 @@ function media_filter_format_edit_form_validate($form, FormStateInterface $form_
   +    if (_media_configure_embed_view_display($type)) {
   +      \Drupal::messenger()->addStatus(t('Embed view display has been created for the %type media type.', [
   +        '%type' => $type->label(),
   +      ]));
   +    }
   

   Since the return value of _media_configure_embed_view_display() is useless, I would instead suggest this:

   try {
     _media_configure_embed_view_display($type);
    \Drupal::messenger()->addStatus(...);
   }
   catch (EntityStorageException $e) {
     \Drupal::messenger()->addError($e->getMessage());
     return;
   }
   

   Or, if we want to be harsher, we can just call the function and not bother catching the exception.

8. +++ b/core/modules/media/media.post_update.php
   @@ -5,6 +5,80 @@
   +      'width' => 720,
   +      'height' => 405,
   

   How did we arrive at these dimensions?

9. +++ b/core/modules/media/media.post_update.php
   @@ -5,6 +5,80 @@
   +  return t('The %label image style has been created successfully.', ['%label' => 'Media Embed']);
   

   Why not use ['%label' => $image_style->label()]?

10. +++ b/core/modules/media/media.post_update.php
    @@ -5,6 +5,80 @@
    +  $values = [
    +    'id' => 'media.embed',
    +    'targetEntityType' => 'media',
    +    'label' => t('Embed'),
    +    'dependencies' => [
    +      'enforced' => [
    +        'module' => [
    +          'media',
    +        ],
    +      ],
    +      'module' => [
    +        'media',
    +      ],
    +    ],
    +  ];
    +  if (!EntityViewMode::load('media.embed')) {
    +    EntityViewMode::create($values)->save();
    +  }
    

    I think I'd like a release manager to validate this. Also, nit: $values doesn't need to be its own variable.

11. +++ b/core/modules/media/media.post_update.php
    @@ -5,6 +5,80 @@
    +  // Embeds needs a special view display to make sure the out-of-the-box
    +  // display doesn't use very large images. It was not automatically
    +  // created for custom media types, so let's make sure this is fixed.
    +  $types = [];
    +  foreach (MediaType::loadMultiple() as $type) {
    +    $view_display_created = _media_configure_embed_view_display($type);
    +    if ($view_display_created) {
    +      $types[] = $type->label();
    +    }
    +  }
    +  if ($types) {
    +    return t('The Embed view display has been created for the following media types: @types.', [
    +      '@types' => implode(', ', $types),
    +    ]);
    +  }
    

    So, here's the thing -- most media types don't necessarily need a specialized embed display. The only ones which really do, at least for now, are the ones that use images as their source field. I'm wondering if, to simplify things, we should create the view displays selectively -- i.e., only do this for image media types. I'd like to hear a product manager's opinion on this idea before we proceed, though.

12. +++ b/core/modules/media/tests/src/Functional/MediaEmbedDisplayModeTest.php
    @@ -0,0 +1,139 @@
    +  protected static $modules = [
    +    'field_ui',
    +    'media',
    +    'system',
    +  ];
    

    system is installed anyway; I don't think it needs to be explicitly listed in $modules.

Thanks for the updates! This is shaping up.

1. +++ b/core/modules/media/config/install/core.entity_view_mode.media.embed.yml
   @@ -0,0 +1,12 @@
   +langcode: en
   +status: true
   +dependencies:
   +  enforced:
   +    module:
   +      - media
   +  module:
   +    - media
   +id: media.embed
   +label: 'Embed'
   +targetEntityType: media
   +cache: true
   

   So this has cache: true (which is the default value)...yet we explicitly disable caching when rendering embedded media, instead relying on the caching of the host entity. I wonder if we should have cache: false in this view mode, for this reason. I'd like Wim to confirm or deny this idea.

2. +++ b/core/modules/media/media.module
   @@ -496,3 +501,76 @@ function media_filter_format_edit_form_validate($form, FormStateInterface $form_
   + * @return bool
   + *   Returns true if the EntityViewDisplay display was configured.
   

   This return description should be rephrased. How about: "Returns TRUE if the entity view display was configured correctly, FALSE otherwise."

3. +++ b/core/modules/media/media.module
   @@ -496,3 +501,76 @@ function media_filter_format_edit_form_validate($form, FormStateInterface $form_
   +  // If a site builder has deleted the 'Embed' view mode, do not
   +  // configure the display.
   +  $view_mode = EntityViewMode::load('media.embed');
   +  if (!$view_mode) {
   +    return FALSE;
   +  }
   

   Nice. This is a good use of the boolean return! :) I agree with your justification.

4. +++ b/core/modules/media/media.module
   @@ -496,3 +501,76 @@ function media_filter_format_edit_form_validate($form, FormStateInterface $form_
   +  $display = EntityViewDisplay::load('media.' . $type->id() . '.embed');
   +
   +  if ($display) {
   +    return FALSE;
   +  }
   

   A comment over this bit would be useful. "If the embedded entity view display has already been configured, we don't need to redo it."

5. +++ b/core/modules/media/media.module
   @@ -496,3 +501,76 @@ function media_filter_format_edit_form_validate($form, FormStateInterface $form_
   +  foreach ($display->getComponents() as $name => $config) {
   +    if (!empty($config['type']) && $config['type'] == 'image' && $config['settings']['image_style'] === '') {
   +      $config['settings']['image_style'] = 'media_embed';
   +      $display->setComponent($name, $config);
   +    }
   +  }
   

   This will use our image style for every image-formatted component in the display. I think that's a bit overreaching. Instead, we should check if the source field is an ImageItem (or a subclass thereof), and if it is, we should configure only the source field to use our special image style (assuming it's using the image formatter, of course, and has no image style already applied). So that will be a bit more verbose, but I think it will also be less "invasive". Something like this:

   $source_field_definition = $type->getSource()->getSourceFieldDefinition($type);
   
   if (is_a(ImageItem::class, $source_field_definition->getItemDefinition()->getClass(), TRUE)) {
     $component = $display->getComponent($source_field_definition->getName());
     if ($component && $component['type'] === 'image' && $component['settings']['image_style'] === '') {
     // ...
     }
   }
   

I'm glad we were able to agree on a scope for this patch. This is cleaner, better, and constantly improving.

1. +++ b/core/modules/media/media.module
   @@ -496,3 +502,84 @@ function media_filter_format_edit_form_validate($form, FormStateInterface $form_
   +  $display = EntityViewDisplay::load('media.' . $type->id() . '.embed');
   +
   +  // If the embedded entity view display has already been configured, we don't
   +  // need to redo it.
   +  if ($display) {
   +    return FALSE;
   +  }
   

   We should use \Drupal::service('entity_display.repository')->getViewDisplay('media', $type->id(), 'embed') here, rather than calling static methods of EntityViewDisplay. It will create the display (without saving) if it doesn't exist. We can return FALSE if $display->isNew().

2. +++ b/core/modules/media/media.module
   @@ -496,3 +502,84 @@ function media_filter_format_edit_form_validate($form, FormStateInterface $form_
   +  // Set the image style to the default included in the the media module.
   +  if (is_a(ImageItem::class, $source_field_definition->getItemDefinition()->getClass(), TRUE)) {
   

   Let's change the comment a bit. I would suggest "If this media type is using an image field as its source field, set the image style to the 'large' image style included with the Image module."

3. +++ b/core/modules/media/media.module
   @@ -496,3 +502,84 @@ function media_filter_format_edit_form_validate($form, FormStateInterface $form_
   +      $config['settings']['image_style'] = 'large';
   +      $config['settings']['image_link'] = '';
   +      $display->setComponent($source_field_name, $config);
   

   What's $config? This should be:

   $component['settings']['image_style'] = 'large';
   $component['settings']['image_link'] = '';
   $display->setComponent($source_field_name, $component);
   

   Also, we should be returning early from this function if the 'large' image style doesn't exist.

4. +++ b/core/modules/media/media.post_update.php
   @@ -5,6 +5,50 @@
   +use Drupal\image\Entity\ImageStyle;
   

   This is no longer needed.

5. +++ b/core/modules/media/media.post_update.php
   @@ -5,6 +5,50 @@
   +      'dependencies' => [
   +        'enforced' => [
   +          'module' => [
   +            'media',
   +          ],
   +        ],
   +        'module' => [
   +          'media',
   +        ],
   +      ],
   

   We don't need to set the 'module' dependencies manually. The fact that it's an enforced dependency makes that redundant. So this should be:

   'dependencies' => [
     'enforced' => [
       'module' => ['media'],
     ],
   ],
   

6. +++ b/core/modules/media/media.post_update.php
   @@ -5,6 +5,50 @@
   +    $view_display_created = _media_configure_embed_view_display($type);
   +    if ($view_display_created) {
   +      $types[] = $type->label();
   +    }
   

   Nit: There's no need for $view_display_created to be its own variable.

7. +++ b/core/modules/media/tests/src/Functional/MediaEmbedDisplayModeTest.php
   @@ -0,0 +1,129 @@
   +    // Create a non-image media type through the UI.
   +    $athos = 'athos';
   +    $edit = [
   +      'label' => 'Athos',
   +      'id' => $athos,
   +      'source' => 'file',
   +    ];
   +    $this->drupalPostForm('admin/structure/media/add', $edit, 'Save');
   +    $this->drupalPostForm(NULL, [], t('Save'));
   

   Why do we need to do drupalPostForm() twice? That shouldn't be necessary...

8. +++ b/core/modules/media/tests/src/Functional/MediaEmbedDisplayModeTest.php
   @@ -0,0 +1,129 @@
   +    $this->assertSession()->pageTextContains("The Embed view display has been created for the Athos media type.");
   

   Nit: Let's call $assert_session = $this->assertSession() at the top of the method, and reuse that throughout. I'd like Media to standardize on one style of asserting things.

9. +++ b/core/modules/media/tests/src/Functional/MediaEmbedDisplayModeTest.php
   @@ -0,0 +1,129 @@
   +    $dartagnan = 'dartagnan';
   

   This isn't really needed.

10. +++ b/core/modules/media/tests/src/Functional/MediaEmbedDisplayModeTest.php
    @@ -0,0 +1,129 @@
    +    $this->assertSession()->pageTextNotContains("The Embed view display has been created for the $dartagnan media type.");
    

    This is a false assertion. It should be asserting the absence of the text "The Embed view display has been created for the d'Artagnan media type." (It should not have the label of the media type, not its ID.) This is yet another reason why we shouldn't be keeping the media type ID in its own variable. :)

11. +++ b/core/modules/media/tests/src/Functional/MediaEmbedDisplayModeTest.php
    @@ -0,0 +1,129 @@
    +   * Assumes the source type has a source field.
    

    There is no need for this line. All media types are required to have a source field.

12. +++ b/core/modules/media/tests/src/Functional/MediaEmbedDisplayModeTest.php
    @@ -0,0 +1,129 @@
    +    if ($type_id == 'image') {
    

    Except in certain unusual cases, we should use === everywhere.

13. +++ b/core/modules/media/tests/src/Functional/MediaEmbedDisplayModeTest.php
    @@ -0,0 +1,129 @@
    +      $this->assertNotEmpty($field);
    

    This can be $this->assertInternalType('array', $field).

14. +++ b/core/modules/media/tests/src/Functional/MediaEmbedDisplayModeTest.php
    @@ -0,0 +1,129 @@
    +      $this->assertSame('large', $field['settings']['image_style']);
    

    We should also assert that the image_link setting is empty.

15. +++ b/core/modules/media/tests/src/Functional/MediaEmbedDisplayModeTest.php
    @@ -0,0 +1,129 @@
    +      $source_field = $type->getSource()->getSourceFieldDefinition($type);
    +      $field = $display->getComponent($source_field->getName());
    +      $this->assertNotEmpty($field);
    

    This can be a little cleaner, IMHO, in this form:

    $component = $display->getComponent($type->getSource()->getSourceFieldDefinition($type)->getName());
    $this->assertInternalType('array', $component);
    

    Overall, this method should assert the view display exists and has an array component for the source field. Then, if the type's source plugin is 'image', it should also assert that the component has the correct image_style and image_link configuration. So generally I think we should refactor this whole method in that direction.

16. +++ b/core/modules/media/tests/src/Functional/Update/MediaUpdateTest.php
    @@ -114,4 +116,19 @@ public function testEnableStandaloneUrl() {
    +    $this->assertNull(ImageStyle::load('embed'));
    

    This, and its related use is not needed.

17. +++ b/core/modules/media/tests/src/Functional/Update/MediaUpdateTest.php
    @@ -114,4 +116,19 @@ public function testEnableStandaloneUrl() {
    +    $this->assertNull(EntityViewDisplay::load('media.file.embed'));
    +    $this->assertNull(EntityViewDisplay::load('media.image.embed'));
    

    Aren't there more media types for us to assert?

18. +++ b/core/profiles/standard/config/optional/core.entity_view_display.media.image.embed.yml
    @@ -0,0 +1,30 @@
    +langcode: en
    +status: true
    +dependencies:
    +  config:
    +    - core.entity_view_mode.media.embed
    +    - field.field.media.image.field_media_image
    +    - image.style.large
    +    - media.type.image
    +  module:
    +    - image
    +id: media.image.embed
    +targetEntityType: media
    +bundle: image
    +mode: embed
    +content:
    +  field_media_image:
    +    label: visually_hidden
    +    settings:
    +      image_style: large
    +      image_link: ''
    +    third_party_settings: {  }
    +    type: image
    +    weight: 1
    +    region: content
    +hidden:
    +  created: true
    +  langcode: true
    +  name: true
    +  thumbnail: true
    +  uid: true
     
    

    I think there's an extra blank line at the end of this file. (There should only be one.)

Almost there -- unfortunately, there's a big flaw in the test. :( But I don't think it'll take much effort to fix.

1. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaEmbedDisplayModeTest.php
   @@ -0,0 +1,127 @@
   +    // Wait for form to complete with ajax.
   

   Should be 'AJAX'.

2. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaEmbedDisplayModeTest.php
   @@ -0,0 +1,127 @@
   +    if ($type_id === 'image') {
   

   This is wrong. We are passing media type IDs into this method, and none of them are 'image'. We really should be loading the media type, getting the source field definition, and only asserting things if it's an image field. So, I think this method should look like this:

   $type = MediaType::load($type_id);
   $display = EntityViewDisplay::load(...);
   $this->assertInstanceOf(EntityViewDisplay::class, $display);
   $source_field_definition = $type->getSource()->getSourceFieldDefinition($type);
   $component = $display->getComponent($source_field_definition->getName());
   $this->assertInternalType('array', $component);
   
   if ($source_field_definition->getType() === 'image') {
     // Assert the image style and linking
   }
   <code>
   </li>
   
   <li>
   <code>
   +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaEmbedDisplayModeTest.php
   @@ -0,0 +1,127 @@
   +      $this->assertNotEmpty($field['settings']['image_link']);
   

   This should be assertEmpty(). We want to prove that the image is not linked in an embed.

Then, I noticed an architectural thing:

+++ b/core/modules/media/media.module
@@ -496,3 +502,85 @@ function media_filter_format_edit_form_validate($form, FormStateInterface $form_
+  if (is_a(ImageItem::class, $source_field_definition->getItemDefinition()->getClass(), TRUE)) {
+    $source_field_name = $source_field_definition->getName();
+    $component = $display->getComponent($source_field_name);
+    if ($component && $component['type'] === 'image' && $component['settings']['image_style'] === '') {
+      $component['settings']['image_style'] = 'large';
+      $component['settings']['image_link'] = '';
+      $display->setComponent($source_field_name, $component);
+    }
+  }

Looking at this more closely, I think we might be violating encapsulation here. This kind of logic is exactly what MediaSourceInterface::prepareViewDisplay() was intended to do, and so IMHO the Image source plugin should be responsible for doing this in its prepareViewDisplay() method. I think that will be much cleaner.

So, I suggest that we remove this block, and modify \Drupal\media\Plugin\media\Source\Image::prepareViewDisplay() -- we'll need to override the method -- like so:

public function prepareViewDisplay(MediaTypeInterface $type, EntityViewDisplayInterface $display) {
  parent::prepareViewDisplay($type, $display);

  if ($display->getMode() === 'embed') {
    $field_name = $this->getSourceFieldDefinition($type)->getName();
    $component = $display->getComponent($field_name);
    $component['settings']['image_style'] = 'large';
    $component['settings']['image_link'] = '';
    $display->setComponent($field_name, $component);
  }
}

Oh, this looks gooood...

1. +++ b/core/modules/media/media.module
   @@ -521,34 +519,22 @@ function _media_configure_embed_view_display(MediaTypeInterface $type) {
   +  // If this media type is using an image field as its source field, this will
   +  // set the image style to the 'large' image style included with the Image
   +  // module.
   

   I would suggest moving this comment to Image::prepareViewDisplay() and rephrasing it slightly. Something like: For the `embed` display mode, use the `large` image style and do not link the image to anything.

2. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
   @@ -161,4 +162,18 @@ public function createSourceField(MediaTypeInterface $type) {
   +    parent::prepareViewDisplay($type, $display);
   +    if ($display->getMode() === 'embed') {
   

   Can we get a blank line after the parent::prepareViewDisplay() call, for readability?

Thank you!

+++ b/core/modules/media/src/Plugin/media/Source/Image.php
@@ -167,6 +167,9 @@ public function createSourceField(MediaTypeInterface $type) {
+    // For the `embed` display mode, use the `large` image style and do not
+    //link the image to anything.

Second line of the comment needs a space after the //.

Also, I just realized this comment doesn't explain why we're doing this. So let's add some more wording. I suggest: "This is done in order to ensure the image does not become too unwieldy when embedded into formatted text, and to allow it to wrapped by an author-created link."

1. +++ b/core/modules/media/media.post_update.php
   @@ -5,6 +5,43 @@
   +  // Embeds needs a special view display to make sure the out-of-the-box
   

   Should be "Embeds need..."

2. +++ b/core/modules/media/media.post_update.php
   @@ -5,6 +5,43 @@
   +  // display doesn't use very large images. It was not automatically
   +  // created for custom media types, so let's make sure this is fixed.
   

   This is a bit inaccurate. The display is automatically created for custom media types, it just needs to be re-created for existing media types. Maybe we should change that sentence to "Ensure that all existing media types have an embed display."

3. +++ b/core/modules/media/tests/src/Kernel/MediaEmbedFilterTest.php
   --- /dev/null
   +++ b/core/profiles/demo_umami/config/install/core.entity_view_display.media.image.embed.yml
   

   We should probably update DemoUmamiProfileTest to ensure this display exists and looks the way we expect.

4. +++ b/core/profiles/demo_umami/config/install/core.entity_view_display.media.image.embed.yml
   --- /dev/null
   +++ b/core/profiles/standard/config/optional/core.entity_view_display.media.image.embed.yml
   

   Same for StandardTest.

DemoUmamiProfileTest::assertDefaultConfig and DemoUmamiProfileTest::testConfig already assures the config is installed in that profile. What were you thinking of as far as making sure it looks OK? I mean it's just using the large image style. What do we need to test?

I was wrong about DemoUmamiProfileTest; we can leave that one alone. StandardTest, though, has a testStandard() method which, among other things, does some testing of the media types bundled with Standard. So, my recommendation would be to assert that the Image media type has an "embed" view display enabled, and that is uses the Large image style and no link; for every other media type, we should assert that there is no embed display (i.e., when on the "Manage display" page, under "Custom settings", the "Embed" checkbox should exist and not be checked).

+++ b/core/modules/media/src/Plugin/media/Source/Image.php
@@ -173,6 +174,25 @@ public function prepareViewDisplay(MediaTypeInterface $type, EntityViewDisplayIn
+      $storage = $this->entityTypeManager->getStorage('image_style');
+      $image_style = $storage->load('large');
+      // If the site builder has deleted the large image style, recreate it.
+      if (!$image_style) {
+        $image_style = $storage->create([
+          'name' => 'large',
+          'label' => 'Large (480×480)',
+        ]);
+        $image_style->addImageEffect([
+          'id' => 'image_scale',
+          'weight' => 0,
+          'data' => [
+            'width' => 480,
+            'height' => 480,
+            'upscale' => FALSE,
+          ],
+        ]);
+        $image_style->save();
+      }

I don't think I'm in favor of this. If the site builder deletes the Large image style, there's a 99% chance they did it on purpose, and they won't want to see it come back inexplicably without their input. Besides, thanks to the config dependency system, these "Embed" 'displays will be automatically deleted if the image style is.

IMHO, if they delete the Large image style, we should just fall back to the original-sized raw image. But let's get feedback from others before proceeding with that.

OK, sweet, let me confirm that's true, but if that's the case, I'm OK with using the media library image style as the fallback.

We can't, sadly. The media_library image style comes from Media Library, but the embedded view mode comes from Media. So that creates an implicit dependency on Media Library (and, since Media Library depends on Media, it's actually a circular dependency). We'd have to provide a new fallback image style in this patch. Which might actually be the best solution, since it echoes existing precedents in core. Let's get a committer (framework or product manager) to weigh in.

I think we should do what we did for the media_library image style. Create a new 'embed' image style which, apart from the label and machine name, is identical to 'large', and remove the ability to delete it in the UI. It should never be automatically recreated.

1. +++ b/core/modules/media/config/install/core.entity_view_mode.media.embed.yml
   @@ -0,0 +1,12 @@
   +dependencies:
   +  enforced:
   +    module:
   +      - media
   +  module:
   +    - media
   

   I'm not sure we need both module: [media] and enforced: {module: [media]}. I think we can just use the enforced dependency.

2. +++ b/core/modules/media/config/install/core.entity_view_mode.media.embed.yml
   @@ -0,0 +1,12 @@
   +cache: true
   

   In accordance with Wim's feedback in #51.1, we can set this to false.

3. +++ b/core/modules/media/media.install
   @@ -118,6 +122,36 @@ function media_requirements($phase) {
   +    $view_mode = EntityViewMode::load('media.embed');
   +    if (!empty($view_mode)) {
   

   I think we should have a good long comment above this section, explaining what we're doing and why.

4. +++ b/core/modules/media/media.install
   @@ -118,6 +122,36 @@ function media_requirements($phase) {
   +      foreach (MediaType::loadMultiple() as $type) {
   

   I think that, inside this foreach loop, we have a lot of nesting which is making it a bit harder to follow the logic. I'd prefer if we did early continues for failing return conditions, like so:

   foreach (MediaType::loadMultiple() as $type) {
     if (!$source_field_is_an_image) {
       continue;
     }
     // ...
     $component = $display->getComponent($field_name);
     if (!$component || $component['type'] !== 'image') {
       continue;
     }
   }
   

5. +++ b/core/modules/media/media.install
   @@ -118,6 +122,36 @@ function media_requirements($phase) {
   +        if (is_a($source, Image::class, TRUE)) {
   

   We should be filtering by the field item type (which we can ascertain by $source_field_definition->getItemDefinition()->getClass()), not source plugin class. In this case, we want to target media types which have a source field that uses Drupal\image\Plugin\Field\FieldType\ImageItem or any subclass thereof.

6. +++ b/core/modules/media/media.install
   @@ -118,6 +122,36 @@ function media_requirements($phase) {
   +            if ($component['settings']['image_style'] === '') {
   

   We also need to check if the $component['type'] is 'image', since that's the image formatter.

7. +++ b/core/modules/media/media.install
   @@ -118,6 +122,36 @@ function media_requirements($phase) {
   +                $action_item = new TranslatableMarkup('If you would like to change this, <a href=":display">add an image style to the field %field_name</a>.', ['%field_name' => $source_field->label(), ':display' => $url]);
   

   Can the wording of the link be changed to "add an image style to the %field_name field"?

8. +++ b/core/modules/media/media.install
   @@ -118,6 +122,36 @@ function media_requirements($phase) {
   +                'description' => new TranslatableMarkup('The %view_mode_label display for the media type %type is not currently using an image style on the field %field_name. Not using an image style can lead to much larger file downloads. @action_item', ['%view_mode_label' => $view_mode->label(), '%field_name' => $source_field->label(), '@action_item' => $action_item, '%type' => $type->label()]),
   

   Same here. Otherwise, this wording is great. Minor nit: the arguments should be on their own lines, I think, for clarity. Otherwise this is a super long line. So something like:

   'description' => new TranslatableMarkup('...', [
     '%view_mode_label' => $foo,
     '%type' => $type,
     // ...etc.
   ]),
   

9. +++ b/core/modules/media/media.install
   @@ -118,6 +122,36 @@ function media_requirements($phase) {
   +              if ($module_handler->moduleExists('field_ui')) {
   +                $url = Url::fromRoute('entity.entity_view_display.media.view_mode', ['media_type' => $type->id(), 'view_mode_name' => 'embed'])->toString();
   ...
   +              }
   
   +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaEmbedDisplayModeTest.php
   @@ -0,0 +1,150 @@
   +    $assert_session->linkByHrefExists('/admin/structure/media/manage/madame_bonacieux/display/embed');
   

   Nice. Covering our bases! 👍

   However, I think we also need to cover the cases where the Field UI module is not installed, or it is installed but the user does not have permission to administer the displays. To facilitate that, we might want to move Madame Bonacieux's stuff into its own test method.

10. +++ b/core/modules/media/media.install
    @@ -118,6 +122,36 @@ function media_requirements($phase) {
    +              if ($module_handler->moduleExists('field_ui')) {
    +                $url = Url::fromRoute('entity.entity_view_display.media.view_mode', ['media_type' => $type->id(), 'view_mode_name' => 'embed'])->toString();
    +                $action_item = new TranslatableMarkup('If you would like to change this, <a href=":display">add an image style to the field %field_name</a>.', ['%field_name' => $source_field->label(), ':display' => $url]);
    +              }
    

    We also need to check if the current user has access to configure the media displays (i.e., they have the 'administer media display' permission).

11. +++ b/core/modules/media/media.install
    @@ -118,6 +122,36 @@ function media_requirements($phase) {
    +              $requirements['media_embed_image_style_' . $type->id()] = [
    +                'title' => t('Media'),
    +                'description' => new TranslatableMarkup('The %view_mode_label display for the media type %type is not currently using an image style on the field %field_name. Not using an image style can lead to much larger file downloads. @action_item', ['%view_mode_label' => $view_mode->label(), '%field_name' => $source_field->label(), '@action_item' => $action_item, '%type' => $type->label()]),
    +                'severity' => REQUIREMENT_WARNING,
    +              ];
    

    This will, indeed, produce a warning for each media type which isn't using the image style. Good, I say! I think that having a clickable action item for each media type is more valuable than trying to aggregate all of these things into one big (and therefore confusing, overwhelming, and not really actionable) warning.

12. +++ b/core/modules/media/media.module
    @@ -508,3 +511,64 @@ function media_field_widget_form_alter(&$element, FormStateInterface $form_state
    +  catch (EntityStorageException $e) {
    +    \Drupal::messenger()->addError($e->getMessage());
    +    return;
    +  }
    

    We don't need the return here.

13. +++ b/core/modules/media/tests/src/Functional/Update/MediaUpdateTest.php
    @@ -114,4 +116,19 @@ public function testEnableStandaloneUrl() {
    +    $this->assertNull(ImageStyle::load('embed'));
    

    This line is no longer relevant.

14. +++ b/core/modules/media/tests/src/Functional/Update/MediaUpdateTest.php
    @@ -114,4 +116,19 @@ public function testEnableStandaloneUrl() {
    +    $this->assertNull(EntityViewDisplay::load('media.file.embed'));
    +    $this->assertNull(EntityViewDisplay::load('media.image.embed'));
    +    $this->runUpdates();
    +    $this->assertInstanceOf(EntityViewDisplay::class, EntityViewDisplay::load('media.file.embed'));
    +    $this->assertInstanceOf(EntityViewDisplay::class, EntityViewDisplay::load('media.image.embed'));
    

    Why aren't we also testing that the displays are created for the remote_video, audio_file, and video_file media types?

    Also, shouldn't we test that they are configured as expected (as in MediaEmbedDisplayModeTest)?

15. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaEmbedDisplayModeTest.php
    @@ -0,0 +1,150 @@
    + * Tests that media automatically configures an embed view mode and display.
    

    Nit: 'media' should be 'Media', since that's the proper name of the module.

16. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaEmbedDisplayModeTest.php
    @@ -0,0 +1,150 @@
    +      // We need 'access content' for system.machine_name_transliterate.
    +      'access content',
    

    🙄 What a WTF. Thanks for the comment :)

17. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaEmbedDisplayModeTest.php
    @@ -0,0 +1,150 @@
    +    // If for some reason a site builder deletes the 'large' image style, it
    +    // should be recreated when adding a new media type with an image source.
    

    This comment is not accurate. :)

18. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaEmbedDisplayModeTest.php
    @@ -0,0 +1,150 @@
    +    $assert_session->pageTextContains('The Embed display for the media type Madame Bonacieux is not currently using an image style on the field Image.');
    

    We should also assert the rest of the warning, including the text of the link.

19. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaEmbedDisplayModeTest.php
    @@ -0,0 +1,150 @@
    +      if (is_a(ImageStyle::load('large'), ImageStyleInterface::class, TRUE)) {
    

    I don't think we need to is_a() here. If we can load the image style at all, it's guaranteed to be an ImageStyleInterface instance.

20. +++ b/core/profiles/standard/tests/src/Functional/StandardTest.php
    @@ -256,6 +256,19 @@ public function testStandard() {
    +      $page->hasUncheckedField('display_modes_custom[full]');
    

    What is this for?

21. +++ b/core/profiles/standard/tests/src/Functional/StandardTest.php
    @@ -256,6 +256,19 @@ public function testStandard() {
    +        $page->hasCheckedField('display_modes_custom[embed]');
    

    This isn't an assertion. It should be $assert_session->checkboxChecked().

22. +++ b/core/profiles/standard/tests/src/Functional/StandardTest.php
    @@ -256,6 +256,19 @@ public function testStandard() {
    +        $this->assertTrue($assert_session->optionExists('fields[field_media_image][type]', 'image')->isSelected());
    

    I don't think we need the assertTrue(). $assert_session->fieldValueEquals() should do the trick.

23. +++ b/core/profiles/standard/tests/src/Functional/StandardTest.php
    @@ -256,6 +256,19 @@ public function testStandard() {
    +        $assert_session->elementTextContains('css', 'tr[data-drupal-selector="edit-fields-field-media-image"]', 'Image style: Large (480×480)');
    

    Oh man, that is detailed. I approve! 👍

24. +++ b/core/profiles/standard/tests/src/Functional/StandardTest.php
    @@ -256,6 +256,19 @@ public function testStandard() {
    +        $page->hasUncheckedField('display_modes_custom[embed]');
    

    This should be $assert_session->checkboxNotChecked().

Virtually everything else I found was nitpicks. This is really close to done. Since there is an update path, I'm tagging this for framework manager review (and ideally a framework manager will commit it).

1. +++ b/core/modules/media/media.install
   @@ -118,6 +122,61 @@ function media_requirements($phase) {
   +    // When a new media type is created, we're configuring the entity view
   +    // display for embedded media.  For media types with an image source,
   +    // we're using the 'large' image style.  Unfortunately, if a site builder
   +    // has deleted the 'large' image style, we need some other image style to
   +    // use, but at this point, we can't really know the site-builder's
   +    // intentions.  So rather than do something surprising, we're leaving the
   +    // embedded media without an image style and adding a warning that the site
   +    // builder might want to add an image style.
   

   This could use a @see comment for _media_configure_embed_view_display(). Also, some supernits: every sentence is separated by two spaces after the period, but it should just be one. Also, this is using "site builder" and "site-builder" interchangeably; it should just standardize on "site builder".

   Otherwise, this is 💯

2. +++ b/core/modules/media/media.install
   @@ -118,6 +122,61 @@ function media_requirements($phase) {
   +    if (!empty($view_mode)) {
   

   Supernit that you should feel completely free to ignore: in this case, if (!empty($view_mode)) is identical to if ($view_mode).

3. +++ b/core/modules/media/media.install
   @@ -118,6 +122,61 @@ function media_requirements($phase) {
   +        $display = $display_repository->getViewDisplay('media', $type->id(), 'embed');
   

   Nit: I don't think we really need $display_repository to be its own variable.

4. +++ b/core/modules/media/media.install
   @@ -118,6 +122,61 @@ function media_requirements($phase) {
   +        $item_class = $source_field_definition->getItemDefinition()->getClass();
   +        if (!is_a($item_class, ImageItem::class, TRUE)) {
   

   $item_class doesn't really need to be its own variable either.

5. +++ b/core/modules/media/media.install
   @@ -118,6 +122,61 @@ function media_requirements($phase) {
   +        $field_name = $source_field_definition->getName();
   +        $component = $display->getComponent($field_name);
   

   Nit: Same with $field_name.

6. +++ b/core/modules/media/media.install
   @@ -118,6 +122,61 @@ function media_requirements($phase) {
   +        if (empty($component) || $component['type'] !== 'image' || $component['settings']['image_style'] !== '') {
   

   Nit: I could change the last condition here to !empty($component['settings']['image_style']), since that's technically more "correct".

7. +++ b/core/modules/media/media.install
   @@ -118,6 +122,61 @@ function media_requirements($phase) {
   +          $url = Url::fromRoute('entity.entity_view_display.media.view_mode', ['media_type' => $type->id(), 'view_mode_name' => 'embed'])->toString();
   

   Nit: I would put the route parameters onto a new line, for readability, like so:

   $url = Url::fromRoute('entity.entity_view_display.media.view_mode', [
     'media_type' => $type->id(),
     'view_mode_name' => 'embed',
   ]);
   

   ...and then use $url->toString() when creating the variables for the TranslatableMarkup.

8. +++ b/core/modules/media/media.install
   @@ -118,6 +122,61 @@ function media_requirements($phase) {
   +          'description' => new TranslatableMarkup('The %view_mode_label display for the media type %type is not currently using an image style on the %field_name field. Not using an image style can lead to much larger file downloads. @action_item',
   

   Nit: It might read better to rearrange the words "media type %type" to "%type media type".

9. +++ b/core/modules/media/media.module
   @@ -508,3 +511,63 @@ function media_field_widget_form_alter(&$element, FormStateInterface $form_state
   +  return $display->save();
   

   This should be (bool) $display->save(), so that we're acting in accordance with the @return part of the doc comment.

10. +++ b/core/modules/media/media.module
    @@ -508,3 +511,63 @@ function media_field_widget_form_alter(&$element, FormStateInterface $form_state
    +function media_form_media_type_add_form_alter(&$form, FormStateInterface $form_state, $form_id) {
    

    &$form should be type hinted as an array.

11. +++ b/core/modules/media/media.post_update.php
    @@ -5,6 +5,39 @@
    +      'dependencies' => [
    +        'enforced' => [
    +          'module' => ['media'],
    +        ],
    +      ],
    

    This shouldn't be using enforced dependencies, as per the last few comments.

12. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
    @@ -161,4 +162,27 @@ public function createSourceField(MediaTypeInterface $type) {
    +    // For the `embed` display mode, use the `large` image style and do not
    +    // link the image to anything. This is done in order to ensure the image
    +    // does not become too unwieldy when embedded into formatted text, and to
    +    // allow it to wrapped by an author-created link.  If the `large` image
    +    // style has been deleted, do not set an image style.
    +    if ($display->getMode() === 'embed') {
    +      $field_name = $this->getSourceFieldDefinition($type)->getName();
    +      $component = $display->getComponent($field_name);
    +      $component['settings']['image_link'] = '';
    +      $component['settings']['image_style'] = '';
    +      if ($this->entityTypeManager->getStorage('image_style')->load('large')) {
    +        $component['settings']['image_style'] = 'large';
    +      }
    +      $display->setComponent($field_name, $component);
    +    }
    

    👍

13. +++ b/core/modules/media/tests/src/Functional/Update/MediaUpdateTest.php
    @@ -114,4 +117,42 @@ public function testEnableStandaloneUrl() {
    +  /**
    +   * Asserts the embed view display components for a media type.
    +   *
    +   * @param string $type_id
    +   *   The media type ID.
    +   */
    +  protected function assertViewDisplayConfigured($type_id) {
    +    $type = MediaType::load($type_id);
    +    $display = EntityViewDisplay::load('media.' . $type_id . '.embed');
    +    $this->assertInstanceOf(EntityViewDisplay::class, $display);
    +    $source_field_definition = $type->getSource()->getSourceFieldDefinition($type);
    +    $component = $display->getComponent($source_field_definition->getName());
    +    $this->assertInternalType('array', $component);
    +    if ($source_field_definition->getType() === 'image' && $component['type'] === 'image') {
    +      if (ImageStyle::load('large')) {
    +        $this->assertSame('large', $component['settings']['image_style']);
    +      }
    +      else {
    +        $this->assertEmpty($component['settings']['image_style']);
    +      }
    +      $this->assertEmpty($component['settings']['image_link']);
    +    }
    +  }
    

    It's too bad we need to repeat this code in another test, but I can't think of a more appropriate, shared place to put it. (Maybe MediaTypeCreationTrait would be correct? I dunno...I'd like a framework manager opinion on this.)

14. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaEmbedDisplayModeTest.php
    @@ -0,0 +1,171 @@
    +    $assert_session->pageTextContains('The Embed display for the media type Madame Bonacieux is not currently using an image style on the Image field. Not using an image style can lead to much larger file downloads.');
    +    $assert_session->pageTextNotContains('If you would like to change this, add an image style to the Image field.');
    

    Maybe we should also add an $assert_session->linkByHrefNotExists().

15. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaEmbedDisplayModeTest.php
    @@ -0,0 +1,171 @@
    +    $assert_session->pageTextContains('The Embed display for the media type Madame Bonacieux is not currently using an image style on the Image field. Not using an image style can lead to much larger file downloads. If you would like to change this, add an image style to the Image field.');
    +    $assert_session->linkByHrefExists('/admin/structure/media/manage/madame_bonacieux/display/embed');
    

    I would also add a call to $assert_session->linkExists() to ensure that the link has the correct text.

16. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaEmbedDisplayModeTest.php
    @@ -0,0 +1,171 @@
    +    // The image style warning should not include an action link when the
    +    // Field UI module is uninstalled.
    +    $this->container->get('module_installer')->uninstall(['field_ui']);
    +    $this->drupalGet('/admin/reports/status');
    +    $assert_session->pageTextContains('The Embed display for the media type Madame Bonacieux is not currently using an image style on the Image field. Not using an image style can lead to much larger file downloads.');
    +    $assert_session->pageTextNotContains('If you would like to change this, add an image style to the Image field.');
    

    This could probably also benefit from a call to $assert_session->linkByHrefNotExists().

+++ b/core/modules/media/media.post_update.php
@@ -5,6 +5,39 @@
+  if (!EntityViewMode::load('media.embed')) {
+    EntityViewMode::create([
+      'id' => 'media.embed',
+      'targetEntityType' => 'media',
+      'label' => t('Embed'),
+      'dependencies' => [
+        'enforced' => [
+          'module' => ['media'],
+        ],
+      ],
+    ])->save();
+  }

This is still using enforced dependencies, and it probably should not be. :)

I think that's all I see needing changing. Once that's done, I'm officially out of things to complain about. Fantastic work, this patch looks terrific!

Code looks good, so I gave this a quick manual test.

For the most part, it behaved exactly the way you'd expect, and it was just glorious. Creating a link around an embedded image media item worked flawlessly in CKEditor. So hooray for that!!

But then I tried creating a new "Remote video" media type, called "alpha". I got the expected message about the new Embed view display being created for the media type, but it looked like this when I went to configure it:

I think this will be a bit of a WTF for site builders, to be honest. If I create a new remote video media type, I'd expect that only "Remote video URL" would be shown in an embed context. It seems like the line for $display->set('content', []) is not taking effect? Either way, I think we might need to increase the test coverage a bit.

Another thing that is kind of annoying here is that, in a new installation, we don't ship Embed view displays for any media type but Image. Yet we always create an Embed view display for each new media type. That can lead to a situation where, in a site that has updated to Drupal 8.8, all media types will have an active Embed view display, but in a new site, only the Image media type will. That inconsistency has me wondering if we should ship an Embed view display for all media types we currently ship with Standard and Umami, just to keep things nice and consistent. That's a product manager decision, so I'm tagging this as needing product manager review.

Also, a minor thing: the label of the view mode is "Embed", not "Embedded". I found that confusing, and I suspect site builders might as well. I personally think we should change the view mode's label to "Embedded" (no need to change the machine name). However, since that is purely a string change, we don't need to do it in this issue if we don't want to (and, in fact, can do it all the way until RC, as I understand it).

similarity index 72%
rename from core/modules/media/config/install/core.entity_view_mode.media.embed.yml

rename from core/modules/media/config/install/core.entity_view_mode.media.embed.yml
rename to core/modules/media/config/install/core.entity_view_mode.media.embedded.yml

Yikes. This is overkill, IMHO -- we do not need to change the machine name of the view mode to match the label. In the UI, though, I think it would be clearer to say "Embedded" than "Embed" -- so, really, I was only suggesting that the label be changed. For now, let's revert all of this, because it is out of scope. We can easily change the label in another issue, and we can do it all the way through beta if we want to. I'd rather discuss that with the UX team or a product manager first anyway.

Thanks for changing the label; I think that will be clearer, in all honesty. Hopefully product managers agree.

1. +++ b/core/modules/media/media.install
   @@ -118,6 +122,63 @@ function media_requirements($phase) {
   +    // use, but at this point, we can't really know the site-builder's
   

   Nit: "site-builder" should be "site builder" (no hyphen).

2. +++ b/core/modules/media/media.install
   @@ -118,6 +122,63 @@ function media_requirements($phase) {
   +        $display = \Drupal::service('entity_display.repository')
   +          ->getViewDisplay('media', $type->id(), 'embed');
   +        if (empty($display)) {
   +          continue;
   +        }
   

   So, getViewDisplay() will never return an empty display; it creates it if it doesn't already exist. So we should probably change that to if ($display->isNew()).

3. +++ b/core/modules/media/tests/src/Functional/Update/MediaUpdateTest.php
   @@ -114,4 +117,42 @@ public function testEnableStandaloneUrl() {
   +    if ($source_field_definition->getType() === 'image' && $component['type'] === 'image') {
   

   I think this should be if ($source->getPluginId() === 'image'). The reason being that the logic which sets up the image style (or lack thereof) is confined to that specific plugin, and changing the if statement would make that a bit clearer.

4. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaEmbedDisplayModeTest.php
   @@ -0,0 +1,211 @@
   +    if ($source_field_definition->getType() === 'image' && $component['type'] === 'image') {
   

   I would also suggest changing this to if ($type->getSource()->getPluginId() === 'image'), for the reason I stated previously.

5. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaEmbedDisplayModeTest.php
   @@ -0,0 +1,211 @@
   +    // Assert the source field is enabled some other default fields are disabled.
   +    $assert_session->elementExists('css', 'input[name="' . $source_field_definition->getName() . '_settings_edit"]');
   +    $expected_hidden = [
   +      'uid',
   +      'created',
   +      'thumbnail'
   +    ];
   +    foreach ($expected_hidden as $field_name) {
   +      $assert_session->elementNotExists('css', 'input[name="' . $field_name . '_settings_edit"]');
   +    }
   

   The only flaw I can see here is that we are asserting that the uid, created, and thumbnail fields are not appearing, and then hoping that the lack of those three implies the absence of every other field (except the source field). What I might suggest is doing something like this instead:

   $assert_session->elementsCount('css', 'input[name$="_settings_edit"]', 1);
   $assert_session->buttonExists($source_field_definition->getName() . '_settings_edit');
   

I don't think it does, for the simple reason that all of this configuration belongs to the Media module and is intended to be used with the MediaEmbed filter that is part of Media. If sites want to prevent the view mode from being created -- which sounds like an edge case to me -- they would be better off just editing that function and returning early so that the view displays are not created.

I don't think we should do that in this issue. That would expand the scope, which is the last thing we need right now; "making media responsive" is a bigger issue, and should be dealt with in a follow-up in a more holistic way.

I discussed this with @effulgentsia and @webchick to solidify the plan here. We agreed on:

• We'll remove the "Embed" view mode, and all related config, from this patch.
• However, we will keep most of the code and test coverage. We like the fact that the Image source plugin tries to use the large image style, if available, when configuring a view display. It should do it for every new display, then, regardless of view mode. (In other words, just remove the if ($display->getMode() === 'embed') check.)
• We also like the fact that the status report will warn you if you have any media displays which aren't using an image style. So let's keep this, but change it so it creates warnings for the default view mode, rather than the now-defunct embed view mode.
• We will change the default view display we ship for the Image media type so it uses the large image style. This will not need an update path.
• But, let's open a follow-up issue to ship a full view display for the Image media type, which doesn't use the image style. (i.e., this allows us to preserve the previous behavior when viewing a canonical media item).

Re-titling to reflect the new directions we're taking. Also removing "needs framework manager review". Since there will be no update path in this patch, we don't need FM sign-off.

1. +++ b/core/modules/media/media.install
   @@ -118,6 +121,59 @@ function media_requirements($phase) {
   +      $display = \Drupal::service('entity_display.repository')
   +        ->getViewDisplay('media', $type->id(), 'default');
   

   We don't need to pass 'default' to getViewDisplay(); it has that as the default argument already. :)

2. +++ b/core/modules/media/media.install
   @@ -118,6 +121,59 @@ function media_requirements($phase) {
   +      $source = $type->getSource();
   +      $source_field_definition = $source->getSourceFieldDefinition($type);
   

   Nit: I don't think $source needs to be its own variable.

3. +++ b/core/modules/media/src/Plugin/media/Source/Image.php
   @@ -161,4 +162,35 @@ public function createSourceField(MediaTypeInterface $type) {
   +    if ($display->getMode() === 'default') {
   +      // Remove components other than the source field.
   +      foreach (array_keys($display->getComponents()) as $name) {
   +        if ($name !== $field_name) {
   +          $display->removeComponent($name);
   +        }
   +      }
   +    }
   

   This seems like a very strong opinion for any source plugin to have. I would suggest moving this snippet elsewhere; perhaps to MediaTypeForm::save(), or an implementation of hook_entity_view_display_presave(). The source plugin should really only be concerned with how its source field is going to be displayed.

4. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaDisplayTest.php
   @@ -101,10 +101,13 @@ public function testMediaDisplay() {
   +    $expected_image_src = file_url_transform_relative(file_create_url(\Drupal::token()->replace('public://styles/large/public/[date:custom:Y]-[date:custom:m]/example_1.jpeg')));
   

   😲 Methinks Drupal's file API is too complicated...

5. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaDisplayTest.php
   @@ -101,10 +101,13 @@ public function testMediaDisplay() {
   +    $assert_session->elementExists('css', '.field__label.visually-hidden');
   +    $assert_session->elementNotExists('css', '.field--name-field-media-image a');
   

   These seem a bit loose. What if we did this:

   $field = $assert_session->elementExists('css', '.field--name-field-media-image');
   $assert_session->elementExists('css', '.field__label.visually-hidden', $field);
   $assert_session->elementNotExists('css', 'a', $field);
   

   In other words, scope these assertions to the field we're checking out, rather than the entire page.

6. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceImageTest.php
   @@ -57,16 +62,118 @@ public function testMediaImageSource() {
   +    $image_element = $assert_session->elementExists('css', '.field--name-field-media-image img');
   +    $expected_image_src = file_url_transform_relative(file_create_url(\Drupal::token()->replace('public://styles/large/public/[date:custom:Y]-[date:custom:m]/example_1.jpeg')));
   +    $this->assertContains($expected_image_src, $image_element->getAttribute('src'));
   +    $assert_session->elementExists('css', '.field--name-field-media-image .field__label.visually-hidden');
   +    $assert_session->elementNotExists('css', '.field--name-field-media-image a');
   

   Same idea here, I'd say.

7. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceImageTest.php
   @@ -57,16 +62,118 @@ public function testMediaImageSource() {
   +    // Ensure the image has the correct alt attribute.
   +    $assert_session->elementAttributeContains('css', '.field--name-field-media-image img', 'alt', 'Image Alt Text 1');
   

   I think we could just use $this->assertSame('Image Alt Text 1', $image_element->getAttribute('alt')) here, rather than repeating selectors.