Problem/Motivation

For entities that have a media field, it would be useful if Feeds could accept an url as source and then automatically create a media entity from it.

Right now, when you have an entity with a media field, the media field does appear on the target list on the mapping page, but you can only import media items for them that already exist on the system by referencing them by name, ID or UUID.

The workaround that exists for this is to create a separate feed type for importing media entities and after the media is imported, import the content with the media field on them, but it does require the source to specify a unique name, ID or UUID in order to select the right media entity.

Steps to reproduce

  1. Install Node, Media, Feeds.
  2. Create a content type.
  3. Add a media field to the content type (for example called "field_media").
  4. Create a feed type.
  5. On the mappings page, select the media field from the target list.

Mapping for media can be configured, but you cannot import new media this way.

Proposed resolution

Create a new FeedsTarget plugin specific for Media items. Make sure that when the source provides a url to a file, a media entity with that file gets created. And a reference to the media item is saved on the media field for the entity that is currently being imported.

Remaining tasks

  • Write a test that imports a media image on a node. Assert that the media entity gets created and attached to the node.
  • Write a test that imports a media document on a node.
  • Write a test that imports a media audio file on a node.
  • Write a test that imports a media video file on a node.
  • Write a test that imports a media remote video on a node.
  • Make the import work without configuring language on the target (see #108).
  • Get the tests from MediaTest passing.
  • When importing media images via a media field on a node, make sure that the image is visible on the media edit form (see #112).
  • Make sure that existing feed types with mapping to a media field are updated after applying the changes (see #108).
  • Make code pass cspell, phpcs and phpstan (phpstan next minor may be ignored for now).
  • Add support for media reference fields that allow more than one media type (followup issue?)
  • Add test coverage for referencing existing media (followup issue?)
  • Add a test for importing multilingual media (followup issue)
  • Add support for importing remote videos (followup issue)

For obtaining sample files for the tests to be written, it would be a good idea to check how the core Media module tests each media type.

User interface changes

Possibily more options appear for media targets.

Introduced terminology

API changes

Data model changes

Original report by kumkum29

Hello,

I want to import datas from a rss feed to generate medias (facebook).
But, I don't see the "media target" in the settings of my feed type.
Do you think add this target in the futur?

Thanks.

CommentFileSizeAuthor
#127 feeds-combined-media-fix.patch18.73 KBkopfduenger
#105 2928904-feeds-media-target-media-name.patch17.01 KBgiuseppe87
#104 2928904-feeds-media-target-media-name.patch16.88 KBgiuseppe87
#86 add_a_mapping_target_to_media_field-2928904-86.patch9.86 KBamanire
#86 add_a_mapping_target_to_media_field-2928904-86.patch9.86 KBamanire
#84 add_a_mapping_target_to_media_field-2928904-84.patch1.9 KBamanire
#80 add_a_mapping_target_to_media_field-2928904-80.patch9.84 KBroydench
#75 interdiff-58-74.txt1.02 KBbgilhome
#75 2928904-74.patch9.47 KBbgilhome
#73 getTitle error.png50.5 KBbdimaggio
#58 interdiff-2928904-55-58.txt2.58 KBmarthinal
#58 2928904-58.patch9.48 KBmarthinal
#55 feeds-media-image-2928904-54.patch9.51 KBmarthinal
#53 feeds-media-image-2928904-53.patch12.86 KBmattjones86
#50 feeds-media-image-2928904.patch12.86 KBalemadlei
#49 feeds-media-image-2928904-49.patch7.37 KBsocialnicheguru
#47 interdiff-2928904-39-47.txt1005 bytesvselivanov
#47 feeds-media-image-2928904-47.patch7.37 KBvselivanov
#45 interdiff-2928904-39-45.txt627 bytesvselivanov
#45 feeds-media-image-2928904-45.patch7.41 KBvselivanov
#39 interdiff-2928904-35-39.txt746 bytesgngn
#39 feeds-media-image-2928904-39.patch7.32 KBgngn
#35 feeds-media-image-2928904-35.patch7.3 KBmarthinal
#35 interdiff-2928904-33-35.txt2.07 KBmarthinal
#33 interdiff_32-33.txt657 bytestostinni
#33 feeds-media-image-2928904-33.patch6.96 KBtostinni
#32 Screenshot 2020-10-21 at 09.13.15.png160.74 KBmarthinal
#32 feeds-media-image-2928904-32.patch6.82 KBmarthinal
#32 interdiff-2928904-31-32.txt499 bytesmarthinal
#31 interdiff-2928904-27-30.txt2.37 KBmarthinal
#31 feeds-media-image-2928904-30.patch6.8 KBmarthinal
#27 feeds-media-image-2928904-27.patch6.33 KBmarthinal
#24 2928904-24-videoembed-mapping.patch806 bytesdarrenwh
#19 2928904-19-videoembed-mapping.patch585 bytesTimelessDomain
#13 feeds-video_embed_field-support.patch421 bytesjosh waihi
#11 VideoEmbed.php_.patch585 bytesopsdemon
#6 feeds-media-processor-2928904-6.patch1.08 KBthomasfowles

Issue fork feeds-2928904

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

kumkum29 created an issue. See original summary.

megachriz’s picture

Category: Support request » Feature request

Is the media field in Drupal core? If so, it could be added in the future. The focus is first on other things, though. See #2917928: Plan for 8.x-3.0-alpha1 release.

megachriz’s picture

Title: Rss + target media » Add a mapping target to media field
thomasfowles’s picture

I am running a documentation site, which involves frequently updating files from our local machine. Most files are documents of some sort, and the site is configured to upload these as media objects and not content nodes.

Feeds would be an excellent way of keeping this collection up to date - however, media objects do not seem to be supported as a target.

Is this an easy fix for a very novice module developer? If so, with some pointers, I could give it a crack.
Looking at the module files - I think this is likely out of my league...

megachriz’s picture

Closed #2960274: Add a video embed mapping target and #2973167: Does feeds support media:description, media:title etc? for youtube as duplicates.

Useful info from there:

To get the media reference field supported as target, code a new FeedsTarget plugin and add it to feeds/src/Feeds/Target. It could be that media fields work similar to file fields. In that case you could use the target plugin "image" as example. If not, perhaps the target plugin "link" would be good to use as starting point. Either way, you'll need to find out how the properties of a media field are called and be sure to convert the values for each property in a format that the media field expects in the prepareValue() method of the target plugin.

thomasfowles’s picture

StatusFileSize
new1.08 KB

I've had a little play, and please be kind - this is my first attempt at a contribution to Drupal!

The problem does not seem to be requiring a new target, but requiring a new processor.

The attached patch includes a media processor.

The media module is not enabled by default in core. The form at admin/structure/feeds still allows the media processor to be selected when the media module is disabled - which will need correcting.

Perhaps using:

$moduleHandler = \Drupal::service('module_handler');
if ($moduleHandler->moduleExists('media')){

// Processor

}
thomasfowles’s picture

Status: Active » Needs work
tedwyer’s picture

@thomasfowles - you can enable the core media in 8.5. Working with your patch. Thanks for the effort. No result yet.

megachriz’s picture

From #2973170-3: Importing new files fails when "Autocreate entity" option is turned on.:

After importing ~500 nodes, the images do not show in the media library. If you remove the image manually and add it back, it includes it in the library.

From #2973170-5: Importing new files fails when "Autocreate entity" option is turned on.:

Ah, it looks like the media item needs a Media Name and the "save to library" option checked before it shows.

This sounds like that a media image field target (if that is what it is), needs a submapping target for "Media Name" and an option for "save to library".

@thomasfowles
Thanks for your contribution. With the addition of the generic content entity processor in #2958150: Create custom ECK entities via Feeds, we no longer need a separate processor for Media entities (unless Media entities would require special handling). I do think we need media as a target, for example to get images saved in the Media library. But I could be completely wrong, cause I haven't even installed the Media module for D8 yet. This idea is purely based on my experience with the Media module in D7.

kobb’s picture

FYI: It looks like this is being put into core in 8.6.

#2835825: Add d8 media migration

opsdemon’s picture

StatusFileSize
new585 bytes

For anybody wishing to use the video_embed_field module with Feeds, here is a new patch that works for me.

With the patch, fields of type "Video Embed" now appear in the "Target" dropdown in Feeds Mappings.
You can then create a mapping for a video embed field in the content type to a field in the CSV file containing a video URL.
Subsequently, on import, the video URL in the CSV will be used to the populate the corresponding video embed field in the content type.

Hope this helps!

hongpong’s picture

Thanks opsdemon. Is it possible to use oEmbed in a text field, in such a way that a YouTube channel or playlist can automatically be fed in to create new nodes with the videos simply in the body field?

josh waihi’s picture

StatusFileSize
new421 bytes

The Video Embed Field has only one key called "value" making it identical to the StringTarget class. So rather than adding another class, here is an alternate patch to #11 which just expands the StringTarget annotation to include video_embed_field.

@HongPong, I ingested a youTube channel using this patch. I simply mapped the Item URL to the Video Embed Field.

megachriz’s picture

Issue tags: +Needs tests

@Josh Waihi
I haven't checked the details of the video embed field, but I can imagine that some extra handling could be required when preparing the value. So in that case a separate class would be better.

New targets do need a kernel test to ensure values (in this case video urls) can be imported with success.

drupalnesia’s picture

Version: 8.x-3.x-dev » 8.x-3.0-alpha3
Status: Needs work » Reviewed & tested by the community

Path #11 working fine, successful import 2,000 node with Video Embed Field, thanks!

The last submitted patch, 11: VideoEmbed.php_.patch, failed testing. View results

drupalnesia’s picture

Path failed to submit because of underscore on VideoEmbed.php_.patch ? Should be VideoEmbed.php.patch ? If so, how to rename?

hongpong’s picture

drupalisme you should use the format for naming the patch like 2928904-18-videoembed-mapping.patch where 18 is the comment number assigned to it and 2928904 is this issue. You could attach a reroll of these changes and I believe it would work.

TimelessDomain’s picture

2928904-19-videoembed-mapping.patch works well, thanks! same as #11 but renamed accordingly

OCTOGONE.dev’s picture

If you are wondering where to apply that patch, you have to copy the file VideoEmbed.php to the folder feeds/src/Feeds/Target.

jjwfcd’s picture

confirm #20

just download the patch #19 in feeds/src/Feeds/Target and rename it to VideoEmbed.php

and

drush cr

then the videoembed field shows up on the target list.

thanks guys.

megachriz’s picture

Status: Reviewed & tested by the community » Needs work

Support for the Video Embed Field module should go into that module. Feeds only aims to support field types provided by Drupal Core. This issue was originally about support for the Media module, so focus here on that.

Also, closed #3071687: Fix mapping from public:// image URL to Media Image object as a duplicate. Images from that issue:

TimelessDomain’s picture

All future work for Video Embed Field feed mappers should go into this issue https://www.drupal.org/project/video_embed_field/issues/3056385

darrenwh’s picture

StatusFileSize
new806 bytes

Rerolled patch 19, did not apply

mrpauldriver’s picture

This issue seems to have strayed off-topic and is not too helpful.

Very keen to know what is the best advice for importing D7 files or images into a D8 media field?

Given that we are now on D9 it would be good to have this resolved.

jwilson3’s picture

I started to have a look at this but I quickly got out of my depth, once I realized the media target handler needs to:

* handle multiple entity bundle types, each of which has its own primary field (eg field_media_image or field_media_file or field_media_video) all of which are themselves different field types (image reference, file reference, string field.
* And then apparently I'm seeing we may also need to setup a "thumbnail" field on some of the bundle types for it to save correctly.

Basically, I'm lost at the Media::prepareTarget() step, because all the other example Target types add a property target_id which is easy enough to copy and paste, but for the Media entity there is an intermediate entity reference along the lines of field_media_image/target_id. Fwiw, the forward slash to separate sub-fields is the route being taken by the core D7 -> D8 Media Migration effort on #2835825: Add d8 media migration. I have no idea if this would work here or not.

This is as far as I got, maybe someone can run with this:

<?php

namespace Drupal\feeds\Feeds\Target;

use Drupal\Core\Field\FieldDefinitionInterface;
use Drupal\feeds\FieldTargetDefinition;

/**
 * Defines a media entity mapper.
 *
 * @FeedsTarget(
 *   id = "media",
 *   field_types = {"media"}
 * )
 */
class Media extends EntityReference {

  /**
   * {@inheritdoc}
   */
  protected static function prepareTarget(FieldDefinitionInterface $field_definition) {
    return FieldTargetDefinition::createFromFieldDefinition($field_definition)
      // @todo dont hardcode this because different media bundles have different file names and file types
      ->addProperty('field_media_image/target_id')
      ->addProperty('name');
  }

  /**
   * {@inheritdoc}
   */
  protected function prepareValue($delta, array &$values) {

    // @todo request a media ID for the specified URI.

  }

  /**
   * {@inheritdoc}
   */
  protected function getEntityType() {
    return 'media';
  }

  /**
   * Returns a media id given a url.
   *
   * @param string $value
   *   A URL to a media object.
   *
   * @return int
   *   The media id.
   *
   * @throws \Drupal\feeds\Exception\EmptyFeedException
   *   In case an empty url is given.
   */
  protected function getMedia($value) {
    if (empty($value)) {
      // No file.
      throw new EmptyFeedException('The given url is empty.');
    }

    // Perform a lookup against the value using the configured reference method.
    if (FALSE !== ($mid = $this->findEntity($value, $this->configuration['reference_by']))) {
      return $mid;
    }

    // @todo implement logic to obtain the Media id, based indirectly on
    // the provided URI. This needs to work for the various "source_field"
    // definitions (field defined in: media.type.X.yml:source_configuration:source_field).
    // Examples include:
    //  * field_media_image/target_id (image entity reference)
    //  * field_media_file/target_id (file entity reference)
    //  * field_media_video_file/target_id (file entity reference)
    //  * field_media_audio_file/target_id (file entity reference)
    //  * field_media_document/target_id (file entity reference)
    //  * field_media_oembed_video (string field for oEmbed URL)

    // needs to support both local and remote URI's so guzzle is probably the
    // way to do this for file entity references.
    //
    // Probably should follow example code from
    //    Drupal\feeds\Feeds\Target\File::getFile()

  }

}
marthinal’s picture

StatusFileSize
new6.33 KB

I've created a plugin for images.

/**
 * Defines a file field mapper.
 *
 * @FeedsTarget(
 *   id = "media_image",
 *   field_types = {"image"}
 * )
 */
  1. The field type is "image".
  2. We create the Media entity from ::setTarget()
  3. We add the plugin from FieldTargetBase::targets()
marthinal’s picture

Status: Needs work » Needs review
megachriz’s picture

@marthinal
Thanks for contributing!

+++ b/src/Plugin/Type/Target/FieldTargetBase.php
@@ -52,8 +52,17 @@ abstract class FieldTargetBase extends TargetBase implements ConfigurableTargetI
-          $target->setPluginId($definition['id']);
-          $targets[$id] = $target;
+          // Set Plugin for Media.
+          $fieldSettings = $field_definition->getFieldStorageDefinition()->getSettings();
+          if (isset($fieldSettings['target_type']) && $fieldSettings['target_type'] == 'media') {
+            $type = array_key_first($field_definition->getSettings()['handler_settings']['target_bundles']);
+            $target->setPluginId("media_$type");
+            $targets[$id] = $target;
+          }
+          else {
+            $target->setPluginId($definition['id']);
+            $targets[$id] = $target;
+          }

Based on the changes you made in FieldTargetBase, it looks like we need to change something at the API level, so that exceptions for specific targets don't have to be made there.

marthinal’s picture

@MegaChriz yep, I agree. IMO it makes sense to add ::targets() to MediaImage plugin. But we are executing that function multiple times and I had different issues with that. It probably makes sense to add the pluginId if that value does not exist in $targets. I'm working on other things and didn't have time to continue with that tbh.

marthinal’s picture

If the fid already exists then can use the same Media entity

marthinal’s picture

I'm using JMESPath to import multiple images into the same Media field. You can do that with [image,image2] for example. With this small change I see the expected result.

mango

tostinni’s picture

Status: Needs review » Needs work
StatusFileSize
new6.96 KB
new657 bytes

I used this patch to import a RSS feed from a Drupal site and the images had an itok query paramenter image.jpg?itok=f7JbTrD1 which resulted in an error of invalid extension.

The quick & dirty fix was to remove the query parameter from the URL, but I think this would need more thoughts.

marthinal’s picture

Version: 8.x-3.0-alpha3 » 8.x-3.x-dev
marthinal’s picture

Status: Needs work » Needs review
StatusFileSize
new2.07 KB
new7.3 KB

In our use case, we import images from different sources into the same field. In some cases we have duplicates. The same id has been added multiple times to the same field. This is correct and we are not duplicating the Media entity but it is weird to see the same image attached multiple times. I've implemented some code to fix that.

bkosborne’s picture

Issue summary: View changes

Hmm I'm confused. Is the proposed plugin in the latest patch assuming that you have a feed that imports items directly as media items?

I'm looking for a way to import image URLs into an image media reference field on some parent entity. For example, a News Article content type that has a media reference field on it for an image. I want to import that image from a source RSS feed that has the image defined in an enclosure element.

I think for that, what's needed is an extension of the existing EntityReference target plugin.

megachriz’s picture

@bkosborne
The FeedsTarget plugin in the patch does extend the existing EntityReference target: MediaImage extends Image, which extends File, which extends EntityReference.

@marthinal
It would be cool if you could add tests for this feature, if you think your approach is generic enough. Glancing over the code, I think that is the case.

finex’s picture

Hi, on my simple case (single image per post) the patch #35 worked very well. Thanks!

gngn’s picture

StatusFileSize
new7.32 KB
new746 bytes

#35 worked for me.

One small thing: in MediaImage::setTarget line

            $mid = array_shift(array_values($mid));

generates a PHP Notice: Only variables should be passed by reference.

I think we should change that to:

            $mids = array_values($mid);
            $mid = array_shift($mids);

New patch and interdiff attached.

carolpettirossi’s picture

I'm trying to use the patch provided on #39. However, I'm getting an error:

cURL error 52: Empty reply from server (see https://curl.haxx.se/libcurl/c/libcurl-errors.html)

I've added a new source to the mappings where the Target is field_logo which is a reference to a Media Image. I've tried the setting File ID and Filename, but both settings have the same curl error result.

My CSV import file is something like:

Name,Logo
A,https://www.logodesign.net/logo/abstract-cuboid-building-4519ld.png 

Is anyone able to help me using this patch successfully?

Thanks :)

EducoWebDesign’s picture

This is awesome, thanks so much! The patch in #39 worked great to import media entities referenced in my article nodes. I wanted to use it to import media on an ECK entity, and getTitle() was throwing an undefined method error, so I had to change line 110 from
$media->setName($entity->getTitle())->setPublished(TRUE)->save();
to
$media->setName($entity->label())->setPublished(TRUE)->save();

carolpettirossi, did you ever figure out your issue?

carolpettirossi’s picture

Hi EducoWebDesign,

Actually not. We decided to got with another approach since uploading/creating the media via the CSV wasn't working.

vlooi vlerke’s picture

I have installed the patch in #39

But it only shows the image file map.
How do I get the image alt and caption (extra fields in the image file) to show up as feed mappers?
Thanks

vlooi vlerke’s picture

@carolpettirossi

I can import images from a csv file.
I copied all my images to my public files folder inside a folder called "import"
Then using feed tamper I rewrite the output to look like this:

http://localhost/my_drupal_site/public_html/sites/default/files/import/[logo]

I imported over 1000 images with no problems.
The next obstacle is to map the Alt and Caption fields

Edit:
I also import multiple images into a single media field by rewriting the to look like this:

http://localhost/my_drupal_site/public_html/sites/default/files/import/[logo1],http://localhost/my_drupal_site/public_html/sites/default/files/import/[logo2],http://localhost/my_drupal_site/public_html/sites/default/files/import/[logo3]

I then add "explode" tamper feature to make the feed input an array. This enables you to import multiple images

vselivanov’s picture

StatusFileSize
new7.41 KB
new627 bytes

Hi!

I have XML parser from feed_ex module and it works with MediaImage target plugin.
Drupal 8.9.16
Feeds 3.0.0-alpha10 (also tested with 8.x-3.x dev version) with patch #39 or my attached patch with ALT tag.

I updated patch #39 to add ALT tag to the image file. Alt is same as Media name - title XML element.

I had some problems and spent a several hours to extract image url from the XML media:content - url attribute. But now it works. Hope my setup will help someone.

field_thumb_image - Image media field in the target node.
I download XML from external url - Google calendar feed.

XML structure:

<rss xmlns:dc="http://purl.org/dc/elements/1.1/" xmlns:geo="http://www.w3.org/2003/01/geo/wgs84_pos#" xmlns:media="http://search.yahoo.com/mrss/" xmlns:xCal="urn:ietf:params:xml:ns:xcal" version="2.0">
  <channel>
    <title>Feed example title</title>
    <link>https://example.feed/url</link>
    <description>Feed description</description>
    <lastBuildDate>Wed, 02 Jun 2021 16:00:23 +0000</lastBuildDate>
    <ttl>120</ttl>
    <language>en-us</language>
    <generator>Localist</generator>
    <item>
      <title>Feed item title 1</title>
      <description>Lorem ipsum 1 dolor sit amet, consectetur adipiscing elit. Morbi</description>
      <media:content medium="image" url="https://example.com/photos/link-to-image/image-1.jpg"/>
    </item>
    <item>
      <title>Feed item title 2</title>
      <description>Lorem ipsum 2 dolor sit amet, consectetur adipiscing elit</description>
      <media:content medium="image" url="https://example.com/photos/link-to-image/image-2.jpg"/>
    </item>
  </channel>
</rss>

Main part of the YML config:

parser: xml
parser_configuration:
  context:
    value: //item
  sources:
    media_content_url:
      label: 'media:content/@url'
      value: 'media:content/@url'
custom_sources:
  media_content_url:
    label: 'media:content/@url'
    value: 'media:content/@url'
    machine_name: media_content_url
mappings:
  -
    target: field_thumb_image
    map:
      target_id: media_content_url
    settings:
      language: ''
      reference_by: filename
      existing: '1'
      autocreate: 0

Status: Needs review » Needs work

The last submitted patch, 45: feeds-media-image-2928904-45.patch, failed testing. View results

vselivanov’s picture

StatusFileSize
new7.37 KB
new1005 bytes

Small improvement: we don't need to rename media if it was reused. Just ensure that it's published.

socialnicheguru’s picture

this is not applying to new dev version of feeds

socialnicheguru’s picture

StatusFileSize
new7.37 KB

I just rerolled for it to apply. No other changes were made for the dev version

alemadlei’s picture

StatusFileSize
new12.86 KB

The following also adds a mapping target for media files. It works with 3.0-alpha10 as this is the version we are using on our client's project.

Basically, the image one was copied and modified to also work for media files.

neograph734’s picture

Status: Needs work » Needs review

Switching NR for the latest patches.

The last submitted patch, 49: feeds-media-image-2928904-49.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mattjones86’s picture

StatusFileSize
new12.86 KB

Re-roll of patch for latest dev

EDIT: not sure on test failures, perhaps more changed on latest dev than I thought.

Status: Needs review » Needs work

The last submitted patch, 53: feeds-media-image-2928904-53.patch, failed testing. View results

marthinal’s picture

Status: Needs work » Needs review
StatusFileSize
new9.51 KB

Hi guys

I fixed the errors and added a base class. Sorry @MegaChriz I didn't have time to work on the tests for this feature...

Let's see what the tests say.

Status: Needs review » Needs work

The last submitted patch, 55: feeds-media-image-2928904-54.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

marthinal’s picture

Okay the new error makes sense

Drupal\Tests\feeds\Functional\Feeds\Target\FileTest::test
Drupal\Component\Plugin\Exception\PluginNotFoundException: The "media" entity type does not exist.

/var/www/html/core/lib/Drupal/Core/Entity/EntityTypeManager.php:150
/var/www/html/core/lib/Drupal/Core/Entity/EntityFieldManager.php:202
/var/www/html/core/lib/Drupal/Core/Entity/EntityFieldManager.php:179
/var/www/html/core/lib/Drupal/Core/Entity/EntityFieldManager.php:330
/var/www/html/modules/contrib/feeds/src/Plugin/Type/Target/MediaTargetBase.php:55

The tests need work.

marthinal’s picture

StatusFileSize
new9.48 KB
new2.58 KB

Updating the patch

marthinal’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 58: 2928904-58.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mattjones86’s picture

Does the patch need to add a `media` module requirement to make the tests pass, or do they just need work to make sure media is installed as part of the test setup?

megachriz’s picture

@mattjones86
There are no tests in the latest patch yet. So I think that in the code there needs to be checks if the media entity type is available.

A partial code review:

  1. +++ b/src/Plugin/Type/Target/FieldTargetBase.php
    +++ b/src/Plugin/Type/Target/FieldTargetBase.php
    @@ -55,8 +55,17 @@ abstract class FieldTargetBase extends TargetBase implements ConfigurableTargetI
    
    @@ -55,8 +55,17 @@ abstract class FieldTargetBase extends TargetBase implements ConfigurableTargetI
           }
           if (in_array($field_definition->getType(), $definition['field_types'])) {
             if ($target = static::prepareTarget($field_definition)) {
    -          $target->setPluginId($definition['id']);
    -          $targets[$id] = $target;
    +          // Set Plugin for Media.
    +          $fieldSettings = $field_definition->getFieldStorageDefinition()->getSettings();
    +          if (isset($fieldSettings['target_type']) && $fieldSettings['target_type'] == 'media') {
    +            $type = array_key_first($field_definition->getSettings()['handler_settings']['target_bundles']);
    +            $target->setPluginId("media_$type");
    +            $targets[$id] = $target;
    +          }
    

    I like to see this being moved to MediaTargetBase.

  2. +++ b/src/Plugin/Type/Target/MediaTargetBase.php
    @@ -0,0 +1,161 @@
    +    $fileSettings = $entity_field_manager->getFieldDefinitions('media', 'image')["field_{$this->pluginDefinition['id']}"]->getSettings();
    

    I think this call causes an issue when the media module is not available, so here a check should be added if the entity type "media" exists.

    Alternatively, maybe we should check if we can prevent this plugin from being instantiated when the media module is not available. Not sure if that is easy to do. I would think that somehow the targets() method could prevent that. Perhaps it is enough if MediaTargetBase::targets() bails out early if the entity type "media" does not exist.

neograph734’s picture

Hi all, thanks for the work so far, but I wonder if this approach is the one we should want.

Allthough it will work, it will result in a very inflexible solution with hardcoded media bundles and target plugins for each media type (bundle). If I have another media image bundle, for instance for product images, that I want to keep separate from the content images, I need to create yet another target plugin.

For over multiple days I have been trying to clean up and simplify the work done in the 8.x branch of media feeds (which is a 99% copy of feeds_para_mapper). But yet without succes. (The lack of releases probably indicates their efforts stranded as well.) I do however believe that to be a much more flexible solution.

Perhaps it is something to look into before people start wring tests for this approach?

b.friddy’s picture

Hello @Vlooi Vlerke

I tried to import images based on your instructions, unfortunately it's not working for myself. I assume I'm doing something wrong.

Here is my setup:

  • File type is an Entity Reference (image - file_nn_image) - media type selected is only image
  • Created import folder under sites/default as you mentioned and copied images there
  • Added Rewrite tamper plugin for the image with the replacement pattern - http://my-site.docksal/sites/default/files/import/[image]

The system imports the nodes, but the file path isn't correct (see below) and no images are imported / copied to the sites/default/files/styles/medium folder (I assume because of the invalid path)

http://my-site.docksal/sites/default/files/styles/medium/http/my-site.do...

Any help is appreciated.

Thanks

MacSaveNy’s picture

Thanks for everyone's help on this. I recently updated one of the sites I was working on to Drupal 9.3 and am now stuck with all my feeds not able to import media. After the patch I am getting the error: The "media_document" plugin does not exist for any file field.

For the image fields I am getting: Undefined index: file_extensions in Drupal\feeds\Feeds\Target\File->__construct() (line 83 of modules/feeds/src/Feeds/Target/File.php).

Either way.. I cant code myself out of this... hoping you smarter coders out there can get this media support back for 9.3

b.friddy’s picture

FYI - I found this issue (https://www.drupal.org/project/feeds/issues/3052350) which was helpful regarding my problem above. It explains in more detail how to import Media.

Thanks again Vlooi Vlerke for the instructions, it is appreciated.

Christopher Riley’s picture

I applied the patch and I have images now coming in however I am seeing the following issue:

Notice: Undefined index: display_default in Drupal\feeds\Feeds\Target\File->prepareValue() (line 131 of /home/sites/sitename/public_html/modules/contrib/feeds/src/Feeds/Target/File.php)

I am not sure if this is because on my feed the media files are actually within a enclosures field or if this is a issue overall.

Any suggestions would be appreciated.

irinaz made their first commit to this issue’s fork.

socialnicheguru’s picture

When trying to add a media field I saw no popup for feed source.

Drupal 9.2+
php 7.4

I got the error below in my logs.

/admin/structure/feeds/manage/my_feeds_csv/mapping|1||Drupal\Component\Plugin\Exception\PluginNotFoundException: The "media_audio" plugin does not exist. Valid plugin IDs for Drupal\feeds\Plugin\Type\FeedsPluginManager are: address_zone_feeds_target, address_country_feeds_target, address_feeds_target, commerce_feeds_price, commerce_feeds_physical_dimensions, commerce_feeds_physical_measurement, date_recur, timestamp, telephone, file_uri, path, file, temporary_target, email, langcode, boolean, string, media_file, media_image, integer, link, image, config_entity_reference, number, daterange, user_role, entity_reference, uri, datetime, feeds_item, password, text, fraction, geofield_feeds_target, geolocation_feeds_target, office_hours_feeds_target, weight, paragraphs in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 53 of drupal9/html/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).

I installed media from core and audio is installed.
There is a media type named audio.
I do not know where the plugin for 'media_audio' is even defined.
In core/modules/media/src/Plugin/media/Source, Image.php, Audio.php, File.php, and VideoFile.php plugins are defined. none have an id of media_xxxx (I think).
In the patch there a line to iterate through the media_$type plugins.
$target->setPluginId("media_$type");

demonde’s picture

Sorry, I am a little confused. Is the an intermediate state what is possible and what not using a certain patch?

I would like to import media images in a node feed but it looks like this is not possible yet.

Right now I am using a separate feed for media items an reference them from the node feed with the feeds uid.

Christopher Riley’s picture

Tried patch 58 on a D9.4 with PHP 8.1 and got the following when I tried to edit the mapping.

TypeError: array_key_first(): Argument #1 ($array) must be of type array, null given in array_key_first() (line 61 of modules/contrib/feeds/src/Plugin/Type/Target/FieldTargetBase.php).

Christopher Riley’s picture

I think I found the issue. I was trying to use other files types besides image. For example audio, documents, etc. Can anyone else verify that setting those will cause errors?

bdimaggio’s picture

StatusFileSize
new50.5 KB

Have other folks run into this problem with the latest patch (in #58)?

As far as I can tell getTitle()only ever existed on Node, not Media. When I changed the offending lines (both 25, as mentioned in the error, and 29 of the same file) to getName(), the feed import continued without complaint. I can certainly submit a patch to fix this, but I want to be sure that this problem isn't just user error first.

gngn’s picture

@bdimaggio you are talking about #58, yes?

#58 contains three uses of getTitle(), all on EntityInterface $entity:

  • MediaImage::createMedia(EntityInterface $entity, $target_id)
  • MediaFile::createMedia(EntityInterface $entity, $target_id)

EntityInterface does not provide a getName() either - only the usual label().
Symfony\Component\HttpFoundation\File and Drupal\media\MediaInterface both offer a getName() - so I am not sure which types we can expect in the two createMedia() functions...

bgilhome’s picture

StatusFileSize
new9.47 KB
new1.02 KB

Here's an updated patch replacing getTitle() on the media entities with core method label().

ecj’s picture

well sorry to bring bad news. this patch brings down/fails the regular image field (still needed for many galleries, etc).
Tried to look into it, but can't find the time to extensively analyse the problem.

I did come to the conclusion that some weird/added media_image mapping in src/Plugin/Type/Target/FieldTargetBase.php seems to be the cause. Reminded me of the entity_reference (media type) vs image (image type) different handlings.

Please look into this problem?
I'd love to import media + normal types together into 1 node/product.

keep up the good work

ecj’s picture

when this patch is applied, and when under mapping trying to add a mapping of an image field, following error msg:

Location	https://xxx/admin/structure/feeds/manage/field_image/mapping?_wrapper_format=drupal_ajax&_wrapper_format=drupal_ajax&ajax_form=1
Referrer	https://xxx/admin/structure/feeds/manage/field_image/mapping
Message	Drupal\Component\Plugin\Exception\PluginNotFoundException: The "media" entity type does not exist. in Drupal\Core\Entity\EntityTypeManager->getDefinition() (line 150 of /xxx/core/lib/Drupal/Core/Entity/EntityTypeManager.php).
Severity	Error

so seems to see image field as a media entity I guess

mattjones86’s picture

If you're Media entity type does not exist, then most likely you need to install the Media module?

I was getting the same thing in my comment above. It was because the media module was not enabled before running the tests.

megachriz’s picture

1) Drupal\Tests\feeds\Functional\Feeds\Target\FileTest::test
Drupal\Component\Plugin\Exception\PluginNotFoundException: The "media" entity type does not exist.

The error in this test suggests that importing files would be broken if the Media module is not installed. That's not acceptable in my opinion. I think that Feeds shouldn't require the Media module.

I think that we should look for an other solution than this:

+++ b/src/Plugin/Type/Target/FieldTargetBase.php
@@ -55,8 +55,17 @@ abstract class FieldTargetBase extends TargetBase implements ConfigurableTargetI
       if (in_array($field_definition->getType(), $definition['field_types'])) {
         if ($target = static::prepareTarget($field_definition)) {
-          $target->setPluginId($definition['id']);
-          $targets[$id] = $target;
+          // Set Plugin for Media.
+          $fieldSettings = $field_definition->getFieldStorageDefinition()->getSettings();
+          if (isset($fieldSettings['target_type']) && $fieldSettings['target_type'] == 'media') {
+            $type = array_key_first($field_definition->getSettings()['handler_settings']['target_bundles']);
+            $target->setPluginId("media_$type");
+            $targets[$id] = $target;
+          }
+          else {
+            $target->setPluginId($definition['id']);
+            $targets[$id] = $target;
+          }

In my opinion, FieldTargetBase is not the place for Media specific code.

If targets need to be conditional overridden, see how the UserRole target did that:

  /**
   * {@inheritdoc}
   */
  public static function targets(array &$targets, FeedTypeInterface $feed_type, array $definition) {
    $processor = $feed_type->getProcessor();

    if (!$processor instanceof EntityProcessorInterface) {
      return $targets;
    }

    $field_definitions = \Drupal::service('entity_field.manager')->getFieldDefinitions($processor->entityType(), $processor->bundle());

    foreach ($field_definitions as $id => $field_definition) {
      if ($field_definition->getType() == 'entity_reference' && $field_definition->getSetting('target_type') == 'user_role') {
        if ($target = static::prepareTarget($field_definition)) {
          $target->setPluginId($definition['id']);
          $targets[$id] = $target;
        }
      }
    }
  }

If a target depends on the existing of something, for example a module, see how the Book target handled that:

  /**
   * {@inheritdoc}
   */
  public static function targets(array &$targets, FeedTypeInterface $feed_type, array $definition) {
    // Don't show mapping target to book, if there is none.
    if (!\Drupal::moduleHandler()->moduleExists('book')) {
      return;
    }

    $processor = $feed_type->getProcessor();

    if (!$processor instanceof EntityProcessorInterface) {
      return $targets;
    }

    // The book module solely works with nodes.
    if ($processor->entityType() != 'node') {
      return $targets;
    }

    if ($target = static::prepareTarget()) {
      $target->setPluginId($definition['id']);
      $targets['book'] = $target;
    }
  }
roydench’s picture

Compatible with core 9.5.9 and 8.x-3.0-beta4

joaopauloscho’s picture

#80 works for us, thanks.

besek’s picture

#80 worked for me, however it saevd node title as image alt. I think it should be configurable so you can choose what values you want to save in title and alt fields of the image. Thanks

jrochate’s picture

I don't have standard media machine names, and the latest path WSODs.

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "media_base_image" plugin does not exist. Valid plugin IDs for Drupal\feeds\Plugin\Type\FeedsPluginManager are: file, datetime, number, media_file, uri, feeds_item, config_entity_reference, integer, temporary_target, daterange, email, timestamp, password, book, image, media_image, user_role, link, text, boolean, telephone, langcode, entity_reference, path, string, geofield_feeds_target, paragraphs in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 53

and also

Error: Class "Drupal\feeds\Feeds\Target\MediaFile" not found in Drupal\feeds\Entity\FeedType->getMappingTargets() (line 305 of /var/www/clients/client1/web39/web/repos/ccmar/web/modules/contrib/feeds/src/Entity/FeedType.php)

it looks like we must use standard profile installation because the fields are hardcoded.

amanire’s picture

I've updated the patch in #80 for Drupal 10 compatibility. The corresponding feed had stopped importing and was throwing the following warning:

Entity queries must explicitly set whether the query should be access checked or not. See Drupal\Core\Entity\Query\QueryInterface::accessCheck().

Oops, looks like i didn't get everything in that patch. Trying again.

amanire’s picture

amanire’s picture

OK this patch now includes the explicit accessCheck for D10 compatibility.

amanire’s picture

ed2908’s picture

On Patches
2928904-58.patch
add_a_mapping_target_to_media_field-2928904-80.patch
and
add_a_mapping_target_to_media_field-2928904-86_0.patch

I'm seeing a lot of:
"The file, 917, failed to save because the extension, 17, is invalid.
The file, 918, failed to save because the extension, 18, is invalid.
The file, 920, failed to save because the extension, 20, is invalid."

and a lot of "Image (field_media_image): This value should not be null."

My mappings are:
Feed field containing public://PATH TO IMAGE.jpeg -> Image (field_media_image)
Feed field with file id -> Image (field_media_image): File ID
Feed field with filename -> Name (name)

My file IDs from the old site are 917, 918, 220 ect.
Nowhere in the feed is 17, 18, 20. Certainly not being used as a file extension.

Am I doing something completely wrong?

isa.bel’s picture

I had some issues after updating to the latest stable version of the module, but using the patch from #84 worked for me!

jesss’s picture

Patch #86 is working for me!

djschoone’s picture

@jesss on which version did you apply? #86 on 3.0.0-beta4 gives me when visiting admin/structure/feeds/manage//mapping this error:

Error: Call to a member function getSettings() on null in Drupal\feeds\Plugin\Type\Target\MediaTargetBase->__construct() (line 58 of modules/contrib/feeds/src/Plugin/Type/Target/MediaTargetBase.php).
Drupal\feeds\Plugin\Type\Target\MediaTargetBase::create(Object, Array, 'media_image', Array) (Line: 21)
Drupal\Core\Plugin\Factory\ContainerFactory->createInstance('media_image', Array) (Line: 22)
Drupal\feeds\Plugin\Type\FeedsAnnotationFactory->createInstance('media_image', Array) (Line: 83)
Drupal\Component\Plugin\PluginManagerBase->createInstance('media_image', Array) (Line: 515)
Drupal\feeds\Entity\FeedType->getTargetPlugin(6) (Line: 375)
Drupal\feeds\Form\MappingForm->buildRow(Array, Object, Array, 6) (Line: 160)
Drupal\feeds\Form\MappingForm->buildForm(Array, Object, Object)
call_user_func_array(Array, Array) (Line: 536)
Drupal\Core\Form\FormBuilder->retrieveForm('feeds_mapping_form', Object) (Line: 283)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 73)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 627)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 53)
Asm89\Stack\Cors->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 704)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
jesss’s picture

I'm on 3.0.0-beta4. Did you clear cache after applying the patch?

nicasso’s picture

I'm on 3.0.0-beta4 too and patch #86 does indeed generate an error when visiting: /admin/structure/feeds/manage/something/mapping

A cache rebuild did not solve it unfortunately.

Error: Call to a member function getSettings() on null in Drupal\feeds\Plugin\Type\Target\MediaTargetBase->__construct() (line 58 of modules/contrib/feeds/src/Plugin/Type/Target/MediaTargetBase.php).

jidrone made their first commit to this issue’s fork.

jidrone’s picture

Status: Needs work » Needs review

Hello,

The existing patch has the following issues:

  • The media targets provided are tightly coupled to other existing targets.
  • It supports only image and file media types, so if you have a media type with a different name, it will not work.
  • It assumes the source file of the media entity types always follows the pattern field_media_[media_type_id].
  • The comments from the maintainer in #79 where not addressed.

This new MR, has the following approach:

  • As minimal as possible changes to existing targets.
  • All the code is in just one new target called Media for loose coupling.
  • Maintainer comments addressed to ensure it is only applicable when media module is enabled
  • It is only applicable when the media field has at least one media type that contains a source field of type file.
  • Detects the applicable media type depending on the file extension.

I know there are still improvements to do and it will need tests, but this could be a better starting point.

The last submitted patch, 86: add_a_mapping_target_to_media_field-2928904-86.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

megachriz’s picture

Thanks @jidrone! That looks like a big step in getting this issue done. 😃

Who wants to make a start on writing the tests?

irinaz’s picture

@MegaChriz, I am wondering if we could ask GitHub copilot to write tests. I want to experiment and see what AI can do these days. I can provide code of the module the module and file " src/Feeds/Target/Media.php" ask a question "please write unit test for xxxx function". I wonder what information AI needs to write suggested unit tests.

megachriz’s picture

@irinaz Nice idea to ask AI for writing tests! Maybe they can help a little on the unit tests. But I think we do also need Kernel tests here, that can check if media gets imported as expected. I wonder if AI would be able to figure out how to write tests for that. It would need to know how a Feeds import goes programmatically, it needs to know what data to feed and it needs to know how the imported media should look like.

irinaz’s picture

@MegaChriz, thanks! We probably already have examples for other Targets, correct? We would add examples of data with media, probably. I am quite interested in testing what we need to provide to AI and what can be automated.

irinaz’s picture

Hi @megachirz and @jidrone,
I did an experiment with writing kernel test using AI. I will try to run tests next week, but wanted to share some progress.

giuseppe87 made their first commit to this issue’s fork.

giuseppe87’s picture

StatusFileSize
new16.88 KB

I have added an extra settings to the current patch, that let the user choose to save the new media with the name of the entity or the file.

This could be useful, as it's common to have multiple entities with the same files, and it isn't ideal to have the media name given by the first entity imported.

However being an extra\different approach I haven't updated the MR.

giuseppe87’s picture

StatusFileSize
new17.01 KB

Updated the patch of #104

megachriz’s picture

Issue summary: View changes

I've updated the issue summary and added a list of tasks remained to be done.

megachriz’s picture

@batigolix and I looked at this issue yesterday at the DICTU Drupal Developers Day in Assen.
@batigolix successfully imported a media image on a node. There were some errors, however:

  • When mapping to a media field first and then checkout the code from this issue he got an error for getSummary().
  • The first import test resulted into a SQL error related to language (language cannot be NULL). When changing the language on the target then to "English", the import went fine. It could be that there's an issue when creating a Media entity when the language is not specified. I did see that on his configuration the language on the target was set to an empty string, so perhaps that is related to the error.

@batigolix is working on a test, but ran into the error "filename does not exist", which I did not have an explanation for right away.

hepabolu’s picture

FYI I have tested this:

- Drupal 10.3.10
- PHP 8.3.14
- Lando 3
- Feeds 3.0.0-rc3
- Feeds Tamper 2.0.0-beta4

- Content type Publication with field_cover = Image
- Media type Photos with field_image = Image and field_credits = Text
- Content type Model with field_photos = entity reference to media type Photos

- Feed import Publication, CSV upload, tamper format string to turn image filename into url
- Feed import Photos, CSV upload, tamper format string to turn image filename into url
- Feed import Model, CSV upload, figuring out how to set the reference to the imported Photos

Without any patch:
- the import of the image for Publication cover went fine
- the import of the Photos failed on a missing name, but the files were added anyway with the tampered name as 'filename', adding the Name field didn't help

With the patch from #105:
- I set the language from Default to English
- I explicitly set the Name field to a copy of the filename (in a separate CSV column)

Import of the media type Photos went fine, filename was correct.

I'll see if I can do more tests and report back.

max.valetov’s picture

In #105 if (preg_match('/^Autocreate/', $line->__toString())) { __toString doesn't seem to be a valid method on render array.

megachriz’s picture

In #105 if (preg_match('/^Autocreate/', $line->__toString())) { __toString doesn't seem to be a valid method on render array.

@max.valetov Thanks for spotting! I think that the issue with the "Autocreate" option should be fixed in #2973170: Importing new files fails when "Autocreate entity" option is turned on. first. I think that one is easy to do.

iknowbryan’s picture

SYSTEM:
DRUPAL 11.1.1
PHP: 8.3.12

GOAL: import news from an old site via CSV including the cover image via full URL.

Installed #105 patch
Now gives me the option to reference a media field with the FILE ID. GREAT.
When I run the feed to import that news articles, all the info gets imported without errors. GREAT.
When I edit one of the news items all the info is there including the cover image. GREAT.
(Or so it seams because the thumbnail is generated.)
I can also see the thumbnail in the media library grid.

HOWEVER, when I go edit the media, the image field is empty. BOO.

Any thoughts on how this could be??

jrochate’s picture

Status: Needs review » Needs work

Same as #112, using Drupal 10.4 and PHP 8.4.

When editing Media, the image it's no there, but it can be seen on files and on the related node.

megachriz’s picture

Issue summary: View changes

@jrochate
I added the task "When importing media images via a media field on a node, make sure that the image is visible on the media edit form." to the remaining task list. Does that summarize the issue well?

jrochate’s picture

hi @megachriz

Yep, that nails it. Thanks.

max.valetov’s picture

Hi @megachriz,

@max.valetov Thanks for spotting! I think that the issue with the "Autocreate" option should be fixed in #2973170: Importing new files fails when "Autocreate entity" option is turned on. first. I think that one is easy to do.

Not entirely sure what needs to be fixed/applied from that issue. Applying MR plain diff doesn't resolve the issue.

(Latest v3 feeds with 105 patch applied)

hswong3i made their first commit to this issue’s fork.

hswong3i’s picture

Since people already keep testing #105 and feedback directly, I give a merge to https://git.drupalcode.org/project/feeds/-/merge_requests/161 for simplify.

Moreover, I merge changes for local file import logic from https://www.drupal.org/project/feeds/issues/2968671, else this 2 MR will get conflict and working together.

megachriz’s picture

@hswong3i
I understand that people could need this and #2968671: File target: add support for local file path as source together, but for keeping the issues focussed, it is better to keep them separated. Instead, maybe it is better to open a separate branch called '2928904-2968671-media-and-local-file-do-not-merge' for that.

For people would like to see this issue to be resolved, we need to have automated tests - and there appears to be an issue with updating existing configuration. See Remaining tasks for details.

megachriz changed the visibility of the branch 2928904-add-a-mapping-test to hidden.

megachriz changed the visibility of the branch 2928904-add-a-mapping to hidden.

megachriz’s picture

Issue summary: View changes

I made a start with writing the tests. The tests are currently failing though.

max.valetov’s picture

Thanks @megachriz, applying MR161 works.

We have an issue where on some sites (with the same feed type settings) we are getting an error "Media type could not be determined for [Media Title here]". If I am interpreting things correctly, it gets Title instead of a path so there's no file extension to match against and hence fails to determine the type.
Do you know what could be causing this issue?

megachriz’s picture

@max.valetov
I did not take a deep dive into the Media target issue yet, mostly only worked on the tests so far.
I do know that the source that you should map to the Media target must be an url using the http or https scheme.
Other source types, like local path or not yet supported, but there is being worked on: #2968671: File target: add support for local file path as source.
The source url must contain an extension as well, but there is an issue open for supporting file urls that do not specify the extension: #3076946: Failed to validate remote image with no file extension

If you are not sure what value Feeds receives, I recommend to enable the Feeds Log module (included with Feeds). With that module you can see what items were processed and what the source item looked like at the time of processing.

amanire’s picture

Is it possible to map a source to populate the alt tags of new media items? I've been experimenting with this line of code in a debugger but it doesn't seem to be saving an alt value to the media item or node for me.

    return MediaEntity::create([
      'bundle' => $media_type_id,
      'name' => $media_name,
      'langcode' => $this->getConfiguration('language'),
      'uid' => $processor_owner_id,
      $source_field => [
        'target_id' => $target_id,
        'alt' => $media_name, <-- is always empty??
      ],
    ]);

Thank you for all of your hard work on this issue!

jennypanighetti’s picture

Tried to install patch from MR161 on feeds 3.0 and it failed to apply. Should this be run against dev?
[edit]
Ran against dev, applied, and media items still fail to be linked to the newly imported node.

kopfduenger’s picture

StatusFileSize
new18.73 KB

This patch builds on the patch #105 of giuseppe87 and fixes what was missing in order to attach podcast audio files (my use case) correctly to show up in media entities that were automatically created during a feed import.

Specifically, it:
• Modifies the dedicated Media.php plugin introduced before, and uses the already introduced File plugin to support entity reference fields targeting media entities.
• Automatically selects the correct media bundle based on file extension, even when multiple media types are available.
• Adds support for configuring whether the media entity name is derived from the referencing entity or the file name.
• Ensures existing media entities referencing the same file are reused to avoid duplication.
• Adds proper ownership handling (based on Feed author or fixed user ID).
• Sets the media entity’s alt text and language properly.
• Handles directory creation dynamically based on the media type’s file settings (uri_scheme, file_directory).
• Improves logging and error handling via \Drupal::logger(), especially for download and media creation errors.
• Strips query parameters from URLs to prevent extension mismatches.
• Removes Feeds’ default autocreate option for clarity and to prevent conflicts with custom logic.
• Updates File.php to support passing the file URL to getDestinationDirectory(), enabling per-media-type storage paths.

The patch is intended for drupal/feeds version dev-3.x, commit 9f8db96, and is compatible with Drupal 10 and core Media module setups.

skaught made their first commit to this issue’s fork.

skaught’s picture

Status: Needs work » Needs review

re: 127. have opened branch '2928904-127-feeds-combined-media-fix' with this patch for wider testing.

megachriz’s picture

@skaught
Thanks for helping, could the improvements be added to 2928904-media-target as well? Or do the changes from #127 not build on top of what is in 2928904-media-target? I see that the branch '2928904-127-feeds-combined-media-fix' is based on a very old Feeds version (8.x-3.0-alpha11, almost 4 years ago).

I have planned to look more closely to this issue in September/October this year. (Though I've also a few projects left to make Drupal 11 compatible, which I've also planned for that period.)

megachriz’s picture

Status: Needs review » Needs work
skaught’s picture

@megachriz
i'm not actually clear on the difference beyond what kopfduenger outlines in 127.

To report, at least on my own testing with this:
-> i'm using a Custom Parser in my client project as they have a feed with several custom field names in a feed. in short: patch works as designed. It did successfully take the image page and save out the media item. +1

notes:
- in my case, images are provided by 'in-house feed', i know the images are 'low file-sized'. but this may not be true in the wide. import/batch timeouts COULD BE EXPECTED... perhaps add max file size setting so site owners can set their own 'risk level'...
- i don't know: if routine has any file availability fallout (file not online)
- what if url/file extension is not valid (wrong file type). <<. in Feeds as a contrib. project, this could have security concerns.

megachriz’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests

I've made two updates to the code:

  • Test coverage is added for confirming that the media is available on the media edit form (resolves #112). It is still possible that the media isn't displayed on the edit form, but that could be caused by a misconfiguration on the form display settings for the media type.
  • A post update is added to update existing media target configuration + test coverage (resolves #108).

Still to be done:

  • Add support for media reference fields that allow more than one media type.
  • Add test coverage for referencing existing media.
  • Add a test for importing multilingual media.
  • Add support for importing remote videos.

I'm considering to handle these in follow-up issues. If so, it would be good to document on the media target that:

  • There is an issue when trying to import data for media reference fields that allow more than one media type.
  • Importing remote video's is not supported yet.

I set the status to "Needs review" because I'd like to hear your feedback. :)

megachriz’s picture

Assigned: Unassigned » megachriz
Status: Needs review » Needs work

Now that #3565186: Add service for resolving files is in, the Media target needs to be refactored to properly make use of the new service. I've been working on that, but the code that I have locally is currently based on an older version of the File Resolver service. So it will take me a while to get things sorted out properly.

megachriz changed the visibility of the branch 2928904-2968671-media-and-local-file-do-not-merge to hidden.

megachriz’s picture

I've created a new MR where Media target no longer extends File. This way, the File target doesn't need to be adjusted to add a compatibility layer for the Media target.

The implementation makes use of the new FileResolver service. AI assisted with writing the code. It has been two months since this code was written and it was based on an older version of the FileResolver service. So it's possible some tweaks are needed. But at least I should go thoroughly through all the code to see if everything still makes sense.

megachriz’s picture

Status: Needs work » Needs review

Tests are passing, I think it is ready for testing and reviewing. And I plan to review it too.

megachriz’s picture

Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

I went through all the code and made some minor changes + added more automated tests. I think that this is ready. I plan to merge it soon.

I did identify a few things that could be improved, but I propose to handle that in follow-ups:

  • Add support for importing remote videos.
  • Make the import work for media reference fields supporting multiple media types.
  • Add consistency for setting media owner vs file owner: in some configurations the created file may now get an other owner than the media entity. For file creation there is a fallback to the current user (which is either the feed owner or user 1), but that behavior is not Media specific.
  • Tokens configured for directory on file field possibly not always respected, but that issue is not Media specific.
  • When running the import in multiple threads, it can theoritically happen that two media entities get created that point to the same file, due to a race condition. I expect it's extremely rare to occur, so I just noted it and may open a new issue for it.
priti197’s picture

Thanks for all the great work regards to media related import. For now the image alt is assigned as media label, would be great to have the actual image alt value get assigned during media creation process.

amanire’s picture

I haven't had a chance to try the latest code on this MR but +1 to priti197's request.