Do not show name by default in media displays

I think in most cases you don't want to have that name shown at all, so maybe that's what we should do by default?

If you want to have a visible text, then it's likely an additional description/caption field or so, the name is something that is mostly just used in the backend.

You can have it configurable and hidden by default by just using ->setDisplayConfigurable('view', TRUE) without having a ->setDisplayOptions('view') call, so that might be an option?

You want both. Your patch does it for the image media type, what I wrote is to have that the default when creating a new media type and the view display is initially created from the base field definitions.

Should we have a test of this?

I think we should also add a test to verify it is actually hidden for the image media type. Maybe just add it to MediaSourceImageTest?

I was thinking that we should test it functionally, by actually adding a media item, then visiting that item and asserting that its name does not appear...

While looking at it this morning, I noticed we don't have a functional media display test yet? So we might want to add MediaDisplayTest. I started working on something but got distracted, so let me share what I started.

The media name is shown as the page title on the media detail page, so the assert should be changed to check for the name field by class/ID or something.

namespace Drupal\Tests\media\Functional;

use Drupal\media\Entity\Media;

/**
 * Basic display tests for Media.
 *
 * @group media
 */
class MediaDisplayTest extends MediaUiFunctionalTest {

  /**
   * Test basic media display.
   */
  public function testMediaDisplay() {
    $assert_session = $this->assertSession();
    $media_type = $this->createMediaType();

    // Create media.
    $media = Media::create([
      'bundle' => $media_type->id(),
      'name' => 'Fantastic!',
    ]);
    $media->save();

    $this->drupalGet('media/' . $media->id());
    // Name should be hidden by default.
    $assert_session->pageTextNotContains('Fantastic!');
  }

}

+1 to a test like that, that would also have failed on #2775131: Media entities should support contextual links

+++ b/core/modules/media/templates/media.html.twig
@@ -32,17 +32,5 @@
  */
 #}
 <div{{ attributes }}>
-  {#
-    In the 'full' view mode the entity label is assumed to be displayed as the
-    page title, so we do not display it here.
-  #}

+++ b/core/modules/media/tests/src/FunctionalJavascript/MediaDisplayTest.php
@@ -0,0 +1,87 @@
+    $this->drupalGet('media/' . $media->id());

What about the template in stable and classy?

We already started a discussion about changing this or not in the contextual links issue.

I'm also still not sure what the meaning of title_prefix and suffix is. I thought it is about contextual links, but actually, while we have *tons* of templates with that, I can only see one usage of it and that's some shortcut thing.

Should we add a test to verify the display of media in an entity reference field? I think we should verify the media name is not added there.

Thanks @marcoscano, we needed this :).
Could we add 1 more assert, for the image media referenced in the node, that the media name is 100% not in the page? We now have an assert for the name field, but if someone adds the H2 again in another issue we might not catch that?

I think this looks really good. I have a few suggestions, but they really are just suggestions and don't need to block RTBC.

1. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaDisplayTest.php
   @@ -0,0 +1,150 @@
   +  /**
   +   * Use the Standard profile for this test.
   +   *
   +   * @var string
   +   */
   +  protected $profile = 'standard';
   

   Ye gods. This slows things down to a crawl; is there any possible chance of using the testing profile instead?

2. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaDisplayTest.php
   @@ -0,0 +1,150 @@
   +    $this->drupalGet('media/' . $media->id());
   +    $assert_session->elementExists('css', '.field--name-name');
   +    $name_field = $page->find('css', '.field--name-name .field__item');
   +    $this->assertEquals($media->label(), $name_field->getText());
   

   Let's add a comment explaining what this is doing.

3. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaDisplayTest.php
   @@ -0,0 +1,150 @@
   +    $this->assertEquals(1, count($page->findAll('css', 'article.media--type-image > div')));
   +    // 2) Assert the image is present, with "medium" image style.
   +    $assert_session->elementExists('css', 'article.media--type-image img.image-style-medium');
   

   Didn't we have an issue open at some point to wrap media items in div tags, not article tags? If so, we should probably loosen or change this selector in the name of future proofing.

   Also, I believe we can rewrite this to harden the assertions a little:

   $media_item = $assert_session->elementExists('css', 'article.media--type-image > div');
   $assert_session->elementExists('css', 'img.image-style-medium', $media_item);
   

