Problem/Motivation

In Feeds 7.x-2.0-alpha8 and earlier there was some inconsistency with importing "blank" values (0, NULL or ""). Some mappers ignored empty values, some overwrote the target value.

The patch from #161 was committed to fix this inconsistency and always overwrite the target value, even if the field was not in the source.

There are some different kinds of expectations around importing blank values:

  1. Some expect blank values should always be ignored.
  2. Some expect blank values should always overwrite the target.

The first behaviour is how it was for most mappers in 7.x-2.0-alpha8 and earlier, the second behaviour is how it is now in dev. Providing the best of both worlds was not possible for the following reasons:

  • With the current architecture, the distinction of non-existent vs. empty can not be made for a generic solution. (See #147, #160)
  • Making every field configurable as to how it handles empty values is outside the scope of Feeds. (See #162)

Proposed resolution

Always overwrite the target value. Patch for this was committed. For the behaviour "ignore empty values" there is now the experimental module Feeds empty (suggestions for a better name are welcome, see #164).

Remaining tasks

  • Write tests for the new behaviour. The tests should check if target values are overwritten when importing blank values for the following mappers:

    A start for this test was made in #173, next patch was in #194.

Original report by jptl

So if the csv for field_summary is blank (hello,,1234), when the node is displayed, it still shows as...

Something: hello
Summery:
Value: 1234

I can fix it by editing the node and, without making any changes, just pressing save but that can be pain when uploading 500+ nodes.

From what I can tell, it only happens for text fields. I was looking through the code and in mappers/field.inc, it has the following but that doesn't seem to work.

  if (empty($value)) {
    return;
  }
CommentFileSizeAuthor
#224 feeds-1107522-224-allow_processors_to_override_nulling.patch2 KBmikemccaffrey
#223 allow_processors_to_override_nulling_1107522-223.patch1.16 KBgcb
#212 feeds-test-cases-empty-fields-1107522-212.patch47.34 KBtwistor
#212 interdiff.txt15.44 KBtwistor
#209 feeds-test-cases-empty-fields-1107522-209.patch36.83 KBtwistor
#209 interdiff.txt893 bytestwistor
#208 interdiff-1107522-207-208.txt1.94 KBMegaChriz
#208 feeds-test-cases-empty-fields-1107522-208.patch36.49 KBMegaChriz
#207 feeds-test-cases-empty-fields-1107522-207.patch36.12 KBtwistor
#207 interdiff.txt522 bytestwistor
#205 feeds-test-cases-empty-fields-1107522-205.patch36.06 KBtwistor
#205 interdiff.txt1.61 KBtwistor
#199 interdiff-1107522-197-199.txt1.41 KBMegaChriz
#199 feeds-test-cases-empty-fields-1107522-199.patch35.28 KBMegaChriz
#197 interdiff-1107522-196-197.txt8.28 KBMegaChriz
#197 feeds-test-cases-empty-fields-1107522-197.patch35.23 KBMegaChriz
#196 interdiff-1107522-195-196.txt1007 bytesMegaChriz
#196 feeds-test-cases-empty-fields-1107522-196.patch27.84 KBMegaChriz
#195 1107522-195-failing-link-test.png25.23 KBMegaChriz
#195 interdiff-1107522-194-195.txt9.41 KBMegaChriz
#195 feeds-test-case-empty-fields-1107522-195.patch27.4 KBMegaChriz
#194 feeds-test-cases-empty-fields-1107522-194.patch18.46 KBMegaChriz
#182 allow_processors_to_override_nulling_alpha8.patch1.03 KBgcb
#181 allow_processors_to_override_nulling.patch1013 bytesgcb
#173 feeds.test-case-fix-empty-fields-1107522-173.patch8.32 KBMegaChriz
#172 feeds.test-case-fix-empty-fields-1107522-172.patch7.7 KBMegaChriz
#161 feeds-fix-empty-fields-1107522-162.patch816 bytestwistor
#123 feeds.code_.1107522-123.patch6.91 KBmparker17
#123 interdiff.txt3.52 KBmparker17
#112 feeds-empty_text_field_value_imports-1107522-112.patch7.44 KBditcheva
#110 feeds-empty_text_field_value_imports-1107522-110.patch5.15 KBditcheva
#104 feeds_empty_field_options_1.png17.22 KBditcheva
#104 feeds_empty_field_options_2.png45.35 KBditcheva
#104 feeds-empty_text_field_value_imports-1107522-104.patch10.92 KBditcheva
#97 file.txt4.58 KBMichsk
#95 import_improvements.png28.62 KBditcheva
#85 Screen Shot 2013-06-14 at 10.02.59.png254.91 KBvaccinemedia
#85 Screen Shot 2013-06-14 at 10.06.23.png490.43 KBvaccinemedia
#85 Screen Shot 2013-06-14 at 10.05.31.png265.69 KBvaccinemedia
#81 feeds-empty-behavior-1107522-81.patch6.58 KBUhkis
#65 feeds-empty-behavior-1107522-65.patch4.14 KBUhkis
#63 ignore-empty-taxonomy-terms-1107522-63.patch451 bytesguillaumev
#60 ignore-empty-link-fields-1107522-60.patch480 bytescthiebault
#59 feeds-empty-behavior-1107522-59.patch950 bytescthiebault
#37 feeds-empty_values.patch2.62 KBfranz
#25 feeds-empty_values.patch2.12 KBfranz
#15 ignore-empty-link-fields-1107522-15.patch568 bytesnielsonm
#15 ignore-empty-field-values-1107522-15.patch513 bytesnielsonm
#13 ignore-empty-taxonomy-terms-1107522-13.patch522 bytesnielsonm
#3 1107522-empty-fields-2.patch1005 bytesNiklas Fiekas
#2 1107522-empty-fields.patch992 bytesNiklas Fiekas
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ownage’s picture

Priority: Normal » Major

I'm having the same exact problem. The labels of empty fields should be hidden, but in this instance Feeds seems to keep them there. I need to find a way to fix this ASAP because I have ~6k nodes to import.

Anyone have some help to lend?

Niklas Fiekas’s picture

Status: Active » Needs review
FileSize
992 bytes

Please try this untested, hacky, naive patch.

Niklas Fiekas’s picture

FileSize
1005 bytes

No that won't work, because it's neither FALSE nor NULL but ''. empty or == can't be used, because 0 as an integer value might be valid.
Next try attached.

ownage’s picture

Status: Needs review » Fixed

FIXED, thank you very much Niklas!

Niklas Fiekas’s picture

Status: Fixed » Needs review

Great, thank you for testing.

I am setting this back the needs review because the patch isn't comitted, yet. To discuss: Anything to improve coding style wise? Check for empty fields at a different point in code?

froboy’s picture

After my first import had many empty fields showing up, I tested this with an CSV import of 320 rows with about 20 fields each including many blanks. Everything seems to have worked as advertised. The code looks OK from my quick inspection, but I'm not much of an authority on that.

franz’s picture

Title: When importing empty/blank columns from CSV, field label is still displayed in node » When importing empty/blank values, field label is still displayed in node

This is not for CSV only, but any source where you have mappings but empty input values for fields.

franz’s picture

Status: Needs review » Reviewed & tested by the community

And it works! (using XPath Parser on a XML)

johnbarclay’s picture

#3 didn't break anything for me, but didn't solve my problem.

My issue seems completely different, but has the same symptoms: Somehow all my text fields being imported have a single blank space in them. I used the trim function in feeds_tamper and everything is fine.

dman’s picture

+1 this is helpful
I think building in a trim() would be most intuative in most cases. Thinking of user expectations rather than as a coder.

smitty’s picture

This patch works fine, thank you!

But unfortunately it only affects numerical and string fields but not taxonomy fields.

To prevent empty taxonomy fields from showing up you must change function taxonomy_feeds_set_target in line 67 in taxonomy.inc from

if (empty($terms)) {

to

if (empty($terms) || empty($terms[0])) {

This might not be very smart but it does the trick. So my question: has anybody a better solution for this?

franz’s picture

Status: Reviewed & tested by the community » Needs work

I think all official target handlers should get similar fixes, if one can post a patch using #3 as a base

nielsonm’s picture

Here's a patch, based off of #11 that checks for empty taxonomy terms in the foreach loop.

franz’s picture

Status: Needs work » Needs review
nielsonm’s picture

Patched the link and field mappers to ignore empty fields.

rtdean93’s picture

Status: Needs review » Reviewed & tested by the community

I have manually applied the patch in #3 for the latest dev version - and it worked fine. I have opened a new thread for that release at http://drupal.org/node/1372064#comment-5368824

colan’s picture

Version: 7.x-2.0-alpha3 » 7.x-2.x-dev
Status: Reviewed & tested by the community » Needs work

Fix in the latest dev branch first, then backport.

xlyz’s picture

Status: Needs work » Reviewed & tested by the community

patch at #3 works here as well on latest dev. please commit.

emackn’s picture

Status: Reviewed & tested by the community » Needs work

one suggestion for #3, use $entity->language instead of 'und'

Niremizov’s picture

mappers/field.inc
Inside function field_feeds_set_target_text (line 71), there is a check if $value is an array:

function field_feeds_set_target_text($source, $entity, $target, $value) {
  if (!is_array($value)) {
    $value = array($value);
    /* if it is not an array, then create an array from $value... 
        So if $value == NULL, we get:
    $value = array (
       [0] => NULL
    )
    */
  }

______________

Inside "_field_feeds_set_target" function (line 96), there is a check if $value is empty... But

function _field_feeds_set_target($source, $entity, $target, $value, $input_format = FALSE) {
  if (empty($value))  
  /* empty(array([0]=>NULL)) will always return FALSE, 
      so there is no way to get inside "if" statement and return...*/
 {
    return;
  }

I suppose that the check if the $value is empty should be done inside "field_feeds_set_target_text" before "is_array" check, for example:

function field_feeds_set_target_text($source, $entity, $target, $value) {
  if (empty($value)) {
    return;
  }
  if (!is_array($value)) {
    $value = array($value);
  }

...?

Shiraz Dindar’s picture

patch in #3 worked for me too. It applied cleanly to alpha4.

colan’s picture

@emackn: That's better, but what's best is using field_get_items().

madeby’s picture

Will these patches help the following problem:

Empty fields (in the file) overwrite fields with value (in Drupal.)

Also, are these patches part of the latest -DEV?

madeby’s picture

Applied both patches and they work which is awesome! We need one for taxonomy as well. @nielsonm is this something you can do? Or do we have someone that understands this module enough to do it?

Thanks!

franz’s picture

Status: Needs work » Needs review
FileSize
2.12 KB

So we need to combine patches from #3 , #13 and #15. I feel the issue metioned in #19 or #22 do not belong to this issue, as 'und' was being used before it, so better spin-off (if there isn't an issue about it already).

I'm currently testing this, but please test this and review it so we can get it committed soon.

madeby’s picture

Thanks for doing this.

Will #25 prevent the following:

The CSV I'm importing contains a empty value but the drupal node that I'm updating contains a manually entered value. Now, it will not overwrite this with a empty value? Correct?

franz’s picture

Correct, it's supposed to be that way. You can test this on local backup and report if it works well.

madeby’s picture

I have been testing this for a while and I does not seem to work properly.

If the field has content on the drupal node but nothing in the CSV. The content in the field on the drupal node will be replaced and also be empty.

It is very important that it do not replace content with empty and only overwrite the field with empty if it is empty.

cdmo’s picture

I've been trying to figure out what's going on with a problem I'm having related to the Feeds XPath Parser module that seems related to this thread. Basically I'm trying to import a value of 0 and any time I try to do that it just quietly ignores the import. I applied this patch and it didn't have any impact unfortunately. Any advice would be appreciated. My original post is here: http://drupal.org/node/1463232

Thanks.

franz’s picture

@madeby, this was exactly what the patch did, and what you describe is the un-patched behavior. Have you applied the patch properly? What versions are you using?

@cdmo, I don't think this is related, as this thread is discussing the very opposite, unless you applied this patch in the past and it's now causing it. Otherwise, this is another issue.

franz’s picture

FWIW, questions in #19 and #22 can be discussed #1232836: Use proper language instead of 'und'..

madeby’s picture

I use Alpha 4. Can I only use DEV?

nielsonm’s picture

It might not be a bad idea, as long as you're not on a production site or it doesn't break functionality. You could also reroll the patch for alpha.

bnine’s picture

Category: bug » feature
Status: Needs review » Needs work

What about file field?

madeby’s picture

Downloaded a fresh copy of feeds 2.0 DEV. (From March 1)

Tried to apply the patch in #25 with git apply -v feeds-empty_values.patch in the feeds folder.

However, it do not work. The patch do no alter any files.

Can you clarify which version it works on?

franz’s picture

double posting.

franz’s picture

FileSize
2.62 KB

@madeby. Did you download the tarball and used "git apply" to apply the patch?

I just applied the same logic to the file mapper too.

Now, I'm not certain if this is the best way of doing it. As I saw in another issue for Feeds 6.x, using empty() purges "0" as a source value. Sometimes people will want to use these values, even NULL or something, so maybe there should be an option for that in the source config: "Don't skip empty values"/"Feeds usually doesn't update or create data with empty or "0" values. Check this box to override this behavior."

What do you say?

franz’s picture

Title: When importing empty/blank values, field label is still displayed in node » Expected behavior when importing empty/blank values.
Category: feature » task

Another desired behavior here is to have empty values reset the target: #1528784: Behavior when importing empty/blank values for numeric field.

I'm renaming the issue to widen the scope. I'll add information to the issue summary.

franz’s picture

madeby’s picture

@franz turns out the patch only works on the Alpha 4 version and not DEV.

franz’s picture

Per-mapper config is closer than I believed: #860748: Config for mappers?

franz’s picture

franz’s picture

Issue summary: View changes

Updated issue summary.

lunk rat’s picture

Now, I'm not certain if this is the best way of doing it. As I saw in another issue for Feeds 6.x, using empty() purges "0" as a source value. Sometimes people will want to use these values, even NULL or something, so maybe there should be an option for that in the source config: "Don't skip empty values"/"Feeds usually doesn't update or create data with empty or "0" values. Check this box to override this behavior."

I have many "0" values in my csv: I use them to set the key of select lists, or of boolean fields, for example. Clearing "0" csv values would be a disaster.

gaele’s picture

What's meant here? (#37)

  foreach ($value as $v) {
    // Check if field value is empty.
    if(empty($v[0])){
      continue;
    }

This will check whether the first character of a string value is empty, so "003452" will be considered empty.

franz’s picture

It is certainly not right, we need to pull in all the cases and provide the proper treatment. Using empty($string) also fails when string is "0" or "00". Please check summary, I'm sure we can start sketching those conditions and also what to do for each behavior.

franz’s picture

Issue summary: View changes

Updating proposed resolution.

lunk rat’s picture

I'm not up to writing patches for this, but I will share my use-case, as it points to the need for a config in the mapper to allow the user to decide how empty fields should be handled:

  1. When you have several columns in a csv that you would like to map to the same Drupal field. For example, one of my csv files has a column "youtube-video-url" and another field "vimeo-video-url", and, in this case, any given row will never have a value in both columns (it's either/or for each guid). In this case I would want to tell feeds to map both columns to the same Drupal field "field_media_video", and select this new setting of "Do nothing with blank values" so that if one column has a blank value it will not reset the value of the other column. This of course is the current default behavior, but would need to be preserved via a setting if we implement updating for empty source values.
  2. In another instance I have a boolean field where '0' represents some label and '1' represents another. In this case I do not want '0' source values to reset the target field.
  3. Right now I have select list fields that have their label displayed (with no value) as a result of when the feeds source for that field is empty. Not cool. Seems to be only with select list fields, or any field with a 'key'
  4. Multiple value fields. If I import a csv with source column 'image url' and map it to a multi-value Drupal field, then import a different csv that has a new value for 'image url', the default behavior is to overwrite the original value. But if it is a multi-value field, I might want feeds to just 'add' a second value to the multi-value Drupal field.
  5. Multiple value fields that are exploded from source with feeds tamper. This is related to empties, but probably more related to feeds tamper: If I have a multi value field where feeds tamper explodes a pipe-separated list from source, and then I import another source file that has a pipe-separated list of different values for the same node guid, the new values replace the old vlaues. What if I just want the new source values to add to the existing values for that node's multi-value field? Illustrates another case for a mapper config option.

I will update this as I think of more examples.

franz’s picture

We also seem to need a "0" setting like:

"Consider 0 (zero) values as empty".

Let's see a first sketch on those 3 behaviors from summary:

  1. Do not touch: skip setting the target value. In case of updating items, this preserves the previous value.
    <?php
      if (empty($value) && ($zero_empty || $value == "0" || !strlen($value))) {
        continue;
      }
    ?>
  2. Import anyway: Overwrite the target value with blank, NULL or 0. For numeric values, this can be useful. However, in most cases, this will cause empty fields to have a label unexpectedely displayed.
    <?php
      // No action needed here, this is the most simple (need to review every default mapper).
    ?>
  3. Reset field value: #1528784: Behavior when importing empty/blank values for numeric field proposes empty values should reset the field. Maybe this will depend on the target field.
  4. <?php
    if (empty($value) && ($zero_empty || $value == "0" || !strlen($value))) {
        $value = NULL;
    }
    ?>
franz’s picture

Issue summary: View changes

changed "expectated" to expected

franz’s picture

I've updated the summary and re-added related issues as sub-tasks. Just need to finish config for Feeds so we can work on each of them.

madeby’s picture

Any news on this in Alpha 5 and the DEV from May 29th?

liquidcms’s picture

a little surprised this is still open but maybe i am missing the finer points of this. i have a CSV with numerous 0's in it and these will not import.

i have 7.x-2.0-alpha5+24-dev (2012-Jul-12)

liquidcms’s picture

tried patch from #3 and on test of 10 records i now get 2 failing with this error:

SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'node-92' for key 'PRIMARY'

and these 2 records do not import

without patch all 10 imported; i just don't get any of the 0's imported

liquidcms’s picture

but must be a different issue as all my records have 0 values.. hmm.. odd

liquidcms’s picture

i think my issue with not importing 0's in is field_collection_feeds_set_target() (and fixed it)

jjclint’s picture

I've tried applying the patch on #37 but I get a patch failed error message I'm guessing it's because I'm using feeds latest version, should I try and apply the patch manually? Where does this issue stands currently? It's been open for a while now and it seems to really hinder a lot of the progress made on feeds and it's ability to update nodes from a number of different importers.

millaraj’s picture

My specific use case is pulling content in from one xml feed, storing it and then trying to pull extra information in from another feed to enhance the existing data. Unfortunately because I'm not mapping some of the fields it is removing content existing in the already populated fields when importing the second feed.

twistor’s picture

#55, That use case is already supported, you just need to set "Update existing" in the processor settings.

Following.

mksweet’s picture

Confirming that patch #3 worked for me on 7.x-2.0-alpha5.

mErilainen’s picture

For me not, I have 7.x-2.0-alpha5+43-dev. I get the following error:

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')) AND (field_data_field_email0.deleted = '0') AND (field_data_field_email0.enti' at line 1

Investigating further...

EDIT: My bad, it was some feature which doesn't work with the patch for some reason.

cthiebault’s picture

Here is patch #3 for release 2.0-alpha7.

cthiebault’s picture

Here is patch #15 for release 2.0-alpha7.

akosipax’s picture

May I add that empty textfields in the import file should sometimes update the node/any entity's field into an empty value? Like in my case, I have product(entity) with a textfield called 'note'. In the CSV file, the note column can be empty or not. If it's empty, I want the field in the updated product to be empty.

akosipax’s picture

Issue summary: View changes

Updating tasks.

wuinfo - Bill Wu’s picture

Issue summary: View changes

Option 3: Reset field value: #1528784 proposes empty values should reset the field is same as second option.

wuinfo - Bill Wu’s picture

Issue summary: View changes

Updated issue summary.

wuinfo - Bill Wu’s picture

Issue summary: View changes

Updated issue summary.

wuinfo - Bill Wu’s picture

As it in the issue summary:

Proposed resolution

For now, I propose we implement a switch behavior somewhere in the settings. When #860748: Config for mappers? gets in, we will have this as a mapper config. Each mapper, however, needs it's own implementation with proper logic.

Let's put options in feed map. By doing this, default behavior can be set for each field, not just at data type level by applying patches.

Give at least 2 options for each field:

  • Do not touch: skip setting the target value. In case of updating items, this preserves the previous value.
  • Import anyway: Overwrite the target value with blank, NULL or 0. For numeric values, this can be useful. However, in most cases, this will cause empty fields to have a label unexpectedely displayed.#1528784: Behavior when importing empty/blank values for numeric field

Default: Do not touch. (This is current behavior.)

  1. Do not touch(default)
  2. Import anyway

Road map:
1) change UI of feeds map. So we can have those options available.
2) Implementation in each data type (Date, Numeric, File, Text etc.)
3) Have other default behavior added for different data types.( Such as wrong date format for date field)

guillaumev’s picture

Here is patch #13 for release 2.0-alpha8

mikran’s picture

Issue summary needs update as #860748: Config for mappers? is in.

Uhkis’s picture

Status: Needs work » Needs review
FileSize
4.14 KB

Patch to #62 for latest git version. Includes UI for allowing empty values and implements the change in Text datatype.

Status: Needs review » Needs work

The last submitted patch, feeds-empty-behavior-1107522-65.patch, failed testing.

dman’s picture

Status: Needs work » Needs review

This failed because of 0 failures? WTF testbot?

dman’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

The last submitted patch, feeds-empty-behavior-1107522-65.patch, failed testing.

a.ross’s picture

Not because of 0 test failures, but because of 1833 PHP errors.

dman’s picture

Well, really just one. Database was unavailable - it seems.

a.ross’s picture

Err, what? The database wasn't unavailable, as that would result in critical failures when setting up the tests. But the tests ran normally...

wuinfo - Bill Wu’s picture

Status: Needs work » Needs review

Have done a manual test.

Applied patch in version 7.x-2.0-alpha7, tested and found it is working as I had expected.

After test, I have checked PHP log, Apache log and Drupal log. No error was found.

So set to need further review.

wuinfo - Bill Wu’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

The last submitted patch, feeds-empty-behavior-1107522-65.patch, failed testing.

wuinfo - Bill Wu’s picture

Status: Needs work » Needs review

I do not think failure of the test should block our way. Since the test result is not giving any useful info.

mikran’s picture

Uhkis started to work on improved version of the patch, I think PHP errors are solved already but some other improvements are not finished yet.

a.ross’s picture

Status: Needs review » Needs work

The patch is for 7.x-2.0-alpha7?? That's half a year old! Patch needs to be re-rolled against 7.x-2.x-dev, or at the very least tested against the correct version of Feeds! It's currently testing against 7.x-2.x-dev.

a.ross’s picture

Issue tags: +Needs tests

First of all, this sort of new functionality probably needs tests.

+++ b/feeds_ui/feeds_ui.admin.inc
@@ -618,7 +618,6 @@ function feeds_ui_mapping_settings_form($form, $form_state, $i, $mapping, $targe
   }
-
   if ($form_state['mapping_settings_edit'] === $i) {

Needless whitespace removal.

+++ b/feeds_ui/feeds_ui.admin.inc
@@ -627,9 +626,9 @@ function feeds_ui_mapping_settings_form($form, $form_state, $i, $mapping, $targe
     }
-
     // Merge in the optional unique form.

Idem

+++ b/feeds_ui/feeds_ui.admin.inc
@@ -661,7 +660,9 @@ function feeds_ui_mapping_settings_form($form, $form_state, $i, $mapping, $targe
+    if ($allow_empty_summary = feeds_ui_mapping_settings_allow_empty_summary($mapping, $target, $form, $form_state)) {

Why put this in an IF statement if the function call will always return a string, evaluating to true?

+++ b/feeds_ui/feeds_ui.admin.inc
@@ -848,12 +849,25 @@ function feeds_ui_mapping_settings_optional_unique_summary($mapping, $target, $f
+ * Per mapping settings summary callback. Shows whether a mapping is can
+ * be saved with a null value.

Function documentation must start with a single line.

+++ b/feeds_ui/feeds_ui.admin.inc
@@ -848,12 +849,25 @@ function feeds_ui_mapping_settings_optional_unique_summary($mapping, $target, $f
+    if ($mapping['allow_empty']) {
+      return t('Empty values allowed.');

This causes lots of PHP notices, where the 'allow_empty' element is missing from the array. This needs to either include a call to isset() or you must be able to assure that the element is always set.

+++ b/feeds_ui/feeds_ui.admin.inc
@@ -848,12 +849,25 @@ function feeds_ui_mapping_settings_optional_unique_summary($mapping, $target, $f
+

Too much whitespace.

+++ b/feeds_ui/feeds_ui.admin.inc
@@ -866,6 +880,22 @@ function feeds_ui_mapping_settings_optional_unique_form($mapping, $target, $form
+  $settings_form = array();

This is redundant.

+++ b/mappers/text.inc
@@ -19,12 +19,13 @@ function text_feeds_processor_targets_alter(&$targets, $entity_type, $bundle_nam
+    $allow_empty = $targets[$name]['allow_empty'];

Redundant.

+++ b/mappers/text.inc
@@ -34,7 +35,12 @@ function text_feeds_processor_targets_alter(&$targets, $entity_type, $bundle_nam
+  foreach ($source->importer->processor->config['mappings'] as $mapping) {
+    if ($mapping['target'] == $target && $mapping['allow_empty']) {
+      $allow_empty = TRUE;
+    }

This callback has a fifth argument, the mapper configuration. Use that instead of this awkward loop.
http://drupalcontrib.org/api/drupal/contributions!feeds!feeds.api.php/function/my_module_set_target/7

wuinfo - Bill Wu’s picture

@a.ross

This patch is not for 7.x-2.0-alpha7, but it is still applicable to 7.x-2.0-alpha7 version. Here is git message I caught when applying the patch.

$ git apply -v feeds-empty-behavior-1107522-65.patch
Checking patch feeds_ui/feeds_ui.admin.inc...
Hunk #1 succeeded at 624 (offset 6 lines).
Hunk #2 succeeded at 632 (offset 6 lines).
Hunk #3 succeeded at 666 (offset 6 lines).
Hunk #4 succeeded at 841 (offset -8 lines).
Hunk #5 succeeded at 872 (offset -8 lines).
Checking patch mappers/text.inc...
Applied patch feeds_ui/feeds_ui.admin.inc cleanly.
Applied patch mappers/text.inc cleanly.
Uhkis’s picture

Status: Needs work » Needs review
FileSize
6.58 KB

Updated patch for #65, includes 2 simple tests.

a.ross’s picture

That looks a lot better. But you removed the call to the below function. I suggest to restore that bit.

+++ b/feeds_ui/feeds_ui.admin.inc
@@ -848,6 +848,18 @@ function feeds_ui_mapping_settings_optional_unique_summary($mapping, $target, $f
+  if (!empty($mapping['allow_empty'])) {
+    return t('Empty values allowed.');
+  }
+  else {
+    return t('Empty values ignored.');
+  }

As for the function itself, I suggest changing this to the following (so the if statement I referred to in the previous patch can remain):

  if (!isset($mapping['allow_empty'])) {
    return;
  }

  if ($mapping['allow_empty']) {
    return t('Empty values allowed.');
  }
  else {
    return t('Empty values ignored.');
  }
mikran’s picture

Issue tags: -Needs tests

That looks a lot better. But you removed the call to the below function. I suggest to restore that bit.

It's still called in form of:

+ 'summary_callback' => 'feeds_ui_mapping_settings_allow_empty_summary',

As for the function itself, I suggest changing this to the following (so the if statement I referred to in the previous patch can remain):

As the summary callback is now invoked only when it's defined in the target there is no need to check if it's defined.

a.ross’s picture

Status: Needs review » Reviewed & tested by the community

Well caught. I see that the settings form is now also a callback, which is more appropriate. This looks good to me, as far as I'm concerned this is RTBC

vaccinemedia’s picture

FileSize
265.69 KB
490.43 KB
254.91 KB

Has this been resolved yet? I've just imported a load of products into a commerce installation and have blank terms in my taxonomy vocabularies (see attachment) which are being selected and therefore creating empty dropdown selectors (see other attachment) on the product display and I can't delete them! (see final attachment)

johnv’s picture

It will not help your import, but you CAN delete the term by first filling in the title and save, then delete.
(Which is strange behaviour, since you CAN delte driectly from a delete link)

wuinfo - Bill Wu’s picture

@vaccinemedia Patch at #81 did not fix this. I think we will put it to a separate issue.

We need implementation of this in each data type. Taxonomy is one of them.

refer to #62

kingandy’s picture

Title: Expected behavior when importing empty/blank values. » Behavior when importing empty/blank values for text field

There's already a separate issue asking about blank Taxonomy fields - #1668186: Taxonomy terms auto-created with empty name. - I think that should be used to develop this feature for the Taxonomy Term data type. Otherwise, that issue will need to be marked as duplicate of whatever issue gets created to build it...

(Changing title to reflect data type specificity)

a.ross’s picture

The scope of this issue is a bit unclear. It appears to be a meta issue when you look at the description, but it contains a patch for text fields.
Perhaps a new meta issue can be created and the scope of this issue can just be the text field.

a.ross’s picture

Issue summary: View changes

Updated issue summary.

a.ross’s picture

I went ahead and took the liberty to create a meta issue, separating it from this field-specific one.

ownage’s picture

I applaud everyone's efforts in this to date, but how are we sitting with this "Text Field" issue right now?

Have any of these "empty field" patches for text fields been committed to the most recent 7.x-2.0-alpha8 (yet alone dev)?

I need to update my ~6k nodes ASAP and have upgraded to the newest alpha8 (in hopes that there is a current version of what originally worked for me in patch #3).

gapa’s picture

I have sucessfully applied patch from #63 to latest dev version (7.x-2.x-dev 2013-Jul-06) and this did solve my problems.

ownage’s picture

Same with me! Thanks for the update.

wuinfo - Bill Wu’s picture

Title: Behavior when importing empty/blank values for text field » Frame work for expected behavior when importing empty/blank values + text field fix

Since patch at #81 is good for the frame work of all the data type, I think we should not change the switch the original issue title. So, I change the title and description of the issue back.

Following issue can be solved afterward.
#1528784: Behavior when importing empty/blank values for numeric field
#1668186: empty content taxonomy cells imported as taxonomy terms with empty names

wuinfo - Bill Wu’s picture

Issue summary: View changes

Updated issue summary.

wuinfo - Bill Wu’s picture

Issue summary: View changes

Since we have patch already, let's keep this issue solved and committed.

ditcheva’s picture

FileSize
28.62 KB

Hi everyone,

I also really need the ability to update some of my fields (text fields) from containing text to being emptied out through imports. I just tested patch #81 with several imports of 2,000+ records each, and it works beautifully.

It provides an option to adjust each of the fields you're importing in the mapping tab. By default, if the import field is blank, the field in your database is not affected. However, you can specify that an empty field in the import *should* overwrite your fields to be empty. How cool!

Here is a screenshot with the new and improved options:

Feed import improvements

I'd like to encourage any module maintainers to apply this patch when possible. Since it leaves the default option just as it has always been, it should not disrupt users' current workflow.

Michsk’s picture

For the files (https://drupal.org/node/2049341 & https://drupal.org/node/1047894) , we should have a option, 'If empty, keep existing value'.

Michsk’s picture

FileSize
4.58 KB

Here's a start on the file section, tough i'm not sure it's te way to go. We do a node_load to get the node with the revision, that way we can get the currently used field. The alt en title still get removed, haven't done that yet.

I'd have to start working with patches, so sorry that this is not a patch.

johnv’s picture

@ditcheva, #95, thanks for the screenshot!
@Lasac, #96, The option in patch #81 should do just that: "leave current value untouched, if new value is empty".

The wording of the checkbox and/or the summary should be clearer. I wouldn't know what means "empty value alowed".
Here is a proposal, using radiobuttons (since I could not make it very clear in one line, and this allows easier expansion to more options) The same wording can be used in the summary:

(o) Empty new value leaves current value untouched
( ) Empty new value clears current value
Michsk’s picture

@johnv: thats what the new file.inc does.

Michsk’s picture

Issue summary: View changes

Updated issue summary.

johnv’s picture

@lasac, OK. The summary and #94 point to separate issues for other field types, that can use this 'framework', keeping this issue only for textfield as an starting example. For Files you can use #2049341: Images are deleted when node is updated but mapped field is empty to post your patch.

johnv’s picture

Below suggestion probablye does not belong to this issue, but regarding to multi-value fields (files, text/numeric listfield), the following options are needed:

(o) replace current value with new value (clearly the default)
( ) append new value(s) to current value(s) (for multi-value fields)
johnv’s picture

Title: Frame work for expected behavior when importing empty/blank values + text field fix » Framework for expected behavior when importing empty/blank values + text field fix

See below code: the callbacks are overwritten. When trying to implement a new option, only the summary/form of the last option is used. This does not happen for the text-field in the patch, but it does happen in the taxonomy-patch from #63.
I see some alternative options: 1) The name of the callback must be more generic, or 2) the form_callback/summary_callback must accept an array of functions, of 3) the new callback must call the original callback.

@@ -20,11 +20,15 @@ function text_feeds_processor_targets_alter(&$targets, $entity_type, $bundle_nam
   foreach (field_info_instances($entity_type, $bundle_name) as $name => $instance) {
     $info = field_info_field($name);
 
+    $allow_empty = isset($targets[$name]['allow_empty']) ? $targets[$name]['allow_empty'] : 0;
     if (in_array($info['type'], $text_types)) {
       $targets[$name] = array(
         'name' => check_plain($instance['label']),
         'callback' => 'text_feeds_set_target',
         'description' => t('The @label field of the entity.', array('@label' => $instance['label'])),
+        'allow_empty' => $allow_empty,
+        'form_callback' => 'feeds_ui_mapping_settings_allow_empty_form',
+        'summary_callback' => 'feeds_ui_mapping_settings_allow_empty_summary',
       );
     }
   }
johnv’s picture

Status: Reviewed & tested by the community » Needs work

Also, in hook_feeds_set_target($source, $entity, $target, $value, $mapping), the new/last parameter $mapping is not passed. I tested this for both text and taxonomy_term.

ditcheva’s picture

Hi everyone,

I'm attaching a brand new patch, that should apply to the latest 7.x-2.x dev branch, addressing some of the issues raised above.

I completely agree that applying this behavior to additional fields types should be a separate issue. That way we won't allow scope creep of this ticket to keep these changes from being applied.

(As a side note, I've been using feeds (with a .csv file importer) for over a year, not realizing that my fields were not being overwritten by the blank values in my import file. We just recently found out and were shocked. These options will be very welcome and - as I mentioned in my first comment - will not disrupt any current users, since the default value will be what it's always been (blank text field values do NOT overwrite, but retain the current value you have in the database).

Here are my changes, per @johnv's comments
1. The admin UI settings for the new empty-field option are now radio buttons (not a single checkbox) with more descriptive names
Feeds empty field import improvements
2. I tested to make sure the $mapping parameter was passed, and it actually was passed for all my text field tests
Feeds empty field import improvements - mapping parameter

I didn't quite get the issue in comment #103 because the naming convention for the new 'empty field' options mirrors the current 'unique id' field setting pretty closely. So I wasn't sure what a better name may be. I left that out, but would be happy to tweak more if needed.

Again - I've tested this several times, and it works just as expected. If others could test and confirm that it works, that would be great!

wuinfo - Bill Wu’s picture

What is "[32m+[m[32m" ? It appears in front of each added line?

a.ross’s picture

look like spaces? Odd, must be a strange diff format

ditcheva’s picture

That does look strange... must be the diff format, but if you look at the patch all the way through, it appears just on the lines that are the *newly added test file* in the patch. The modifications of existing files further down don't look that way...

In any case, it passed testing, so I'm thinking it's OK.

Do you all have any other feedback or did you get a chance to test?

dman’s picture

Those are ANSII escapes for color codes. If you examine it (try deleting characters) there is an unprintable character hidden before each [.

[1m is 'set text to bold'
[32m is 'set foreground text to green'
[m is reset to normal

So that looks like a result of a syntax highlighter popping up where it should not.

What I thought was surprising was that the patch applied cleanly and tested OK!
What's actually happening is that changes to both feeds_ui.admin.inc, text.inc appear TWICE in the file. Once colorized, once not.
It seems that the second chunk was used.
Also, based on the patch manifest, it seems that tests/feeds/content_changed.csv was added twice.

Altogether a very fishy file.

Edit: Heh, copy&pasting the escape characters messed with my forum post.

ditcheva’s picture

Ooops. Well I definitely don't want to be contributing any 'altogether very fishy files'. :-)

I'll redo the patch tomorrow if someone doesn't beat me to it! And hopefully that will encourage others to test this awesome new functionality out too...

ditcheva’s picture

OK, here it is! Updated patch that doesn't look strange. I tested this one too, let me know what you think.

Again, just as I said in comment #104, this patch adds the ability to specify what to do with blank text field values in your import
- a blank value in the import file can leave current values as are (default behavior)
- a blank value in the import file can clear out the current values from your db.

Thoughts? Improvements? Agreement that this has been reviewed and works?

Thanks, everyone!

Status: Needs review » Needs work

The last submitted patch, feeds-empty_text_field_value_imports-1107522-110.patch, failed testing.

ditcheva’s picture

Status: Needs work » Needs review
FileSize
7.44 KB

Ooops, I see the issue. Let's try this again.

a.ross’s picture

I haven't thoroughly reviewed this, but I did notice that the patch contains tabs.

ditcheva’s picture

Cool. I can definitely re-roll the patch, but would love to see if there is any other feedback on the functionality/code before I do that.

mbatterton’s picture

This patch is great although it only work for text fields. Could it be extended to work for other field types, e.g Image, Integer, Decimal, Date, Float and Price?

kingandy’s picture

Date, Files, Numeric fields and Taxonomy are already being developed in different tasks - see the "Remaining tasks" section of the post header. Looks like Date is completed, the others are in various states of needing work/review.

a.ross’s picture

Why was the link to #2027583: [meta] Behavior when importing blank values removed? This issue is suffering from scope creep IMO.

ditcheva’s picture

It's cool - we're not suffering from scope creep too badly as long as we keep the other field types separate! Glad you guys are on the ball about that! :-)

So it looks like the patch worked for @mbatterton as well. Any other testers who'd be willing to try it out and mark as tested by the community or give feedback? I'd love to keep the momentum going here and to make a commit! We've been using feeds for years without realizing our blank fields weren't getting written, so this would be a huge improvement.

Again the patch should provide you options in the admin interface to change the overwriting behavior for blank fields for each mapped text field (and of course - it sticks with whatever option you've chosen during the import):

Text field overwriting behavior for blank fields

ArtActivator.com’s picture

Hello. I have a bug with duplicate terms on importing (https://drupal.org/node/2033519)
Lasac said, that I need to wait until framework issue is commited.

Will some patch from this issue help me on resolving this?

littledynamo’s picture

Just tested this patch and can confirm that it cleared the field!

Nice work!

ditcheva’s picture

Status: Needs review » Reviewed & tested by the community

Great. Three separate tests. Let's mark RTBC, and see if this first part can be committed!

manuelBS’s picture

Patch works perfect for me, thanks!

mparker17’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.52 KB
6.91 KB

Fixed whitespace...

blueblot’s picture

patch works for me too, thx

blueblot

ditcheva’s picture

Status: Needs review » Reviewed & tested by the community

Yay! Thanks for all the patch testing, you folks are great. And thanks for clearing up the spaces issue @mparker17, I meant to do that and forgot!

Marking RTBC again.

mparker17’s picture

Thanks @ditcheva! :)

ownage’s picture

Just a question--

I currently have the #59 patch installed (based on patch #3 for release 2.0-alpha7) and there seems to be one little issue. I was wondering if this issue would be present in the newest release of this patch.

The feeds behavior of the issue is, when "updating" a text field that used to have content but is now null, feeds does not rewrite the field as null-- the content stays there!

Please let me know if that issue would be present in the newest release of this patch.

franz’s picture

I think this kind of behavior is now configurable, but you can always set your importer to replace nodes, if you don't care at all about losing modifications...

twistor’s picture

Status: Reviewed & tested by the community » Needs work

"Empty new value leaves current value untouched." -- this is not happening in this patch. Feeds resets each entity field long before any mapping is called. And frankly, that's not a feature I'm willing to support.

I agree that some of the targets need more specific value checking, empty() in numeric doesn't work for instance. But, these are all trivial issues. What this patch is trying to accomplish is just strange. What should happen is that each field target ignores emptiness. They just validate the field value, or set it to NULL. Then, in entityValidate(), or someplace before save(), we should iterate over the fields calling the field's is_empty() callback and just remove empty fields.

ditcheva’s picture

Hey @twistor, I want to make sure I understand your comment properly to know what to address. I think this issue is very old, very active and *sorely* needed, so I want to make sure it isn't left in 'Needs work' for a long time just because of lack of clarity. :-)

You mention that 'Empty new value leaves current value untouched" is not happening in this patch. Could you expand on that? Are you saying that the patch doesn't provide the functionality for one of its options? (It only solves the issue for text fields at the moment - better to approve that first before solving terms and other field types etc).

Furthermore, when you explain how to solve this issue ("...in entityValidate(), or someplace before save(), we should iterate over the fields calling the field's is_empty() callback and just remove empty fields.") are you trying to say that, in your opinion, any future work on this issue shouldn't give folks the option for how to handle empty fields? The reason I ask is that there seems to have been a discussion throughout this issue, that was more philosophical than technical about how to treat empty fields. (This was before I found this issue myself in trying to deal with the problem of empty values being ignored, so I never chimed in then). Some seemed to think empty fields in the import file should be ignored, and others (I'm in this camp, as are the majority who have found this issue through the bug queue because this is the part that is missing) believe an empty value in the import file (or equivalent) should empty out the corresponding drupal field. Currently there is absolutely no way to empty out a field you've ever set before because all empties are ignored. :-( In any case, it was decided that both options should be provided because there were voices on both sides of the debate. With this comment, are you saying that you do not want to support the availability of two options? That how empty values are treated should not be an option in the admin UI, and instead, empty values should *always* empty out the corresponding drupal fields?

If that's the case, I understand, I just want to make sure it is clear so we can move this issue forward and actually fix this. I'm afraid that without clarity on what you'd be willing to commit, we may continue working in a futile direction!

Thanks again for getting back so quickly, and if you can clarify the above two questions too, we can keep going. :-)

twistor’s picture

My apologies for letting this run on for so long.

"Empty new value leaves current value untouched":
If you look in FeedsProcessor::map() at the top. You'll see that each field is emptied before the mapping is even started. So, yes, currently there is no way to leave a field untouched that has a mapping. Any fields that don't have a mapping are left alone, as long as you select "Update existing" rather than replace.

are you trying to say that, in your opinion, any future work on this issue shouldn't give folks the option for how to handle empty fields?
Yes. Empty fields should always be empty. I can understand the other side. But, it is a minority. Feeds can't be all things to all people. This could also be solved as an add-on module.

To elaborate on what I was saying before, targets shouldn't be so concerned about emptiness. They should always set a value. Either the correct, validated value, or an empty string. Then in another step, after mapping has occurred, we should loop through the fields and remove the empty values from them. The reason for this is that some targets handle mapping multiple columns of a field. Think of the link mapper. It handles the title, and url fields. In order for this to work properly, both fields need to have a value, even if it's empty, in order to keep the values aligned.

a.ross’s picture

That's unacceptable as I'm sure many people rely on the current functionality. There must be an option to preserve the old way of handling empty values. By the way, is there currently a difference between how missing fields and empty fields are handled?
Sorry missed the second comment. A solution based on feeds tamper would be fine I guess.

twistor’s picture

@a.ross Which part is unacceptable? You'll have to be more specific. I am only suggesting an improvement to the status quo.

a.ross’s picture

The part where the old behavior, where people already rely on, is done away with. The old behavior being that an empty import field does not clear the field in Drupal it's mapped to. Or at least, I experienced that with numeric fields, not sure if it was the same for text fields.

Edit: right, sorry. For text fields the field is emptied, but not nullified. I find it unlikely that people rely on that behavior, but maybe people still do. :/

twistor’s picture

a.ross, if you experienced that, then that would be a bug. I'm assuming that by people, you mean yourself.

a.ross’s picture

I'm sorry, I was kind of confused. I came here because I had an issue with an empty numeric field; the field would be left untouched. This issue (the original report) is different and in fact incompatible with Drupal's validation for text fields, so this is actually a rather straightforward bug. Adding an option for how to handle an empty text field should IMO be a separate issue.

wuinfo - Bill Wu’s picture

It is a little bit confusing. But I believe we are still on the right track.

The patch provide 2 options in dealing with empty field.
1) Empty new value leaves current value untouched.
2) Empty new value clears current value.

What if we change the words like following? Will it be more easy to understand.
1) Do not deal with empty value (or: Not enforcing empty value)
2) Enforce empty value

This is on the Feeds mapping, each data type still need to implement the behavior when enforce empty value. (eg. text can be "", number can be 0 etc.) This is what #119 is waiting for.

a.ross’s picture

Yes, that's what we've been talking about in this issue, but it isn't directly related to the original issue. I noted before that this issue is suffering from scope creep :) maybe I'm the only one who's confused but still better to keep things separate.

johnv’s picture

I think what twistor is trying to say is this: "Since all values are cleared from the target before we enter the mapping, the current value will ALWAYS be empty."

I (used to) have a use case for this patch, and have implemented it, but now I am puzzled (too).

ditcheva’s picture

@johnv, I'm confused because I've tested the patch multiple times myself - with imports of between 6 nodes all the way to 2,700 nodes at a time. It always works - both both admin options of how to treat empty values. We have several other manual tests all saying it works too. The patch does what it says for text fields. So I'm confused - until now nobody had mentioned a failed test. Could it be that you or @twistor are testing it for other types of fields (non-text fields)? If not, could you expand on which part doesn't work? I feel like I'm misunderstanding something or don't know what is meant by 'current value' - the current drupal field or the field being imported that is trying to replace it?

Thank you very much again!

As I said earlier - I am not sure what the use case for ignoring empty fields would be - that's the current behavior folks are saying we should preserve (could anybody explain a good use case for this?). In my view, the most important part is to be able to clear out the values of drupal fields with empty import fields when you need to.

dman’s picture

@ditcheva - since you ask ... a simple use-case is for merging incomplete data into the CMS, where it may be corrected or curated, and then updated later. I've had a number of cases like this.

An almost-real-world example (the truth is worse) :
We pull staffing information from a CRM sort of source. Names, contact info etc that then gets published in a staff directory.
Information we get from the CRM is patchy and incomplete, but important bits (like are they still employed) are the most up-to-date.
Other bits, like the freetext 'biography' blurb or the 'headshot' are irregular and mostly missing, but better than nothing most of the time.
NOW:
On the website, the content editor can go and edit the profiles, fix up formatting in the bios or upload a better headshot suitable for the team pages.
THEN:
Updates from the CRM get pulled in periodically. New staff members appear, phone numbers change, people leave, our CMS data gets updated.
*except for the stuff we have started maintaining locally*

That is the case for

If I already have a value, and the mapped feed source is empty, please don't delete my value.

We've done similar with pure node content - Pulling an RSS from an existing site a client was adding content to while building a full new site in the background. Sorta continuous-integration scenario. We get their data, we *may* curate it a little (tag to the new IA) whilst at the same time importing their live content and tag edits as they happen. On updates, the merge rules included:
content=overwrite,
tags=merge,
menu=don't touch,
attached images=add if new, don't touch if existing (we sometimes added new or better images on the new site where they did not exist on the old)
... etc.
Thus, the configurable options that let me define just a few of these rules would help out a lot.

wuinfo - Bill Wu’s picture

a simple use-case is for merging incomplete data into the CMS

regarding #141, I think this patch is handling the empty/blank values. It is done before it starts merging. Only empty text is implemented in this patch. Others data types are waiting for the commit of this patch. They are listed at top.

This patch is to find out the whether the empty/blank values should be used or now. If user decide to use the empty/blankvalue, then go to next stage which is merging with existing data.

ditcheva’s picture

Thank you all very much for explaining the use case for keeping an option to ignore empty values... That's probably why the decision was originally made to include both options...

I think to move forward, and either get the current patch committed or improved to be committed, it would be useful to get the other part of my question from comment #140 answered - the part about which part of it isn't currently working. Anyone have any insight into that?

johnv’s picture

@ditcheva, here is some update from some others issues (which are all mentioned in the summary):
#1668186: Taxonomy terms auto-created with empty name. has the same problem as OP (adding an new empty value instead of discarding it) + a simple solution. In that respect, this issue is way out of control ! Instead of a simple fix, a whole new system is created. (But i'm glad it's there)
Such a simple change would be sufficient for OP.

The other issues are a result of the fact that Feeds clears all values that are in the Definition of the Importer.
#2049341: Images are deleted when node is updated but mapped field is empty has an implementation to add new values to a multivalue file field. I adapted that patch for multivalue taxonomy terms.
It also contains a hack to preserve the old values, without reloading the Entity.
#1047894: Behavior when fields/columns are not in import file: do not clear field, but leave field untouched. has a change that only clears all values that are in the actual import file (that may have less columns as the definition.)
IMO Feeds does not need to clear the values, and may leave that up to the mapper, but I understand that such a change will break all custom mappers :-(

MegaChriz’s picture

So, if I understand the issue well, with the patch applied the following happens:

  1. When field is in mapping, but not in source:
    Target field is emptied.
  2. When field is in mapping and in source, but source is empty:
    Target field is emptied.

And the exepected behaviour should be:

  1. When field is in mapping, but not in source:
    Leave target field untouched.
  2. When field is in mapping and in source, but source is empty:
    Target field is emptied.

Following twistor's comments in #129 and #131 I translate that as:
"Whether or not the target field should be left untouched, shouldn't be configurable. The target field should always be emptied when the value in the source is empty and always be left untouched when the field in the source does not exists."

If this is the way to go, then the next patch should do the following:

  1. Remove the mapping configuration that was available in previous patches.
  2. Make sure that when a field does not exists in the source, the target field is left untouched.
  3. Make sure that when the field in the source has an empty value, the target field is emptied.
  4. This behaviour should only be applied for text fields, other field types should be dealed with in follow-up issues (supplied in the issue summary of this issue).

When the story above is confirmed, I'm willing to work on a new patch with tests.

Summit’s picture

Hi @MegaChriz, spot on! That should, in my opinion, be the wanted behavior! Not there untouched, There is emptied.
Greetings, Martijn

twistor’s picture

#145,

Make sure that when a field does not exists in the source, the target field is left untouched.

This is not going to happen.
I understand the use case, but it's minimal, and it will break current assumptions. The assumption being that the state of the feed is exactly reflected in the state of the entity. If it doesn't exist in the feed, it doesn't exist in the entity. Whether or not the feed has a value, vs. contains an empty value is too delicate a distinction to make, and it's completely left up to the implementation of the parser.

twistor’s picture

If you can make the distinction of non-existent vs. empty then you probably have control of the feed and can just put the value in.

MegaChriz’s picture

Okay, making the distinction of non-existent vs. empty is not possible for a generic solution, because it completely depends on the implementation of the parser.

@twistor
The status of this issue is set to "needs work". If the proposal in #145 is not achievable, then what's the next step for fixing this issue?

Michsk’s picture

Personally, would like to suggest to do something similar as in: https://drupal.org/node/2049341.

There i send a copy of the current field to the mapper so if needed it can fallback to the current value. That way it's not a problem that feeds unsets the field in the entity and the mapper does not have to do any kind of magic to get the current field value since it's already been passed to the mapper.

loker’s picture

The patch in #63 worked like a charm for me...

ditcheva’s picture

I'd just like to poll again what the status of this issue is. It's been over 2 years that the bug was first reported.

If we use this feeds module to import nodes with text fields and later want to clear out any of those text fields in future imports, we can't.

Some of my web users have resorted to putting in the words "none" to remove text, because there is absolutely no way, with the current module, to clear out a text value.

It just looks so silly and unprofessional on our site to have node fields that say 'none' simply because we cannot import an empty value!

The patch I had in #125 has been tested numerous times and works for me. I feel like I'm left with two options: use my own patch and have a feeds module that has been hacked/does not match the official module here (so I can't do updates on it) or keep putting the words 'none' in my text fields that should be empty, because otherwise, their value that is no longer appropriate remains. Both of these options seem ridiculous, but this issue is also not moving forward in any meaningful way.

Does anyone else have any ideas? Is there a response to @MegaChriz's question in #149? Or does someone have any other work-around we could use in the meantime?

twistor’s picture

@ditcheva, The patch in #123 should not solve your problem at all, and if it does, it's not the point of the patch. As far as I can tell, you should not be seeing the behavior that you are. You always have the option of creating a custom mapper, rather than patching Feeds.

That said, I think all of this is way too complicated. Field mappers shouldn't have to worry about what kinds of values are empty. Fields already know how to check this. On that note, people might be interested in
#2093651: Simplify target callbacks. which integrates a fields empty callback and remove all of the custom checking.

ditcheva’s picture

Hey @twistor, I agree that I should not be seeing the behavior, but I certainly am, and I thought that's the entire purpose of this bug report. Perhaps I'm confused and the folks here are seeing another type of problem...

In any case, I just went and re-checked on a simple drupal install and Feeds 7.x-2.0-alpha8 (latest) and tested everything again. The bug is there.

See if you can reproduce it: With a node importer set to replace nodes upon future feed imports and the simplest of import .csv files, I created three nodes with three text fields.
---------------
Name;Job title;Bio
Boriana Ditcheva;Web developer;Lorem ipsum dolor sit amet, consectetur adipiscing elit.
Someone Else;Other job;Lorem ipsum dolor sit amet, consectetur adipiscing elit.
Test person;Data Analyst;
---------------

The three nodes are created beautifully! Then I deleted two of the fields from the import, and indeed, those fields REMAIN in the original nodes - i.e they are not updated or replaced or removed: (Note below that Boriana's Job title is deleted and 'Someone Else's' Bio is deleted):

---------------
Name;Job title;Bio
Boriana Ditcheva;;Lorem ipsum dolor sit amet, consectetur adipiscing elit.
Someone Else;Other job;
Test person;Data Analyst;
---------------

So - that's the bug I'm experiencing, and it's certainly a bug. Those fields should no longer be there. I can reproduce this behavior every time, and on multiples of my sites, content types and importers. That behavior is definitely fixed with the #123 patch, though I understand it may not implement things properly, in which case I would definitely not push for it. So - would it be better for me to open up a brand new bug issue if I'm seeing this? I'm getting the sense that I misunderstood what this thread is about..

twistor’s picture

@ditcheva, Now that's a bug report! I haven't tested it yet, but yes please open a new issue.

MegaChriz’s picture

If the report in #154 should be a new issue, then what is this issue about? I may be wrong, but I thought this issue was about fixing the behaviour that target fields were not overwritten when the source for that field was blank (or maybe this issue wants to go further then that?). And doesn't the patch in #2093651: Simplify target callbacks. proposes a fix for this issue? (O.K. maybe you could call it a side effect that the patch in #2093651: Simplify target callbacks. possibly fixes this issue.)

twistor’s picture

@ditcheva, would you please test the patch in #2093651: Simplify target callbacks.?

@MegaChriz, I feel for you. I'm not entirely sure what the goal of this issue is. I'm trying to break it up into bugs that exist and features that people want. It seems to me that this issue spans too many things. Re: "fixing the behaviour that target fields were not overwritten when the source for that field was blank". #2093651: Simplify target callbacks. should fix that for most/all cases. It's basically me taking a different approach to solving it generically.

It basically comes down to two different expectations about how empty fields should be handled that are getting confused into one.

hcethatsme’s picture

I'm having the same issue as ditcheva with XML imports, and the Simplify target callbacks patch does fix it--just tested. Thanks, @twistor!

ditcheva’s picture

Yaaaaaaaay! So glad you pointed us in that direction @twistor.

I also just tested the patch at #2093651: Simplify target callbacks and it works! It totally solves the issue I described above. I'll add my comments there too and follow that issue to see when it will be committed (instead of this one). I think many of the folks here may simply need that fix.

MegaChriz’s picture

@twistor, #157

I think it's getting more clear to me what is happening in this issue (too bad I stepped into it quite late). Let me summarize:

The problem is that for some target fields the field doesn't get overwritten when that value for that field is blank in the source. There are multiple solutions proposed for that problem. One of them is simply and straight forward, the others go further then just fixing the problem by also introducing new features or API changes. None of the solutions proposed seem to be perfect, they all have cons.

Proposed solutions (so far)

  1. The simple approach
    Is by just removing the empty() check at the beginning of the target implementation. Similar to the patch in #63.

    Pros

    • Doesn't change much code. Simple fix.

    Cons

    • Installations that rely on the current behavior (expecting that empty/missing values in the source doesn't overwrite targets) will break. See #141 for an example.
  2. Provide a mapping option for how to deal with empty values
    This approach was introduced in #37 and that seemed the way to go for a long time. If I'm not wrong, this solution was proposed to fix the negative side effect of solution #1, namely that some people currently using Feeds in production expect that empty values are always skipped. The latest patch following this approach was posted in #123.
    The patch was marked as "needs work" by twistor in #129, because a part of it didn't exactly do what it said.

    Pros

    • Fixes the negative side effect of solution #1.

    Cons

    • Introduces more overhead, cause all mappers (or at least most) suddenly will need to provide this option to keep things consistent.
    • Current patch doesn't seem to do what is says it does.
    • It's still up to the implementation of the target to handle things correctly.
  3. Only make target fields empty when they exists in the source
    Proposed by me in #145. When the field doesn't exists in the source, leave target untouched, otherwise empty the target. This solution is not achievable, because the distinction of non-existent vs. empty can not be made for a generic solution.

    Pros

    • Fixes the negative side effect of solution #1 (though maybe partly).
    • Would have less overhead as solution #2.

    Cons

    • Generic solution not possible with current architecture.
    • Wouldn't work for some parsers.
  4. Simplify target callbacks
    Proposed in #2093651: Simplify target callbacks.. This solution moves the decision whether something is empty or not one step earlier in the process, out of the target implementations. The implementations of the targets will get simpler and more standarised and will produce less overhead.

    Pros

    • Target implementations get simpler and smaller.
    • Standardisation of how the target implementation will receive the value. Target implementations will no longer need to perform common checks like if they receive a single value or multiple values.

    Cons

    • It's an API change. It can break modules that extend Feeds or provide integration with Feeds.
    • There could be some inconsistency between target implementations for Field API fields as taxonomy and file fields need to be handled differently. (I have yet to verify if this is true.)

It seems to me that solution #4 is nicest one, though I have yet to check out how that exactly works and if the negative side effect that solution #1 has, is addressed (though I would also be fine if the change in behavior is clearly documented). It seem like it does by providing an additional mapping callback called 'post_process'.
If we would go for that solution, we should not forget to document the API change. I suggest to start using change records for this (as Drupal core does). (Side note: we then should also add a change record for the change in #1711648: Abstract bundle handling., as documenting that change was forgotten as reported in #2021087: Add a big notice about "abstract bundle handling" in release notes of 7.x-2.0-alpha8).

twistor’s picture

Status: Needs work » Needs review
FileSize
816 bytes

He, well I feel like an idiot. Figured out the real problem.

twistor’s picture

@MegaChriz, thanks for the synopsis.

The patch in #161 *should* actually fix the issue. Unsetting the field doesn't work as field API purposefully ignores fields if they don't exist, rather than deleting them.

I can't believe I haven't seen this before.

Making every field configurable as to how it handles empty values is outside the scope of Feeds. I do have an idea about how we could do it in a contributed module. #2093651: Simplify target callbacks. Should make it a bit easier as well.

twistor’s picture

Issue tags: +Needs tests

We should definitely add a test-case for this.

twistor’s picture

Sandbox for configurable emptiness handling.

https://drupal.org/sandbox/twistor/2094551

Anyone have any ideas for a good name?

Michsk’s picture

twistor: i think you actually nailed it with #161, this also imports images correctly, i had a ton of duplicate images, and now it does not seem to duplicate them any more.

hcethatsme’s picture

@twistor, thanks for the patch in #161 - tested and works on 7.x-2.0-alpha7! Any plans on when this might get released? We use Git submodules so want to avoid local patches if at all possible. Any other type of testing that could be done to help?

Thank you again!

Summit’s picture

Hi, This issue is a "mother" issue for other issues. Patch #161 works great!
Is it ok to set it to RTBC?
Greetings, Martijn

Summit’s picture

Issue summary: View changes

Added new file mapper in list : #2049341

killua99’s picture

Patch #161 is working.

When some field (source) come empty it fill empty! yey!

killua99’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
sin’s picture

#161 fixed issue with incorrect attached images removal during node updates if source field is empty. 7.x-2.0-alpha8. Thanks guys!

twistor’s picture

I've committed the patch from #161.
http://drupalcode.org/project/feeds.git/commit/95cc744

What else is left to do on this?

MegaChriz’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.7 KB

What else is left to do on this?

Writing a test case as noted in #163.

I started with writing a test for this issue a few months ago, but due to other things on my mind I forgot to finish it. Attached is what I currently got.

MegaChriz’s picture

I forgot to do a git add before creating the patch, so the patch in #172 misses two files.

Attached a new patch.

The last submitted patch, 172: feeds.test-case-fix-empty-fields-1107522-172.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 173: feeds.test-case-fix-empty-fields-1107522-173.patch, failed testing.

Mykolas’s picture

Feeds tamper "Filter empty values" plugin solves this problem.

MegaChriz’s picture

Status: Needs work » Needs review

173: feeds.test-case-fix-empty-fields-1107522-173.patch queued for re-testing.

The test failed because a zero was treated as empty for a text field. Now that #2093651: Simplify target callbacks. is committed, let's see if the test still fails. The one single test method should be split up in two, by the way.

chrowe’s picture

@Mykolas
I don't see a "Filter empty values" plugin
Do you mean "Required field"? I assume this should do the same thing. (correction: this prevents the entire record from being imported, not just the one field.)

chrowe’s picture

The description says "Default: Do not touch. (This is current behavior.)" but I don't think that is the case any more. I think it used to be but has been changed to "Import anyway".

coredumperror’s picture

Chrowe is right. The default behavior of how Feeds treats empty inputs for fields which did not used to be empty is now different. It was years ago, but I think a toggle option for this behavior was discussed earlier in this issue. I think this would be a good idea to implement that, and to set the default to the original behavior. Without such a change, people who upgrade Feeds will suddenly start getting wildly different import behavior that deletes their fields.

gcb’s picture

I also need to have imports leave values alone on import. However, in FeedProcessor.inc, the map() function forces nulling without any way to prevent it other than overriding the entire function (which seems both silly and a maintenance nightmare). I'm attaching a patch that adds a simple check for a flag on the mapping configuration called "prevent_null_overwrite". This allows my FeedsProcessor implementation to set the flag when appropriate and handle the writing or nulling itself in setTargetElement().

It seems to me it would be a sensible default behavior to allow setTargetElement to handle this, but I haven't dug too deeply into the feeds module so I may be missing something. Regardless, if this solution might work for you, feel free to have a look at how I've implemented it over at project/redhen_feeds in the development branch.

gcb’s picture

The patch in #181 is against Dev. Here is a version against alpha-8

Status: Needs review » Needs work

The last submitted patch, 182: allow_processors_to_override_nulling_alpha8.patch, failed testing.

nickmaine’s picture

https://drupal.org/node/1107522#comment-5955580

Multiple value fields. If I import a csv with source column 'image url' and map it to a multi-value Drupal field, then import a different csv that has a new value for 'image url', the default behavior is to overwrite the original value. But if it is a multi-value field, I might want feeds to just 'add' a second value to the multi-value Drupal field.

How is issue #4 being handled ?

I have being trying to solve this issue myself.

dman’s picture

@nickmaine
I think the replace/append/merge question needs a different issue.
Basically, the default behavior will always be 'replace', as the data source is expected to be definitive at import time.
The best we can ask for here is "leave it alone if it's not defined".

Asking for new 'merge/append' behavior for *multiples* = a follow up feature request.

(in my experience with this sort of task, it's a lot harder to *delete* old records than to just add new ones. That's the real challenge.)

MegaChriz’s picture

Issue summary: View changes

I've updated the issue summary and pointed out that:

  • The behaviour in dev is now to always overwrite the target value, even when the field is missing in the source.
  • To ignore empty values, there is now the experimental module Feeds empty.
MegaChriz’s picture

Issue summary: View changes
Exploratus’s picture

So should we use #161?

MegaChriz’s picture

@Exploratus
The patch in #161 is committed to dev, so if you are using the latest dev there is no need to apply that patch. If you are on Feeds 7.x-2.0-alpha8 you could try to apply the patch, though I would recommend to use the latest dev instead as there was also a related fix committed (in #2093651: Simplify target callbacks.) and I'm not sure how Feeds would operate with only the fix from #161 applied on 7.x-2.0-alpha8.

Exploratus’s picture

Thanks. works great

christo snyman’s picture

Ok, so im a bit lost here. I read through all the comments but not sure which path I should follow. When I import blank values I do not want them to import a zero but want them to erase the value. In my scenario I have a price & old price and want new products without an old_price value to have no value and existing products must erase whatever value is in there. Please help out as I got super confused going through the comments (I have very little dev skills)

Thanks people

Nux’s picture

@christo snyman seems like you should use dev version. Current alpha version (7.x-2.0-alpha8) doesn't work well with empty values (the value is added as empty to revisions, but not really changed).

MegaChriz’s picture

Assigned: Unassigned » MegaChriz
Issue summary: View changes
Status: Needs work » Needs review
FileSize
18.46 KB

Finally picking this up again. Note that the fix for this issue was already committed (see #171), it only could use tests.

The attached patch is a follow-up on the patch in #173 and adds three test methods that test the following:

  • If values get cleared out when the field is in the source, but the value is empty.
  • If values get cleared out when the field is not in the source.
  • For some fields, if 0 is not considered empty.

The following test methods are added:

  • FeedsMapperFieldTestCase::testClearOutValues
    For field types text, number_integer, number_decimal, number_float.
  • FeedsMapperDateTestCase::testClearOutValues()
    For field types date, datestamp, datetime. Other than in the other tests, 0 is considered an empty value.
  • FeedsMapperTaxonomyTestCase::testClearOutValues()
    For field type taxonomy_term_reference.

Also the following assert method is added:

  • FeedsMapperTestCase::assertNoNodeFieldValue()
    To assert that a field with a certain value does not exists. This method is to used to assert that after a field should be emptied it no longer contains it's originally value. Maybe I remove this method in a future patch and explicitly check the field for an empty value instead.

This is still in work progress. Things left to do:

  • Test for link field.
  • Test for file field.
  • Maybe replace assertNoNodeFieldValue() asserts with an explicit empty field check.

@gcb, #181, I'm not sure if I understand what your patch does, but I think your proposal could better be handled in a follow-up.

MegaChriz’s picture

And here is the test for the link field. Note that the tests will currently fail, because apparently the fix from this issue (posted in #162) did not fix the whole problem in case of the link field. Though the link field gets successfully emptied, the field labels remain displayed on the node display:

This may be a problem in the link module (because it does not happen for text, number, date and taxonomy fields), so it would be better if this is further investigated first, but I think Feeds should try to fix it anyway if it is easy to do.

I'll check the link field further later. Next step for me will be writing a test for emptying file fields. I hope to have this ready next Tuesday (seven days from now).

MegaChriz’s picture

That's interesting. The tests for link field aren't executed at all. And that is because the test case isn't listed in the info file. Let's add tests/feeds_mapper_link.test to info file.
Tests should fail because of the issue noted in #195.

MegaChriz’s picture

And here is the test for the file field. I also included a fix for the issue with the link field reported in #195: in the target callback for the link field a call to _field_filter_items() is made, which will call link_field_is_empty() and as such filter out empty link field values. I tried first if a fix for #2379631: field_attach_validate() must be called before programmatic entity saves would also solve the problem as _field_filter_items() is also called by field_default_validate(), but this wasn't the case.

Note that not all tests are evaluated by the testbot, herefore I opened #2415283: Some tests are not executed by testbot. Thus this issue basically depends on that one now.

Status: Needs review » Needs work

The last submitted patch, 197: feeds-test-cases-empty-fields-1107522-197.patch, failed testing.

MegaChriz’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
35.28 KB
1.41 KB

Let's see if this fixes the failed assertion from the patch in #197.

Note that #2415283: Some tests are not executed by testbot should first be fixed in order for the testbot to run all the tests that are included with this patch.

Leeteq’s picture

@Twistor: Name suggestion for #164

OnkelTem’s picture

Follow-up on a feature to skip empty (or empty-after-rewrite) sources: #2426613: Add "NOT NULL" option to targets

MegaChriz’s picture

#2415283: Some tests are not executed by testbot is in, let's see if the tests from #199 all pass.

sonicthoughts’s picture

This blockee looks pretty solid esp for an alpha ... can we now release alpha 9 or are you also waiting for child issues?

twistor’s picture

It had to be re-rolled because of the extend mapping API issue.

Rather than special-case the link field, what about doing something like this? I think it will help with #2379631: field_attach_validate() must be called before programmatic entity saves as well.

That's a lot of tests!

Status: Needs review » Needs work

The last submitted patch, 205: feeds-test-cases-empty-fields-1107522-205.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
FileSize
522 bytes
36.12 KB

oops.

MegaChriz’s picture

_field_filter_items() and cardinality correction applied to all field instances?

The code in mappers/field.inc looks like a decent solution for all field targets. One question though: it loops through all field instances of an entity and applies the _field_filter_items() correction and the cardinality correction to all of them. Shouldn't Feeds only apply these corrections to the fields that are targeted and leave the rest alone? For example: if you have an entity with two fields and you only map to the first field, the corrections in mappers/field.inc are also applied to the second field, which you didn't map to.

I've looked through the tests once more and I made some tiny corrections to it.

twistor’s picture

That's probably true about not messing with fields Feeds isn't managing.

Status: Needs review » Needs work

The last submitted patch, 209: feeds-test-cases-empty-fields-1107522-209.patch, failed testing.

twistor’s picture

Status: Needs work » Needs review
FileSize
15.44 KB
47.34 KB

  • twistor committed c896b58 on 7.x-2.x authored by MegaChriz
    Issue #1107522 by MegaChriz, twistor, ditcheva, nielsonm, franz, Niklas...
twistor’s picture

Assigned: MegaChriz » Unassigned
Status: Needs review » Fixed

Thanks all!

Status: Fixed » Closed (fixed)

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

Morbus Iff’s picture

Are there any new developments, or new issues related to this? I have been unable to get it to work. Currently using Feeds 7.x-2.x-beta1, with Feeds XML XPath Parser 1.1 (feeds_xpathparser), and I am unable to get normal target text fields to be wiped out when the value is empty.

1. Process XML file with [Credentials]PhD[/Credentials]
2. Watch the value get assigned to an existing node.
3. Process XML file with [Credentials][/Credentials].
4. See the existing node get updated, but with no change to Credentials.
5. Process XML file with [Credentials]ABC[/Credentials].
6. See the existing node get updated, with a change to Credentials.

apmsooner’s picture

I also have not been able to get it to work with XML file using xpath parser when value is empty for a numeric target. The schema for field is set to numeric and not null => FALSE but anything that has a blank value gets skipped and reports error in feeds log like example:

SQLSTATE[HY000]: General error: 1366 Incorrect decimal value: '' for column 'n_factor' at row 1

Other values get imported just fine.

Here is detailed output from the log. I've also tried as a .csv importer and same result so numeric fields remain a problem for me....

SQLSTATE[HY000]: General error: 1366 Incorrect decimal value: '' for column 'n_factor' at row 1
Original item

array(
  'ndb_no' => '09383',
  'fdgrp_cd' => '0900',
  'long_desc' => 'Tangerines, (mandarin oranges), canned, juice pack, drained',
  'shrt_desc' => 'TANGERINES,(MANDARIN ORANGES),CND,JUC PK,DRND',
  'comname' => '',
  'manufacname' => '',
  'survey' => 'Y',
  'ref_desc' => '',
  'refuse' => '0',
  'sciname' => '',
  'n_factor' => '',
  'pro_factor' => '3.36',
  'fat_factor' => '8.37',
  'cho_factor' => '3.7',
)

Entity

(object) array(
  'vid' => '17307',
  'uid' => '0',
  'title' => 'Tangerines, (mandarin oranges), canned, juice pack, drained',
  'log' => 'Updated by FeedsNodeProcessor',
  'status' => '1',
  'comment' => '0',
  'promote' => '1',
  'sticky' => '0',
  'nid' => '17307',
  'type' => 'food',
  'language' => 'und',
  'created' => '1442946028',
  'changed' => 1442946033,
  'tnid' => '0',
  'translate' => '0',
  'revision_timestamp' => '1442946033',
  'revision_uid' => '1',
  'name' => '',
  'picture' => '0',
  'date' => '2015-09-22 12:20:28 -0600',
  'revision' => FALSE,
  'is_new' => FALSE,
  'timestamp' => 1442946033,
)
Leeteq’s picture

Status: Closed (fixed) » Active

So what is the real status here: is the issue fixed or not, in the latest -dev?

MegaChriz’s picture

Status: Active » Closed (fixed)

This issue is definitely fixed. For every field target that Feeds provides there is an automated test that tests if values are wiped out if an empty or if no value is provided and these tests are still passing. It is possible however that wiping out values does not work for mapping targets provided by other modules. Some modules may still check if the provided value is empty and then decide to do nothing. In such cases, Feeds can nothing do about that, you'll then have to create an issue for the module in question.

The issue that apmsooner had has been resolved in #2572861: Empty Numeric Fields cause SQL errors and was caused by a misunderstanding of the Feeds API.

The issue that Morbus has, sounds like an issue with the name field mapper, which Feeds doesn't provide. This issue should be handled in the Name Field issue queue.

The fix for this issue is included in the 7.x-2.0-beta1 version.

Leeteq’s picture

@MegaChris; thanks for the thorough clarification.

jomarocas’s picture

i'm no sure this working, i have a field date/iso.
and i have default value in today day, and the field is required, with a custom format d/m/Y, this is a field collection item and the importer is configured for a node have infinite fields, the date have a default value and working when add a new field collection item, but when importer dont working, i dont declare in the feed importer the field but i think always take the default value, why? thanks

troybthompson’s picture

What about boolean fields? When I'm importing nulls it's interpreting them as 0's even when I have the field not required. I'm using Feeds (link is external) 7.x-2.0-beta1+14-dev (2015-Oct-11)

gcb’s picture

I still need the ability to prevent nulling, and have upgraded to 2.0-beta1. So, here's a patch for that version to enable a config on your feed processor called "prevent_null_overwrite". It's pretty handy. I think it'd be cool to add, but I'll leave this ticket in "fixed" mode and just leave this here for whoever needs it...

mikemccaffrey’s picture

Updating @gcb's patch to work against the latest dev branch and beta 2, so you can enable a config on your feed processor called "prevent_null_overwrite".

demonde’s picture

Thanks for the patch in #223 and #224, but how do I apply such a config on my feed processor?

vbard’s picture

Module Feeds empty doesn't work with current Feeds version at the moment.

vbard’s picture

@mikemccaffrey, I can't find any config called 'prevent_null_overwrite', where should I seek it?

gcb’s picture

You can add it to the ConfigForm() function in your FeedsProcessor class. Example: http://cgit.drupalcode.org/redhen_feeds/tree/lib/RedhenFeedsContactProce...

vbard’s picture

Thank you @gsb, now I see!
And is there a way to add this setting to existing feeds processor, for example FeedsNodeProcessor, besides hacking it?

gcb’s picture

You could just extend feedsnodeprocessor with your own and only override that one function.

vbard’s picture

@gcb Thank you for that piece of knowlеge, I appreciate this a lot!
Although, I went my way and did some explorations on feeds_empty module. Here is a patch for it https://www.drupal.org/node/2323785#comment-11305767