EditorMediaDialog triggers an "undefined index" notice for data-view-mode

Looks okay to me. We will, however, need a test. But it need not be complex -- it can be a unit or (preferably) kernel test. All it needs to do is directly execute the bad code path, and the notice will cause PHPUnit to fail the test.

This looks pretty good to me -- nice work, @oknate! I think we might be able to simplify parts of the test, though.

1. +++ b/core/modules/media/src/Form/EditorMediaDialog.php
   @@ -173,7 +173,8 @@ public function buildForm(array $form, FormStateInterface $form_state, EditorInt
   +    $default_view_mode = !empty($media_embed_element['data-view-mode']) ? $media_embed_element['data-view-mode'] : NULL;
   

   I think that, since we are using PHP 7 now, we can use the more terse null-coalesce operator here, like so:

   $default_view_mode = $media_embed_element['data-view-mode'] ?? NULL;
   

2. +++ b/core/modules/media/tests/src/Kernel/EditorMediaDialogTest.php
   @@ -0,0 +1,199 @@
   +    $image = File::create([
   +      'uri' => $this->getTestFiles('image')[0]->uri,
   +      'uid' => 2,
   +    ]);
   +    $image->setPermanent();
   +    $image->save();
   +
   +    // Create a sample media entity to be embedded.
   +    $media_type = $this->createMediaType('image', ['id' => 'image']);
   +    EntityViewMode::create([
   +      'id' => 'media.foobar',
   +      'targetEntityType' => 'media',
   +      'status' => TRUE,
   +      'enabled' => TRUE,
   +      'label' => $this->randomMachineName(),
   +    ])->save();
   +    EntityViewDisplay::create([
   +      'targetEntityType' => 'media',
   +      'bundle' => $media_type->id(),
   +      'mode' => 'foobar',
   +      'status' => TRUE,
   +    ])->removeComponent('thumbnail')
   +      ->removeComponent('created')
   +      ->removeComponent('uid')
   +      ->setComponent('field_media_image', [
   +        'label' => 'visually_hidden',
   +        'type' => 'image',
   +        'settings' => [
   +          'image_style' => 'medium',
   +          'image_link' => 'file',
   +        ],
   +        'third_party_settings' => [],
   +        'weight' => 1,
   +        'region' => 'content',
   +      ])
   +      ->save();
   +    $media = Media::create([
   +      'uuid' => static::EMBEDDED_ENTITY_UUID,
   +      'bundle' => 'image',
   +      'name' => 'Screaming hairy armadillo',
   +      'field_media_image' => [
   +        [
   +          'target_id' => $image->id(),
   +          'alt' => 'default alt',
   +          'title' => 'default title',
   +        ],
   +      ],
   +    ])->setOwner($user);
   +    $media->save();
   +    $this->embeddedEntity = $media;
   +
   +    FilterFormat::create([
   +      'format' => 'test_format',
   +      'name' => 'Test format',
   +      'filters' => [
   +        'media_embed' => ['status' => TRUE],
   +      ],
   +    ])->save();
   +    $this->editor = Editor::create([
   +      'editor' => 'ckeditor',
   +      'format' => 'test_format',
   +    ]);
   +    $this->editor->save();
   

   This all seems pretty intense -- more like stuff we'd need in a functional test. Can we get away with using mocked objects for at least some of these things?

3. +++ b/core/modules/media/tests/src/Kernel/EditorMediaDialogTest.php
   @@ -0,0 +1,199 @@
   +    $this->userInput = [
   +      'editor_object' => [
   +        'attributes' => [
   +          'data-entity-type' => 'media',
   +          'data-entity-uuid' => $this->embeddedEntity->uuid(),
   +          'data-align' => 'center',
   +        ],
   +        'hasCaption' => 'false',
   +        'label' => $this->embeddedEntity->label(),
   +        'link' => '',
   +        'hostEntityLangcode' => $this->embeddedEntity->language()->getId(),
   +        'classes' => '',
   +      ],
   +    ];
   

   I don't think this needs to be in a property of the class.

4. +++ b/core/modules/media/tests/src/Kernel/EditorMediaDialogTest.php
   @@ -0,0 +1,199 @@
   +    $this->assertEquals('default alt', $build['alt']['#placeholder']);
   +    $this->assertEquals('center', $build['align']['#default_value']);
   +    $this->assertEquals(FALSE, $build['caption']['#default_value']);
   +    $this->assertEmpty($build['view_mode']['#options']);
   +    $this->assertEquals(FALSE, $build['view_mode']['#default_value']);
   

   So, I don't know if we need these assertions. We're just testing that the code path doesn't trigger a notice. So honestly, if $build comes back at all without failing the test, we can just use $this->pass() to mark the test as passed.

Did some refactoring of the test; I was able to remove a lot of lines. It's more sparse now, but we can always add more coverage to it later as needed. For now, it's best to just have it test the one thing we want it to test.

Reverting an accidental out-of-scope change here.

Two small changes and this should be ready.

1. +++ b/core/modules/media/tests/src/Kernel/EditorMediaDialogTest.php
   @@ -0,0 +1,94 @@
   +    'editor',
   

   The editor module is not necessary for the test.

2. +++ b/core/modules/media/tests/src/Kernel/EditorMediaDialogTest.php
   @@ -0,0 +1,94 @@
   +    $this->installSchema('system', 'sequences');
   

   The system sequences schema is not necessary for the test.

#12: nice catches!

+++ b/core/modules/media/tests/src/Kernel/EditorMediaDialogTest.php
@@ -0,0 +1,94 @@
+class EditorMediaDialogTest extends KernelTestBase {
...
+    $form_state = new FormState();
+    $form_state->setUserInput([
...
+    $form_state->setRequestMethod('POST');
...
+    EditorMediaDialog::create($this->container)
+      ->buildForm([], $form_state, $editor->reveal());
+    $this->pass('Form was built without errors.');

Clever :) I like it. Much simpler than the usual functional tests.

Thank you both! Made the changes requested by @bnjmnm.

Re-rolled the patch

Thanks, @Meenakshi.g!

Committed and pushed 61aa003c83 to 9.0.x and d44b969798 to 8.9.x and 2f209a4eb0 to 8.8.x. Thanks!

Backported to 8.8.x as this is a non disruptive bug fix.