Problem/Motivation

When adding a new media type (and media_library module is enabled), new form and view displays are configured automatically. This was added in #2988433: Automatically create and configure Media Library view and form displays.

The media name field is supposed to get special treatment. When the name field is mapped in media source plugin's field mapping, the name field will be hidden from the new media_library form display. If it isn't mapped to anything, the name field will be shown on the form display.

This works fine for the audio, document, image, and video source plugins. However when adding a new media type using the remote video source, the name field is always shown on the media_library view display.

The relevant code is in media_library.module, in _media_library_configure_form_display():

  // Expose the name field when it is not mapped.
  $field_map = $type->getFieldMap();
  if (empty($field_map['name'])) {
    $display->setComponent('name', [
      'type' => 'string_textfield',
      'settings' => [
        'size' => 60,
      ],
    ]);
  }

The problem is that we are looking for an array key called 'name', but this isn't present in the field map for the remote video media source. So the name field will always be shown on the form display, regardless of whether it has been mapped to an oEmbed property.

Proposed resolution

Instead, we should be looking for an array value in the $field_map array. The check should be something like:

  if (!in_array('name', $field_map)) {

Remaining tasks

  • Update _media_library_configure_form_display()
  • Tests, to include adding a new media type using the remote_video source plugin

User interface changes

None.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andrewmacpherson created an issue. See original summary.

andrewmacpherson’s picture

Status: Active » Needs review
FileSize
618 bytes

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Kristen Pol’s picture

Assigned: Unassigned » Kristen Pol

Assigning to myself for review.

Kristen Pol’s picture

Assigned: Kristen Pol » Unassigned
Issue tags: +Needs tests, +Needs manual testing

Thanks for the patch.

1) Changes seem fine.

2) Patch applies to 8.9, 9.0, and 9.1:

[mac:kristen:drupal-8.9.x-dev]$ patch -p1 < 3122051-2.patch 
patching file core/modules/media_library/media_library.module
[mac:kristen:drupal-9.0.x-dev]$ patch -p1 < 3122051-2.patch 
patching file core/modules/media_library/media_library.module
Hunk #1 succeeded at 403 (offset -1 lines).
[mac:kristen:drupal-9.1.x-dev]$ patch -p1 < 3122051-2.patch 
patching file core/modules/media_library/media_library.module
Hunk #1 succeeded at 403 (offset -1 lines).

3) Searched for empty($field_map['x']) in the media library module and didn't see any other references.

4) Still needs tests so moving back to "Needs work".

5) This also needs to be manually tested.

Kristen Pol’s picture

I tested with and without the patch as follows and am having trouble reproducing the issue. I don't fully follow the issue summary. Please let me know how to update these steps.

1) Enabled Media and Media Library modules

2) Go to: /admin/structure/media

3) Click "Add meta type" button

4) Fill in form including choosing "Media source" of "Remote video" and click "Save" button

5) Go to "Manage form display" for that Media type

6) Click "Media library" link

7) Note position "Name" field

8) Go to "Manage display" for that Media type

9) Click "Media library" link

10) Note position "Name" field

When I tested with and without the patch, the "Name" field showed up in the same place with and without the patch for "Manage form display" and "Manage display".

andrewmacpherson’s picture

@kristen - here's some clarification about the steps you took in #6.

Firstly, ignore steps 8 to 10. This is only about the entity form display.

Next, you've missed the crucial step which makes the bug happen. In the issue summary, the important bit is...

When the name field is mapped in media source plugin's field mapping

The point of the media type "field mapping" is to allow the media entity name field to be filled in automatically. For the pre-configured local file media types in Standard profile, the media entity name is auto-populated using the filename. For the Remote Video media type, the remote oEmbed resource title is used as the media entity name (for example, the title of a YouTube video). Study the pre-configured media types from Standard profile to see how it is used.

So your step 4 has missed that configuration step. It should be expanded like this:

4.1) Fill in form including choosing "Media source" of "Remote video",
4.2) In the "field mapping" fieldset, edit the "Resource title", and choose the "Name" option. The point of this step is to say "I want to use the YouTube video title as the name of the media entity".
4.3) click "Save" button

The extra step 4.2 is the one which will let us see the bug. If you are adding a new Audio, Document, Image, or Video media type, and map the Name field in the "field mapping" fieldset, then the Name field will be hidden from the media library form display. When the "add media type" form is submitted, it checks to see if you have mapped the name field. If so, it automatically hides it from the media library form display.

