When mapping to File or Image fields, empty source for target_id causes Exception and breaks import process.

If the provided value by the source is empty, then the target should be emptied (not skipped). This is how it also works in the D7 version of Feeds.

Second patch is just missing use Drupal\feeds\Exception\EmptyFeedException;
Edit: That's exactly what patch 3 did.

I agree that we need to wipe an existing image if the field is mapped and the source doesnt have a file. That seems to make the most sense, otherwise there is no way for a feed to kill a file.
@MegaChriz I went looking for the file delete in the d7 module and ran across the deleteFile method in FeedsFileFetcher.inc. I see that called if a file is overridden (called in sourceSave if there is an fid, sourceDelete if there is an fid in the source_config, and afterImport if the uploaded file needs to be tossed)but not if the mapping is empty. Is that delete done somewhere else? I'm looking for the existing to find a starting point for this issue.

@wolffereast
Thanks for looking into this! In Feeds there are two places where it deals with files: at the fetcher level and at the mapping level. Fetchers deal with fetching source files which could be in formats like CSV, JSON or XML. Anyway,
these files are expected to contain tabular data which eventually would result into a number of entities being imported (each row from the file representing one entity). At the mapping level, files are saved as an 'attachment' on the entity (they are not parsed or processed). They are saved in a field of type 'file' or 'image'. In the D7 version, the implementation of this mapping target can be found in mappers/file.inc.

I think that in D7, the file module - who provides the file field - handles deleting the file when it receives an empty value. I think what we should do here is prevent Feeds from trying to download the file when the source value is empty and pass an empty value to the file field in question. So that means that getFile() should not be called when the value is empty.

Great, thanks for the starting point! Makes sense to let the file module handle that if possible, I'll look into the getFile call

Looks like this affects the file import as well. Making the getFile call conditional in both prepareValue functions fixes the issue. Attaching the patch, as well as a tar of my config to test. I think thats all you need to get the file and import settings. Let me know if anything is missing.

Scratch that, I had other code that fixed an issue downstream that is still causing problems. Moving back to needs work.

Thanks for your work on this @wolffereast. I guess it would be useful to have an automated test for this. There is now a more solid base test class in Feeds. I think we can handle this case with a Kernel test.

Had to unset the column value entirely to avoid the validation issues. This now works for both images and files on a clean install. Adding the patch that does the trick. Ill see what I can do about tests

Added a test. moving to needs review. The test without the fix has been uploaded for verification.

Let me know if the test could use any optimization. I ran into some weird behavior which drove some of my choices, so feel free to ask if something looks cobbled together.

Great work @wolffereast

Quick glancing over, I do have indeed a question:
Why do we need the anonymous user for this test? Is the issue related to permissions somehow? I thought that the error happened when trying to import an empty value for a file field?

An implementation detail: the methods/properties could be ordered differently:

1. The properties $fileFieldName and $imageFieldName are probably not needed. If it is important to provide different values for them, optional parameters could be added to setUpFileFields().
2. The setUpFileFields() could make use of the existing method createFieldWithStorage() from the trait FeedsCommonTrait.
3. The tests for file target could better live in a separate test class. FeedsItemTest is specifically targeting the feeds_item field stored on entities. Maybe in tests/src/Kernel/Feeds/Target?
4. Enabling the file module could perhaps happen in the test class itself as not every kernel test needs to have that module enabled. I understand though that that could be an issue for setUpFileFields() which assumes the file module to be enabled.

Something seems to be wrong with the test only patch as it is passing.

I'm headed out so Ill take a look at reorganizing the tests a bit later, but I can shed a little light on the anonymous user for now. When running the test without specifically creating an anonymous user with the 'access content' permission the feed was unable to import the content, erroring out with something about access to the file being denied. Digging into that it turns out that a user requires the access content permission to get to any public files. I tried granting anonymous users the access content permission using a function (Im seeing a method here that might do the trick that I hadnt tried) that Im not seeing with a quick search, but firing that then checking if the user had the access content permission (hasPermission, or something similar) returned FALSE. Im going to try the grantPermission method on the role when I have a chance later.

@wolffereast
Perhaps the test can be made simpler: add the CSV files to import to the tests/resources/csv directory and then reference them like this:

$feed = $this->createFeed($feed_type->id(), [
  'source' => $this->resourcesPath() . '/csv/content.csv',
]);
$feed->import();

This is what other tests are doing now. See for example FeedsItemTest::testUpdateItemWithFeedsItem().

