Support for media:content was added in #3049802: Add support for Media RSS.

The text content (media:description) is not included however. I propose adding Media description as a new mapping source. This would be particularly useful for mapping image media alt text.

Initial patch forthcoming.

CommentFileSizeAuthor
#7 add_media_description-3258997-7.patch4.23 KBjastraat
#6 add_media_description-3258997-6.patch2.04 KBjastraat
#2 add_media_description-3258997-2.patch2.04 KBjastraat

Comments

jastraat created an issue. See original summary.

jastraat’s picture

jastraat commented
Status: Active » Needs review
StatusFileSize
new add_media_description-3258997-2.patch2.04 KB

This patch adds the textContent of the media item from the feed as a mapping source. Technically this could be added for the media thumbnail as well. Alternatively, the code could load the media:description element directly and pull the textContent from that.

The tests for MediaRSS should probably be altered to include a media:description element within the media:content element as well.

e.g.

<media:content url="https://url.com" medium="image" type="image/jpeg" width="390" height="260">
    <media:description type="plain">Example description text for this media item.</media:description>
</media:content>
kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Status: Needs review » Needs work
Issue tags: +ContributionWeekend2022

Thanks for the issue and patch. Here is some feedback when reviewing the code:

  1. +++ b/src/Feeds/Parser/SyndicationParser.php
@@ -191,6 +192,10 @@ class SyndicationParser extends PluginBase implements ParserInterface {
         'description' => $this->t('Available if the feed supports the Media RSS specification. Can contain audio, video or other media.'),
       ],
+      'mediarss_description' => [
+        'label' => $this->t('Media description'),
+        'description' => $this->t('Available if the feed supports the Media RSS specification. Can contain audio, video or other media.'),

    Right now the description for mediarss_description is the same as the description for mediarss_content but it should be unique for the field, e.g.

    Available if the feed supports the Media RSS specification. Text that describes the media object. 

  2. +++ b/src/Laminas/Extension/Mediarss/Entry.php
@@ -48,7 +48,14 @@ class Entry extends AbstractEntry {
+        'description' => '',
       ];
+
+      $description = $media->textContent;
+      if (!empty($description)) {
+        $description = trim(str_replace(["\r\n", "\n", "\r"], '', $description));
+        $this->data[$media_key]['description'] = $description;
+      }

    1) Where is textContent coming from? I see a getDescription for the MediaType entity but not in Media entity.

    2) I'm not sure the description should be cleaned up here. Shouldn't it be import as-is? If so, I propose simplifying this as:

       'description' => !empty($media->textContent) ? $media->textContent : '',

Now I'm wondering if there is a description in media by default... have to go check :)

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

Okay, I see my confusion... was thinking about the media entity but this is the media RSS:

https://www.rssboard.org/media-rss#media-description

but still not sure where the textContent is coming from.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Assigned: jastraat » Unassigned
Issue tags: +Needs tests

Also, this does need tests to be updated as mentioned before, so tagging.

jastraat’s picture

jastraat commented
StatusFileSize
new add_media_description-3258997-6.patch2.04 KB

Right now the description for mediarss_description is the same as the description for mediarss_content but it should be unique for the field

This is a great suggestion. I was focused on getting a working patch, but agree that a more specific description would be better. Part of the reason I debated what to say is that in the code I am not technically pulling media:description. Instead I'm getting the textContent of the media:content node.

https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent

This is why I was doing the cleanup; the textContent is not just the text of the description but is essentially everything nested within the media:content entity. In the case of the feeds we are pulling, this is typically media:description and media:credit.

Another approach would be to call getMediaElement a second time on media:description specifically, and perhaps that is a cleaner approach.

Attaching a new version of the patch with an updated description as suggested along with the alternative approach.

This does still need the updated tests -

jastraat’s picture

jastraat commented
Status: Needs work » Needs review
StatusFileSize
new add_media_description-3258997-7.patch4.23 KB

Ok, attaching a version with an attempt to adjust the tests to account for the new element.

  • MegaChriz committed 6f1c3b2 on 8.x-3.x authored by jastraat 
    Issue #3258997 by jastraat, Kristen Pol, MegaChriz: Added media...
megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs review » Fixed
Issue tags: -Needs tests

Thanks for the patch! I fixed the code style issues. Because I was curious what happens if a RSS feed contains a <media:description> element but not a <media:content> element, I added a sixth entry to media-rss.rss2. It turns out that in that case "mediarss_description" is not set. Not sure if that should be the expected behavior, so I left out an assertion for that.

Should "mediarss_description" be able to exist without "mediarss_content"? Or would that be considered an "error" in the RSS feed? Having a <media:description> without <media:content> in a RSS feed item makes no sense?

Anyway, committed #7 with my few additions. Feel free to reopen if there is a case where "mediarss_description" should be able to exist without "mediarss_content".

Thanks @Kristen Pol too for doing the code review.

jastraat’s picture

jastraat commented

My assumption was that media:description would not make sense without media:content, so that was how I wrote the patch initially.

Thanks!

Status: Fixed » Closed (fixed)

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