But if you add a new Remote Video media type, and map the Name field in the "field mapping" fieldset, then the Name field will NOT automatically be hidden from the media library form display. That's the bug. The "add media type" form submission handlers are supposed to hide it from the media library form display, but the logic is wrong and it doesn't work.

So you'll see the outcome in step 7: is the "name" field hidden on the media library form display?

To see why this bug matters, you need to try adding new remote video media via the media library dialog widget in a media reference field. If the name field is mapped to use the YouTube title as the media name, then you don't need to see the name field in the media library dialog widget. You just give it the URL, and you never see the name field. But this bug breaks that for any new remote video media types you create.

Kristen Pol’s picture

Issue tags: +Bug Smash Initiative

@andrewmacpherson Thank you for the thorough explanation. I'll try to test it again soon.

tanubansal’s picture

I have tested with the above mentioned test steps. Bug is still there.
Can someone please provide patch for 9.1

andrewmacpherson’s picture

I queued a test for patch #2 against 9.1.x

Kristen mentioned in #5 that it applies successfully to 9.1.x

Kristen Pol’s picture

Issue tags: +Needs manual testing

Tests for 9.1.x passed but #9 says that manual testing didn't pass. Probably worth another round of that to be certain so tagging.

phenaproxima’s picture

Assigned: Unassigned » phenaproxima

After thoroughly reading through the issue summary and @andrewmacpherson's excellent clarification in #7, I see this bug and what causes it. Very insidious. Assigning to myself for tests.

phenaproxima’s picture

Lucky number 13! Turns out Media Library already had extensive test coverage of this functionality, so I just added an additional test case and now we are covered. :)

Also added strict type checking to the in_array() call and removed an unnecessary local variable.

I'll leave the "needs manual testing" tag on this issue, even though we now have automated test coverage. But we should probably remove the tag when someone manually confirms that this is fixed, and marks this issue RTBC.

Kristen Pol’s picture

Thanks for the update.

1) Manually tested as follows. See screenshots.

1.1) Enabled Media and Media Library modules

1.2) Go to: /admin/structure/media

1.3) Click "Add meta type" button

1.4) Remote video steps from #7
1.4.1) Fill in form including choosing "Media source" of "Remote video"
1.4.2) In the "field mapping" fieldset, edit the "Resource title", and choose the "Name" option. The point of this step is to say "I want to use the YouTube video title as the name of the media entity"
1.4.3) Click "Save" button

1.5) Go to "Manage form display" for that Media type

1.6) Click "Media library" link

1.7) Note position of "Name" field

2) Patch applies cleanly.

3) Waiting on tests.

4) I follow most of the test but not the last couple lines. And this is funny: pinto_bean. :)

+    $this->assertFormDisplay($type_id, FALSE, FALSE);
+    $this->assertViewDisplay($type_id, 'medium');

Without patch:

With patch:

The last submitted patch, 13: 3122051-13-FAIL.patch, failed testing. View results

Kristen Pol’s picture

Status: Needs review » Reviewed & tested by the community

Marking RTBC based on the above and discussion with @phenaproxima. Thanks!

  • larowlan committed bbb2de8 on 9.1.x
    Issue #3122051 by phenaproxima, andrewmacpherson, Kristen Pol: Name...

  • larowlan committed 232cd66 on 9.0.x
    Issue #3122051 by phenaproxima, andrewmacpherson, Kristen Pol: Name...
larowlan’s picture

Title: Name field is always shown on media library form display when adding a new remote video media type » [backport] Name field is always shown on media library form display when adding a new remote video media type
Version: 9.1.x-dev » 8.9.x-dev

Committed bbb2de8 and pushed to 9.1.x. Thanks!

c/p to 9.0.x

Waiting on a green test run for 8.9.x

  • larowlan committed 039c9c6 on 8.9.x
    Issue #3122051 by phenaproxima, andrewmacpherson, Kristen Pol: Name...
larowlan’s picture

Title: [backport] Name field is always shown on media library form display when adding a new remote video media type » Name field is always shown on media library form display when adding a new remote video media type
Status: Reviewed & tested by the community » Fixed

As this is a bug and there it little chance of disruption - backported to 8.9.x

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

dianacastillo’s picture

shouldnt this module address this issue ?https://www.drupal.org/project/media_name