Update existing doesn't reset targets that have real_target set.

I'm seeing this same weirdness. Or rather, the mappers are setup to do what is described above, due to this line:

$field = isset($node->$target) ? $node->$target : array();

which appears in a few mappers (including content.inc which imports basic CCK fields).

However, that line does not actually work in content_feeds_set_target, as by the time the node gets there, the target field has been removed from it (resulting in $field always being set to an empty array). Not sure where/how it's getting removed, but it is.

Interestingly, in the nodereference mapper I'm trying to get included (#724536: Mapper for nodereference field in Drupal 6), the target field isn't removed, so that line of code makes it that subsequent updates via feeds appends content, resulting in duplicate field values. My guess is this is what @marcvangend is also seeing.

So my question is the same: what is supposed to happen? My guess is that each feeds import should overwrite the field values with whatever is included in that import. Otherwise, it's impossible to ever remove field values via an import, as you'll just keep adding new (and duplicate ones).

Marking as major as this issue has the potential to cause (and for me is currently causing) data corruption.

Actually, data corruption is a critical issue. I'll gladly create patches to fix this issue, once I know what the expected behavior is...

The patch should explain.

Hmmm. The "replace existing nodes" doesn't suffer from this issue, correct? @joshua, the unsetting occurs in the map function of FeedsProcessor i think. So, this *shouldn't* be happening, because the field should be reset first... This merits further investigation.

So, currently it is supposed to delete all entries in the CCK field to which it is assigned prior to mapping, and if it doesn't in this case...it should.

@David,
Correct, it's not a problem for replacing existing nodes. If you look at the patch, you'll see where the problem is. It's more or less a typo.
The 'real_target' field is located at:
$targets[$this->id][$mapping['target']]['real_target']

but currently
$targets[$mapping['target']]['real_target']
is used which doesn't exist so the isset() call passes without error.

K, that makes sense. Hopefully either Alex or I will have time to test it soon, looks fine though and will probably go in.

I ran an updated import today using the nodereference importer in my other issue. Before this patch, it resulted in duplicate field values. After this patch, all was good. So looks good to me...

Woops, that was there while I was debugging to get rid of the notices.

let's get this sucker in... bug smash!

hrm, this needs to also check for whether 'real_target' is set in the mapping definition

Before this patch, here was my situation:

If a node has more than one value for a field; after importing, it was writing all the values to a CCK multi-value nodereference field. But when I re-run the importer ('update existing nodes' setting), it would create the existing values again, causing duplicate values. For example my field after multiple imports:

Nodereferences:
Title1
Title22
Title333
Title1
Title22
Title333
.....

But after applying the latest patch (#11), now it deletes all the values and adds just one value to this field. Example:

Nodereferences:
Title333

My desired behaviour is to update the field with the latest values in the latest file, deleting previous values. So after last import, it should be like:

Nodereferences:
Title1
Title22
Title333

While this doesn't work correctly for XML (with XPath parser module) as described on #12, it is working as desired on CSV files. But I didn't check if the CSV import was working correctly even before applying the patch...

This is the version I've been using for a while. No notices or warnings. If you look at where $targets is defined you can see how where the mistake is.

As a note, this isn't affected by the choice of parser. It's only affected by mappers that define real_target.

Patch above is for 7.x-2.x. Here's a patch against 6.x.

I just tested 6x-996808-17.patch and all fields were reset except Date fields. I am using Feeds 6.x-1.x-dev (2011-Jun-28), and Date Module 6.x-2.7. I also have the Node Reference patch installed. If you need any more info please let me know...

UPDATE: Cleaning of the Feeds stored fixed the clearing of the date fields. All fields that I had were indeed cleared. VERY NICE!!

Subscribing

I tested this and it seemed to work for me as well. Same situation as JoaoMachado. Node Reference patch included.

Sounds RTBC to me.

#17 worked for me.

Coming from #1188994: Accumulative product reference mapper

Setting to needs work to point something out that may affect this patch, sorry if it's a little late folks.

Clearly this patch fixes a bug/typo as the property checked is never there, but probably arises a new error, if the fields are not populated on update, what's the actual difference between updating a node and replacing it?, I'd say that if you want to wipe out a field and put your values you'd use replace, but update seems to me as the option to add more and keep the existing ones.

This is the code from the current field.inc mapping from feeds module:

$field = isset($entity->$target) ? $entity->$target : array();

So even feeds "core" is expecting values in $field when updating.

I'd appreciate someone to clarify this subtle difference between replace and update.

The difference between updating and replacing nodes:
When replacing nodes, Feeds checks for an existing node and grabs the nid. It then builds the node from the feed data(only), and saves it.
When updating existing nodes, feeds will load the existing node and then populate the fields it knows about.
The difference is that if you have fields on your node that are populated manually, not by Feeds, "Replace existing" will blow away the changes made, while "Update existing" will keep those fields.

It's really a performance issue, replacing nodes avoids a whole lot of node_load()s.

If you look at FeedsNodeProcessor::entityLoad() in 7.x you can see the difference pretty easily.

Technical differences are pretty clear looking at the code, there's no doubt of that, but my question was more what it should be. As default field mapper from feeds is expecting that target has data at some point I tend to think that update mode was originally thought to accumulate values but I may be wrong

Also see #1183408: Import into real_target

The reason that every mapper starts off with:
$field = $entity->$field ? $entity->$field : array()
Is to attempt to support multiple mappings to the same target. Accumulating in a sense, but not the way you're looking for.
source1 -> field a
source2 -> field a

Changing this back since pcambra found a solution to his issue.

Multiple source not working? Using 6.x-1.0-beta11 and patch: reset-real-targets-6x-996808-17.patch

I have a node type with 10 fields but with two sources. Each source handles different fields of the node type.
I use the Node Title as the GUID.

Source 1: Field1, Field2, Field3, Field4, Field5, Field6
Source 2: Field7, Field8, field9, Field10

When attempting to use the second source to update the node, it creates a duplicate.

Should I be using the dev version?

#29: This is a 7.x issue, perhaps you should file a separate one for 6.x?

Ay caramba!!! my bad! I will post it on 6.x

Joao

Tagging as release blocker.

committed to 7.x-2.x

Woot!

6.x-1.x

http://drupalcode.org/project/feeds.git/commit/c203396