Provide better defaults to media form displays

Suprised to see it even below the save button, isn't that supposed to be #weight 99 or something due to being a type actions?

This looks really good. Just found one thing:

+++ b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
@@ -53,6 +53,11 @@ public function testMediaTypeCreationFormWithDefaultField() {
+    $this->drupalGet("/media/add/$mediaTypeMachineName");
+    $assert_session->elementNotExists('css', 'form #edit-actions + div');

Could we also verify somehow that the source field is now directly under the name field?

I usually do that by checking the position of a string in the text, not sure if there is an official way, at least there wasn't in Simpletest.

So basically get the text, then do a strpos for "Name", "File", "Url Alias" or so, and check that the position of Name is lower than that of "File" and so on.

1. +++ b/core/modules/media/src/MediaSourceBase.php
   @@ -330,7 +330,10 @@ public function prepareViewDisplay(MediaTypeInterface $type, EntityViewDisplayIn
      public function prepareFormDisplay(MediaTypeInterface $type, EntityFormDisplayInterface $display) {
   -    $display->setComponent($this->getSourceFieldDefinition($type)->getName());
   +    // Make sure the source field is placed just after the "name" basefield.
   +    $display->setComponent($this->getSourceFieldDefinition($type)->getName(), [
   +      'weight' => $display->getComponent('name')['weight'] + 0.1,
   +    ]);
   

   I see what you're trying to do, but the weight is defined as an integer in the schema, so should we really set it to a float here? I think it will be cast to an integer again at some point due to the config schema casting/validation, but that also means it is somewhat unpredictable..

   I'd just do a + 1, worst case is that it then has the same weight as the next field, which is still better than right now. Or we explicitly re-set all weights but I think that's overkill.

   You also blindly rely on the name component being visible, but we for example have issues tha are discussing to hide it by default (Although, has commented there, I think that's the wrong approach). But better safe than sorry. Check that you have a name component, fall back to -50 or something? If there's no name, it should probably be first..

2. +++ b/core/modules/media/tests/modules/media_test_source/src/Plugin/media/Source/TestDifferentDisplays.php
   @@ -31,6 +32,7 @@ public function prepareViewDisplay(MediaTypeInterface $type, EntityViewDisplayIn
       */
      public function prepareFormDisplay(MediaTypeInterface $type, EntityFormDisplayInterface $display) {
   +    parent::prepareFormDisplay($type, $display);
        $display->setComponent($this->getSourceFieldDefinition($type)->getName(), [
          'type' => 'entity_reference_autocomplete_tags',
        ]);
   

   I don't think this is working as you think it does.

   setComponent() doesn't merge, not like that. Especially not the weight, you can see in the method that if there's no weight in the input, it explicitly sets it to the heighest weight. So pretty sure the second call would then unset your weight again.

   That means doing this properly either requires you to get the current component configuration, set the weight and set it again or offer someting like a getDefaultComponentWeight($display) method that can be re-used by the child instead of calling the parent.

3. +++ b/core/modules/media/tests/modules/media_test_source/src/Plugin/media/Source/TestWithHiddenSourceField.php
   @@ -22,6 +22,7 @@ class TestWithHiddenSourceField extends Test {
       */
      public function prepareViewDisplay(MediaTypeInterface $type, EntityViewDisplayInterface $display) {
   +    parent::prepareViewDisplay($type, $display);
        $display->removeComponent($this->getSourceFieldDefinition($type)->getName());
      }
   

   and this is pretty bogus too, if all we do is remove the component again.

+++ b/core/modules/media/tests/modules/media_test_source/src/Plugin/media/Source/TestDifferentDisplays.php
@@ -22,7 +22,6 @@ class TestDifferentDisplays extends Test {
   public function prepareViewDisplay(MediaTypeInterface $type, EntityViewDisplayInterface $display) {
-    parent::prepareViewDisplay($type, $display);
     $display->setComponent($this->getSourceFieldDefinition($type)->getName(), [
       'type' => 'entity_reference_entity_id',
     ]);
@@ -32,7 +31,6 @@ public function prepareViewDisplay(MediaTypeInterface $type, EntityViewDisplayIn

Why not calling the parent, getting the component, changing the type and then re-setting the component like @berdir suggested

+++ b/core/modules/media/tests/modules/media_test_source/src/Plugin/media/Source/TestDifferentDisplays.php
@@ -22,18 +22,20 @@ class TestDifferentDisplays extends Test {
-    $display->setComponent($this->getSourceFieldDefinition($type)->getName(), [
-      'type' => 'entity_reference_entity_id',
-    ]);
+    $source_name = $this->getSourceFieldDefinition($type)->getName();

Now re-add the parent::prepareViewDisplay(), then everything should be in place

Nice, +1 for RTBC from me

1. +++ b/core/modules/media/src/MediaSourceBase.php
   @@ -330,7 +330,12 @@ public function prepareViewDisplay(MediaTypeInterface $type, EntityViewDisplayIn
   +    $source_weight = ($name_component && array_key_exists('weight', $name_component)) ? $name_component['weight'] + 1 : -50;
   

   We should change array_key_exists() to isset(), to catch null values.

2. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
   @@ -53,6 +53,15 @@ public function testMediaTypeCreationFormWithDefaultField() {
   +    $form_text = $assert_session->elementExists('css', 'form')->getText();
   

   This could get screwed up if any other form appears on the page, for any reason. Can we find the form using a more specific selector?

3. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
   @@ -53,6 +53,15 @@ public function testMediaTypeCreationFormWithDefaultField() {
   +    $this->assertTrue(strpos($form_text, 'Name') < strpos($form_text, 'Test source'));
   +    $this->assertTrue(strpos($form_text, 'Test source') < strpos($form_text, 'Vertical Tabs'));
   +    // The "Published" checkbox should be the last element.
   +    $this->assertTrue(strpos($form_text, 'Leave blank to use the time of form submission.') < strpos($form_text, 'Published'));
   

   I'm not sure how else to do this, but asserting the relative positions of text seems quite brittle to me, and prone to things like false positives or false negatives. I'll try to think of an alternate approach to that...

I found a better way to assert the position of the elements! :)

I think we should also patch core/profiles/standard/config/optional/core.entity_form_display.media.*.yml with some hard-coded weight that's not the current value of 26.

+++ b/core/modules/media/src/MediaSourceBase.php
@@ -330,7 +330,12 @@ public function prepareViewDisplay(MediaTypeInterface $type, EntityViewDisplayIn
+    $source_weight = ($name_component && isset($name_component['weight'])) ? $name_component['weight'] + 1 : -50;

Are we sure we want to do it this way? The weight of 'name' is set by Media::baseFieldDefinitions() to -5. The weight of the URL alias field is set by path_entity_base_field_info() to 30. That gives us a lot of numbers in between, including 0 and -1. Would it be better to just pick one of those, and thereby leaving some room for contrib to put things in-between? Or alternatively, changing 'name' to -20 and setting the source field to -10, or something along those lines?

@phenaproxima this worked locally for me.

Small-ish comments below.

1. +++ b/core/modules/media/src/MediaSourceBase.php
   @@ -330,7 +330,12 @@ public function prepareViewDisplay(MediaTypeInterface $type, EntityViewDisplayIn
   +    $source_weight = ($name_component && isset($name_component['weight'])) ? $name_component['weight'] + 1 : -50;
   

   I don't think this var is named right as this is really the new component's weight. Also, what happens when the condition is False?

2. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
   @@ -1,6 +1,7 @@
   +use Drupal\Component\Utility\Html;
   

   This needs a space between namespace and use.

3. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
   @@ -53,6 +54,27 @@ public function testMediaTypeCreationFormWithDefaultField() {
   +    // come before the vertical tabs.
   

   this comment could be more clear "before the test field"

Addressed both @effulgentsia and @robpowell's feedback here...

I think we should also patch core/profiles/standard/config/optional/core.entity_form_display.media.*.yml with some hard-coded weight that's not the current value of 26.

Done.

Would it be better to just pick one of those, and thereby leaving some room for contrib to put things in-between?

Yes, it would! I decided to make the source field's weight name + 5.

I don't think this var is named right as this is really the new component's weight.

Discussed with @robpowell, and renamed the variable to $source_field_weight.

In reviewing this I also noticed #2935159: Give default comment fields a heavier weight so they are at the bottom and new fields don't get "lost" , but that happens with any fields, not just Media fields, so it's out of scope for the initiative.

+++ b/core/profiles/standard/config/optional/core.entity_form_display.media.file.default.yml
@@ -22,7 +22,7 @@ content:
       progress_indicator: throbber
     third_party_settings: {  }
     type: file_generic
-    weight: 26
+    weight: 0
     region: content
   name:
     type: string_textfield
diff --git a/core/profiles/standard/config/optional/core.entity_form_display.media.image.default.yml b/core/profiles/standard/config/optional/core.entity_form_display.media.image.default.yml

diff --git a/core/profiles/standard/config/optional/core.entity_form_display.media.image.default.yml b/core/profiles/standard/config/optional/core.entity_form_display.media.image.default.yml
index 6f2323b778..4488b02eeb 100644

index 6f2323b778..4488b02eeb 100644
--- a/core/profiles/standard/config/optional/core.entity_form_display.media.image.default.yml

--- a/core/profiles/standard/config/optional/core.entity_form_display.media.image.default.yml
+++ b/core/profiles/standard/config/optional/core.entity_form_display.media.image.default.yml

+++ b/core/profiles/standard/config/optional/core.entity_form_display.media.image.default.yml
+++ b/core/profiles/standard/config/optional/core.entity_form_display.media.image.default.yml
@@ -24,7 +24,7 @@ content:

@@ -24,7 +24,7 @@ content:
       preview_image_style: thumbnail
     third_party_settings: {  }
     type: image_image
-    weight: 26
+    weight: 0
     region: content
   name:

I think we probably also need to change them for the audio and video form displays now?

Lets do this.

Testing this patch keeps giving me more issues to file. :)

Committed and pushed to 8.5.x. Thanks!