+++ b/tests/src/Kernel/FeedsItemTest.php
@@ -147,4 +150,108 @@ class FeedsItemTest extends FeedsKernelTestBase {
+    $feed->set('source', \Drupal::service('file_system')->realpath($scheme . '://feedTestNoFiles.csv'));

Updating a source can be with the setSource() method:

$feed->setSource($this->resourcesPath() . '/csv/content_updated.csv');

Setting to "Needs work" because the test only patch should have failed tests.

I found that when I had the patch from https://www.drupal.org/project/feeds/issues/2951965 applied then the test would succeed even without the patch, while without that patch it correctly fails. I receive a notification that the extension is invalid when I have the patch applied while the files are correctly removed, but a fatal error without it which keeps the files from getting deleted. I think that means this bug might be limited to the php versions mentioned in the other patch.

How do I go about testing to ensure no notices where thrown during the call? If I can test that then I can force the test to fail if the files are removed but it still calls the getFile function.

A bunch of improvements to the test based on your suggestions:

• I fixed my issue with assigning the access content permission to the anonymous role. Turns out I needed to install the user config before the grantPermission call would stick.
• Removed the fileFieldName and imageFieldName properties in favor of hard coded field names. I don't see a need for arguments, though I guess they could be future friendly.
• setUpFileFields now uses createFieldWithStorage. Thats a helpful method, didn't know it existed.
• Moved the test into its own file
• Enabled the file module in setUpFileFields
• I moved the CSVs into the resource directory. I had initially attempted to load the generated files by url to better emulate the test case, but curl didn't want to play nicely with generated file path, so I switched it to use the file IDs. As long as I can assume that those file IDs won't change then this should be fine.
• used the setSource method to set the source.

Still trying to figure out how to catch the notice in the test, but this does fail for me with the base test and succeed with the test+fix.

When importing an empty value for an image I no longer get an exception as I used to get over a year ago. Maybe the exception happens only on specific versions of PHP? Or maybe something changed in Drupal core that now prevents the exception from happening? On my local machine I'm on Drupal 8.5.1 and PHP 7.0.14.

Hm, the error message is from \Drupal\feeds\Feeds\Target\File::getFileName().

It seems that the fatal error has much to do with #2951965: PHP 5.6: Fatal error during import: FormattableMarkup::__toString() must not throw an exception.

So the test in this issue should somehow ensure that no exceptions of type TargetValidationException are thrown when importing empty values for files.

I gave catching the TargetValidationException when firing the feed->import() a shot with no luck. Looks like its being caught and logged in src/Plugin/Type/Target/FieldTargetBase:prepareValues. Which makes sense, we need to know it happened, but it shouldn't kill the whole import. Would it be possible to assert that the getFile method in the File and Image classes had not been called during the second import? Or assert they had each been called once and only once each after the two feed calls?
We could check for the error message created in prepareValues by getting the messages and seeing if the error array has anything in it, but that seems like a roundabout way to get to the information.

Looking at the code I see that a TargetValidationException is catched early. To test that it is not thrown, the test should be a "Unit" test, meaning that the file target should tested in isolation (thus without the whole import process). Ideally, this test would be added to \Drupal\Tests\feeds\Unit\Feeds\Target\FileTest. But it could be hard to mock the file system in Drupal unit tests. If this is the case, the test may be "upgraded" to a kernel test instead.

The other test, that ensures that file fields can be emptied, can be kept. It only no longer have to fail on the testbot in this issue. As I think now that the fatal error is actually the issue reported in #2951965: PHP 5.6: Fatal error during import: FormattableMarkup::__toString() must not throw an exception, we should try to come up with a test that triggers the fatal error there. I'm still trying to find out how a TargetValidationException can result into a fatal error.

Since we are trying to track down the fatal error in the other ticket does that mean this one should move to needs review, or is there other work outstanding?

@wolffereast
In this issue an unit test should be added that covers behavior for passing an empty value for the file target.

Pseudo-code:

// Test passing empty value behavior (should not result into a TargetValidationException).
$target = new File(...);
$this->callProtectedMethod($target, 'prepareValue', [
  0,
  ['target_id' => ''],
]);

It would also be cool if this could be extended with unit tests for other cases, but we could postpone this to a follow-up if it turns out it is hard to implement:

// Test passing a normal value.
$target = new File(...);
$this->callProtectedMethod($target, 'prepareValue', [
  0,
  ['target_id' => 'http://www.example.com/file.txt'],
]);

// Test passing a file that cannot be found.
$target = new File(...);
$this->callProtectedMethod($target, 'prepareValue', [
  0,
  ['target_id' => 'http://www.example.com/not-found.txt'],
]);

// Test passing a file with an invalid extension.
$target = new File(...);
$this->callProtectedMethod($target, 'prepareValue', [
  0,
  ['target_id' => 'http://www.example.com/file.foo'],
]);

Note: callProtectedMethod() is defined in \Drupal\Tests\feeds\Traits\FeedsReflectionTrait.

I tried something like I proposed in #31. Patch does get a bit big perhaps. I should probably move some of it to #2940280: Add test coverage for the file target..

I also have some issues with getting a working http client in a kernel test.

This a tests only patch.

Try again, without the assets (these are committed separately).

Part of the last patch went in #2940280: Add test coverage for the file target.. Here is a reroll. Still a tests only patch.

Forgot use statements.

Expanding test coverage to image fields. Still tests only.

Now with a possible fix. The fix is actually similar to the fix in #9. I had assumed that fix would make it impossible to empty file fields, but I was wrong.

Working.. Thanks!

Thanks for contributing, @veronicaSeveryn, @wolffereast
Thanks for testing, @saranya purushothaman

Committed #38.

I tried very hard to map the d7 user image (a custom image field, not default user image) with d8 user_picture using feeds. I could't figure out how to make it work. I can only map the File ID. How does it work? Thank you.