FeedsParser throws an exception if http_get_request does not return 200 in getContent() method. However in getFile() method there's no try catch clause for that and it causes an error for instance in case you are requesting an image which does not exist and therefore the whole node creation fails.

Comments

vaartio’s picture

StatusFileSize
new1.52 KB

Here a patch for that.

vaartio’s picture

There were problems applying the one above so here's the new one.

johnbarclay’s picture

Title:Feeds does not catch exceptions properly» Feeds does not catch file exceptions properly in file mapper and FeedsParser.inc getFile() function
Priority:Normal» Major
Status:Active» Reviewed & tested by the community

This turned out to be a big problem. In the case where the image url is empty or NULL those nodes do not import. Although the error message is clear, one has to create multiple feeds for the same entity. One for the other fields and one for the images.

This patch worked fine for me on 7.x-2.x head on Drupal 7.8. I also looked over the code in the patch and don't see any problems it would create.

Another patch should be written for the special case where the value of the file url is NULL or empty. The module should not request the file at all.

Dave Reid’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests
+++ mappers/file.incundefined
@@ -72,7 +72,13 @@ function file_feeds_set_target($source, $entity, $target, $value) {
+    catch (Exception $e) {
+      watchdog('Feeds', nl2br(check_plain($e->getMessage())), NULL, WATCHDOG_WARNING);
+    }           ¶

This should probably be using watchdog_exception()?

+++ plugins/FeedsParser.incundefined
@@ -323,7 +323,12 @@ class FeedsEnclosure extends FeedsElement {
+        try {
+          $file = file_save_data($this->getContent(), "$destination/$filename");
+        } ¶

Same here.

jedprentice’s picture

I apologize if this question has an obvious answer, but could you provide a sample file and/or provide steps to reproduce? That would go a long way toward getting this patch reviewed, tested, and committed. So far I'm having trouble reproducing this issue based on the info provided.

Thanks,
Jed

bancarddata’s picture

The problem this patch is attempting to solve is if you run an import with an image field defined, and your source contains a path to an image that does not exist, the expected behavior would be to import the node without an image and log the fact that the image was not found. Currently, however, the whole node fails to import. This patch brings us the expected behavior.

I have tested on D7.12 and the latest dev copy of Feeds - working fine for me. Thank you vaartio.

bancarddata’s picture

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

Here is a rerolled patch against the latest dev with the changes recommended in #4.

franz’s picture

Status:Needs review» Needs work
+++ b/mappers/file.inc 2012-03-07 18:16:24.000000000 +0000
@@ -75,7 +75,13 @@ function file_feeds_set_target($source,
+    } ¶
+    catch (Exception $e) {
+      watchdog_exception('Feeds', nl2br(check_plain($e)));
+    }           ¶

+++ b/plugins/FeedsParser.inc 2012-03-07 18:16:37.000000000 +0000
@@ -349,7 +349,12 @@ class FeedsEnclosure extends FeedsElemen
+        } ¶

Some trailing whitespace to remove.

franz’s picture

StatusFileSize
new1.39 KB

Re-rolled and cleaned.

franz’s picture

Status:Needs work» Needs review
Fabianx’s picture

Status:Needs review» Needs work

Code should look like this:

<?php
          watchdog_exception
('Feeds', $e, nl2br(check_plain($e)));
?>

Exception is second argument

franz’s picture

Status:Needs work» Needs review
StatusFileSize
new1.4 KB

Fixed watchdog_exception() calls.

sjhuda’s picture

I had a CSV file with an image column. Some rows had images, others hadn't. Without this patch the rows without images would fail. Tested the patch on 7.x-2.0-alpha5 and it worked like a charm. Thanks!

axel.rutz’s picture

twistor’s picture

Status:Needs review» Fixed

Status:Fixed» Closed (fixed)
Issue tags:-Needs tests

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