4. +++ b/core/modules/media/tests/src/Kernel/MediaTest.php
   @@ -21,14 +21,17 @@ public function testEntity() {
   +   * Tests the Media name basefield behavior.
   

   Nit: 'base field' should be two words, and 'name' should be quoted.

1. The config is in standard, so we kind of have to test that.

1. The config is in standard, so we kind of have to test that.

Yes, but we could install Testing, read the config from Standard, and create it in the test's setUp() method. Installing all of Standard seems like overkill.

Thanks, @marcoscano! I have but nitpicks, and then this is RTBC.

1. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaDisplayTest.php
   @@ -16,11 +20,21 @@
   +    if (is_dir($optional_install_path)) {
   

   We don't need this check; we know that Standard's optional config directory exists, and it'd be a serious problem if it didn't. :)

2. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaDisplayTest.php
   @@ -16,11 +20,21 @@
   +      $storage = new FileStorage($optional_install_path, StorageInterface::DEFAULT_COLLECTION);
   

   FileStorage's constructor already specifies StorageInterface::DEFAULT_COLLECTION as its default second argument, so we don't need to pass it here.

3. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaDisplayTest.php
   @@ -117,7 +148,7 @@ public function testMediaDisplay() {
   +    EntityViewDisplay::load("node.{$node_type->id()}.default")
   

   Let's use entity_get_display() here, since it's more robust.

Seems like everything is addressed. Nice work @marcoscano!

APCU out-of-memory failures are not real failures. Back to RTBC.

Just some nits.

+++ b/core/modules/media/templates/media.html.twig
@@ -32,17 +32,5 @@

@@ -32,17 +32,5 @@
  */
 #}
 <div{{ attributes }}>
-  {#
-    In the 'full' view mode the entity label is assumed to be displayed as the
-    page title, so we do not display it here.
-  #}
-  {{ title_prefix }}
-  {% if label and view_mode != 'full' %}
-    <h2{{ title_attributes }}>
-      {{ label }}
-    </h2>
-  {% endif %}
-  {{ title_suffix }}
-

Just noting this was discussed in #21/22 and the Stable theme already doesn't have this hunk, so it's OK to remove here and won't affect themes using Stable.

1. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaDisplayTest.php
   @@ -0,0 +1,172 @@
   +    // Instal the optional configs from the standard profile.
   

   Install

2. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaDisplayTest.php
   @@ -0,0 +1,172 @@
   +
   +    // Here we expect to see only the image, nothing else.
   +    // 1) Assert only one element in the content region.
   +    $this->assertEquals(1, count($page->findAll('css', '.media--type-image > div')));
   +    // 2) Assert the image is present inside the media element, with "medium"
   +    // image style.
   

   Could lose the 1) and 2) here, I don't think they add much.

3. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaDisplayTest.php
   @@ -0,0 +1,172 @@
   +
   +    // Here we expect to see only the linked filename.
   +    // 1) Assert only one element in the content region.
   +    $this->assertEquals(1, count($page->findAll('css', 'article.media--type-file > div')));
   +    // 2) Assert the file link is present, and its text matches the filename.
   +    $assert_session->elementExists('css', 'article.media--type-file .field--name-field-media-file a');
   

   And again here.

Nice and clean. RTBC when testbot smiles upon it.

Testbot random failed again. Back to RTBC.

Committed 26235da and pushed to 8.5.x. Thanks!

This issue relates to #2923701: Mechanism to disable preprocessing of node base fields so they can be configured via the field UI which I am working on hopefully for 8.6.

I think that the net result of this check-in leaves some problems:

• UX: the 'Name' field appears in Manage Display for 'Full Content', but any attempt by the user to manipulate the setting has no effect.
• UX: the Media 'Name' field in other view modes is displayed as plain text, where-as other core entity types use h2 with a link.
• Bug: the 'Name' field will not contain attributes set on the field (e.g. RDFa, in-place editing) - because when the field is hidden that markup is stripped - see comment in EntityViewController->buildTitle for how it does work for nodes.

The issue I am working on will solve these problems. I would welcome comment from participants in this thread

• Help get the exact details for media entity right in my issue.
• Consider if there ought to be a fix for 8.5.x.

Update: the last comment was not entirely correct.

New issue that is relevant here: #2923701: Mechanism to disable preprocessing of node base fields so they can be configured via the field UI. My findings are that when viewing /media/NN:

1. manage name set to displayed => tags are escaped from the page title; page header illegal markup div inside h1
2. manage name set to hidden => tags are stripped from the page title

Hence an unexpected side-effect of this commit is that it changes the default behaviour from 1) to 2).

I think the new issue @AdamPS meant to link to was actually #2941208: Title formatting broken due to flawed EntityViewController->buildTitle

@jonathanshaw oops thanks for the correction