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
- Install Node, Media, Feeds.
- Create a content type.
- Add a media field to the content type (for example called "field_media").
- Create a feed type.
- 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.

Issue fork feeds-2928904
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
Comment #2
megachrizIs 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.
Comment #3
megachrizComment #4
thomasfowles commentedI 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...
Comment #5
megachrizClosed #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:
Comment #6
thomasfowles commentedI'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:
Comment #7
thomasfowles commentedComment #8
tedwyer commented@thomasfowles - you can enable the core media in 8.5. Working with your patch. Thanks for the effort. No result yet.
Comment #9
megachrizFrom #2973170-3: Importing new files fails when "Autocreate entity" option is turned on.:
From #2973170-5: Importing new files fails when "Autocreate entity" option is turned on.:
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.
Comment #10
kobb commentedFYI: It looks like this is being put into core in 8.6.
#2835825: Add d8 media migration
Comment #11
opsdemon commentedFor 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!
Comment #12
hongpong commentedThanks 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?
Comment #13
josh waihi commentedThe 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.
Comment #14
megachriz@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.
Comment #15
drupalnesia commentedPath #11 working fine, successful import 2,000 node with Video Embed Field, thanks!
Comment #17
drupalnesia commentedPath failed to submit because of underscore on VideoEmbed.php_.patch ? Should be VideoEmbed.php.patch ? If so, how to rename?
Comment #18
hongpong commenteddrupalisme 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.
Comment #19
TimelessDomain commented2928904-19-videoembed-mapping.patch works well, thanks! same as #11 but renamed accordingly
Comment #20
OCTOGONE.dev commentedIf you are wondering where to apply that patch, you have to copy the file VideoEmbed.php to the folder feeds/src/Feeds/Target.
Comment #21
jjwfcd commentedconfirm #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.
Comment #22
megachrizSupport 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:


