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.
Comment | File | Size | Author |
---|---|---|---|
#13 | 3122051-13-PASS.patch | 2.01 KB | phenaproxima |
#13 | 3122051-13-FAIL.patch | 1.35 KB | phenaproxima |
#2 | 3122051-2.patch | 618 bytes | andrewmacpherson |
Comments
Comment #2
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedComment #4
Kristen PolAssigning to myself for review.
Comment #5
Kristen PolThanks for the patch.
1) Changes seem fine.
2) Patch applies to 8.9, 9.0, and 9.1:
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.
Comment #6
Kristen PolI 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".
Comment #7
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commented@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...
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.
Comment #8
Kristen Pol@andrewmacpherson Thank you for the thorough explanation. I'll try to test it again soon.
Comment #9
tanubansal CreditAttribution: tanubansal at Salsa Digital commentedI have tested with the above mentioned test steps. Bug is still there.
Can someone please provide patch for 9.1
Comment #10
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedI queued a test for patch #2 against 9.1.x
Kristen mentioned in #5 that it applies successfully to 9.1.x
Comment #11
Kristen PolTests 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.
Comment #12
phenaproximaAfter 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.
Comment #13
phenaproximaLucky 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.
Comment #14
Kristen PolThanks 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
. :)Without patch:
With patch:
Comment #16
Kristen PolMarking RTBC based on the above and discussion with @phenaproxima. Thanks!
Comment #19
larowlanCommitted bbb2de8 and pushed to 9.1.x. Thanks!
c/p to 9.0.x
Waiting on a green test run for 8.9.x
Comment #21
larowlanAs this is a bug and there it little chance of disruption - backported to 8.9.x
Comment #23
dianacastillo CreditAttribution: dianacastillo as a volunteer and commentedshouldnt this module address this issue ?https://www.drupal.org/project/media_name