Comment #23
TimelessDomain commentedAll future work for Video Embed Field feed mappers should go into this issue https://www.drupal.org/project/video_embed_field/issues/3056385
Comment #24
darrenwh commentedRerolled patch 19, did not apply
Comment #25
mrpauldriver commentedThis 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.
Comment #26
jwilson3I 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_idwhich 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:
Comment #27
marthinal commentedI've created a plugin for images.
Comment #28
marthinal commentedComment #29
megachriz@marthinal
Thanks for contributing!
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.
Comment #30
marthinal commented@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.
Comment #31
marthinal commentedIf the fid already exists then can use the same Media entity
Comment #32
marthinal commentedI'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.
Comment #33
tostinni commentedI used this patch to import a RSS feed from a Drupal site and the images had an itok query paramenter
image.jpg?itok=f7JbTrD1which 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.
Comment #34
marthinal commentedComment #35
marthinal commentedIn 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.
Comment #36
bkosborneHmm 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.
Comment #37
megachriz@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.
Comment #38
finex commentedHi, on my simple case (single image per post) the patch #35 worked very well. Thanks!
Comment #39
gngn commented#35 worked for me.
One small thing: in MediaImage::setTarget line
generates a PHP Notice: Only variables should be passed by reference.
I think we should change that to:
New patch and interdiff attached.
Comment #40
carolpettirossi commentedI'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_logowhich 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:
Is anyone able to help me using this patch successfully?
Thanks :)
Comment #41
EducoWebDesign commentedThis 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?
Comment #42
carolpettirossi commentedHi EducoWebDesign,
Actually not. We decided to got with another approach since uploading/creating the media via the CSV wasn't working.
Comment #43
vlooi vlerke commentedI 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
Comment #44
vlooi vlerke commented@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
Comment #45
vselivanovHi!
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:
Main part of the YML config:
Comment #47
vselivanovSmall improvement: we don't need to rename media if it was reused. Just ensure that it's published.
Comment #48
socialnicheguru commentedthis is not applying to new dev version of feeds
Comment #49
socialnicheguru commentedI just rerolled for it to apply. No other changes were made for the dev version
Comment #50
alemadleiThe 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.
Comment #51
neograph734Switching NR for the latest patches.
Comment #53
mattjones86Re-roll of patch for latest dev
EDIT: not sure on test failures, perhaps more changed on latest dev than I thought.
Comment #55
marthinal commentedHi 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.
Comment #57
marthinal commentedOkay the new error makes sense
The tests need work.
Comment #58
marthinal commentedUpdating the patch
Comment #59
marthinal commentedComment #61
mattjones86Does 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?
Comment #62
megachriz@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:
I like to see this being moved to MediaTargetBase.
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 ifMediaTargetBase::targets()bails out early if the entity type "media" does not exist.Comment #63
neograph734Hi 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?
Comment #64
b.friddy commentedHello @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:
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
Comment #65
MacSaveNy commentedThanks 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
Comment #66
b.friddy commentedFYI - 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.
Comment #67
Christopher Riley commentedI 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.
Comment #69
socialnicheguru commentedWhen 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_$typeplugins.$target->setPluginId("media_$type");Comment #70
demonde commentedSorry, 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.
Comment #71
Christopher Riley commentedTried 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).
Comment #72
Christopher Riley commentedI 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?
Comment #73
bdimaggioHave 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) togetName(), 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.Comment #74
gngn commented@bdimaggio you are talking about #58, yes?
#58 contains three uses of getTitle(), all on EntityInterface $entity:
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...
Comment #75
bgilhome commentedHere's an updated patch replacing getTitle() on the media entities with core method label().
Comment #76
ecj commentedwell 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
Comment #77
ecj commentedwhen this patch is applied, and when under mapping trying to add a mapping of an image field, following error msg:
so seems to see image field as a media entity I guess
Comment #78
mattjones86If you're
Mediaentity type does not exist, then most likely you need to install theMediamodule?I was getting the same thing in my comment above. It was because the media module was not enabled before running the tests.
Comment #79
megachrizThe 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:
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:
If a target depends on the existing of something, for example a module, see how the Book target handled that:
Comment #80
roydench commentedCompatible with core 9.5.9 and 8.x-3.0-beta4
Comment #81
joaopauloscho commented#80 works for us, thanks.
Comment #82
besek commented#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
Comment #83
jrochate commentedI 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 53and 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.
Comment #84
amanire commentedI've updated the patch in #80 for Drupal 10 compatibility. The corresponding feed had stopped importing and was throwing the following warning:
Oops, looks like i didn't get everything in that patch. Trying again.
Comment #85
amanire commentedComment #86
amanire commentedOK this patch now includes the explicit accessCheck for D10 compatibility.
Comment #87
amanire commentedComment #88
ed2908 commentedOn 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?
Comment #89
isa.belI had some issues after updating to the latest stable version of the module, but using the patch from #84 worked for me!
Comment #90
jesss commentedPatch #86 is working for me!
Comment #91
djschoone commented@jesss on which version did you apply? #86 on 3.0.0-beta4 gives me when visiting admin/structure/feeds/manage//mapping this error:
Comment #92
jesss commentedI'm on 3.0.0-beta4. Did you clear cache after applying the patch?
Comment #93
nicasso commentedI'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).Comment #96
jidrone commentedHello,
The existing patch has the following issues:
This new MR, has the following approach:
I know there are still improvements to do and it will need tests, but this could be a better starting point.
Comment #98
megachrizThanks @jidrone! That looks like a big step in getting this issue done. 😃
Who wants to make a start on writing the tests?
Comment #99
irinaz commented@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.
Comment #100
megachriz@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.
Comment #101
irinaz commented@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.
Comment #102
irinaz commentedHi @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.
Comment #104
giuseppe87 commentedI 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.
Comment #105
giuseppe87 commentedUpdated the patch of #104
Comment #106
megachrizI've updated the issue summary and added a list of tasks remained to be done.
Comment #108
megachriz@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:
getSummary().@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.
Comment #109
hepabolu commentedFYI 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.
Comment #110
max.valetov commentedIn #105
if (preg_match('/^Autocreate/', $line->__toString())) {__toString doesn't seem to be a valid method on render array.Comment #111
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.
Comment #112
iknowbryan commentedSYSTEM:
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??
Comment #113
jrochate commentedSame 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.
Comment #114
megachriz@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?
Comment #115
jrochate commentedhi @megachriz
Yep, that nails it. Thanks.
Comment #116
max.valetov commentedHi @megachriz,
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)
Comment #118
hswong3i commentedSince 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.
Comment #119
megachriz@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.
Comment #122
megachrizI made a start with writing the tests. The tests are currently failing though.
Comment #123
max.valetov commentedThanks @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?
Comment #124
megachriz@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.
Comment #125
amanire commentedIs 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.
Thank you for all of your hard work on this issue!
Comment #126
jennypanighetti commentedTried 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.
Comment #127
kopfduenger commentedThis 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.
Comment #129
skaughtre: 127. have opened branch '2928904-127-feeds-combined-media-fix' with this patch for wider testing.
Comment #130
megachriz@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.)
Comment #131
megachrizComment #132
skaught@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.
Comment #133
megachrizI've made two updates to the code:
Still to be done:
I'm considering to handle these in follow-up issues. If so, it would be good to document on the media target that:
I set the status to "Needs review" because I'd like to hear your feedback. :)
Comment #134
megachrizNow 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.
Comment #137
megachrizI'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.
Comment #138
megachrizTests are passing, I think it is ready for testing and reviewing. And I plan to review it too.
Comment #139
megachrizI 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:
Comment #140
priti197 commentedThanks 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.
Comment #141
amanire commentedI haven't had a chance to try the latest code on this MR but +1 to priti197's request.