Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Honza Pobořil’s picture

same problem

Row guid in feeds_item table is not filled by ids from source. Mappers are setup correctly.

Honza Pobořil’s picture

Update to dev version of Commerce Feeds fixes this issue on my case.

franz’s picture

Status: Active » Postponed (maintainer needs more info)

I see 2 different issues there.

For the first, can you tell me you setup the "Unique" checkbox for the GUID mapper? Also, what is the selected behavior in the Processor settings for existing content?

F.G’s picture

same trouble with alpha 5
i' have selected : Update existing nodes (slower than replacing them)

F.G’s picture

same trouble with alpha 5
i' have selected : Update existing nodes (slower than replacing them)

and checked unique target
xpathparser:0 GUID

franz’s picture

Status: Postponed (maintainer needs more info) » Active

I've used it without trouble before. This kind of bug happens when the guid is not filled properly. Using feeds xpath parser, you can enabled a checkbox to debug this target (xpathparser:0) and see if it's parsing the value correctly. Also, check the feeds_item table in the database to see if the guid column has proper values.

F.G’s picture

actually i can't access to database, but i can see the guid showed on my teaser in a field and it's exactly the same unique number and alpha code.

note : i used a content type to import alls nodes, because my import url is actually locked.

JonMcL’s picture

From my quick tracing of the code, it appears (to me) that the GUID value is not checked for uniqueness across different source nodes. So if you have your feed importer attached to a content type (i.e. "Feed"), there is a call to check of the incoming item has a unique GUID value, but it also looks at the source node's ID value.

Personally, I think this is a mistake. I think it would be wiser to say that the GUID should be checked for uniqueness across, at most, a common feed importer, but to ignore the source node ID of each URL being fetched. Or possibly to use the content type being used by the node processor, combined with the GUID.

My situation is like this:
I'm using one feed importer, with the JSONPath parser, to get Twitter messages from multiple search API URLs. I am bringing them all into one common content type (Tweet). The problem is that a given Twitter message could be present in the results of multiple search API URLs. I end up with the same message repeated, even though they share the same GUID.

The code that I'm questioning is:

$query = db_select('feeds_item')
      ->fields('feeds_item', array('entity_id'))
      ->condition('feed_nid', $source->feed_nid)
      ->condition('entity_type', $this->entityType())
      ->condition('id', $source->id);

(from FeedsProcessor->existingEntityID() )

Should 'feed_nid' be a condition in there? I'm suggesting no. If it's removed, then different source nodes, that use the same feed importer (represented by $source->id) could reject duplicate items.

But maybe there are more valid reasons to continue this check my one reason to exclude it?

..jon

JonMcL’s picture

Incase my suggestion above is valid, patch attached... for one whole line! :)

RoloDMonkey’s picture

littledynamo’s picture

The patch in 9 didn't work for me. My second Feed Importer created a new set of nodes rather than updating the first set of nodes.

I removed the following line as well, which seemed to do the trick:

->condition('id', $source->id)

New patch attached.

g089h515r806’s picture

Version: 7.x-2.0-alpha4 » 7.x-2.0-alpha8
Category: feature » bug

I have the same issue with alpha8,
it seems that @11 is the correctly sollution.

CHange it to bug report. I think this is a bug, not a feature report. GUID should be unique in Global, instead of in feed_nid.

sin’s picture

The same. Expected GUID to be unique across feeds importers. +1 for #11 to be comitted.

twistor’s picture

Title: Unique GUID not working properly » Allow GUID to be globally unique.,
Version: 7.x-2.0-alpha8 » 7.x-2.x-dev
Component: Miscellaneous » Code
Category: bug » feature
Status: Active » Needs work

We can't just change the meaning of the GUID arbitrarily. Yes, GUID means globally unique, but that's not how it's used in Feeds. There are lots of potential problems, what if different importers are creating different node types?

This needs to be converted into a setting, the default existing behavior shouldn't change.

crystaldawn’s picture

Status: Reviewed & tested by the community » Needs work

I can confirm that this is indeed a bug and the "GUID" field does not work as it should. If GUID is not meant to be a GUID then it should not be called a GUID. The patch in #11 works perfectly and should be considered since the stated purpose of the GUID is to be unique ALL the time, not just sometimes. Then a new "feature" should be implemented as #14 suggests that allows you to make GUID NOT globally unique and not the other way around since it's far to confusing and would result in this issue never being fixed which helps no one.

One way this could be accomplished is to keep this issue marked as a bug, implement the proper fix which is in #11, and then create a new issue for a feature that allows you to expand upon the GUID field by also tagging it with taxonomy, content types, etc. Basically any kind of entity available. This way you end up with a TRUE guid field by default (which is what I and many others expected considering it's name), and then add the ability to have a multi-field/entity GUID that can encompass not only the field itself, but also evaluate the node type or whatever you need which will in essence make it unique to just a certain content type as mentioned in #14. Either way, we need to close this issue by implementing the patch in #11 since this issue has been open for well over a year now, and there is no end in site if compromise is not found and ignore a working patch that obviously fixes the issue.

crystaldawn’s picture

Category: feature » bug
Status: Needs work » Reviewed & tested by the community
crystaldawn’s picture

Title: Allow GUID to be globally unique., » GUID not working properly
Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, feeds-unique_guid_not_working_properly-1539224-11.patch, failed testing.

crystaldawn’s picture

Status: Needs work » Needs review

This patch works great, but it looks like the simpletest cases dont like it.

crystaldawn’s picture

Status: Needs review » Needs work

The last submitted patch, feeds-unique_guid_not_working_properly-1539224-11.patch, failed testing.

twistor’s picture

Category: bug » feature

#15, I understand what GUID means. That names comes from the name in RSS. It's up to us to use it. I'm not going to break everyone's existing sites for semantic purity.

I'm not even sure I would accept a patch that allows this with a setting. It would be too easy to break things. As it stands:
Say I have 2 importers, Page importer, and Article importer.
Page importer creates a node with GUID = 1234
Article importer has a feed item with GUID = 1234
Article importer will then essentially convert the Pag node to an Article node on import.
Lots of things will be broken.

I would consider a patch that adds the importer as a condition, always, and has an optional setting to ignore the feed it. This setting will have to default to the current behavior.

twistor’s picture

Title: GUID not working properly » Add support for unique fields to be unique per importer.
johnv’s picture

@twistor,
I'd rather have the solution "Add support for GUID (unique field) to be unique per node type(entity bundle)."
Your example has two importers, mapping to two different node types. Indeed, you need 2 GUIDs.
OP has two feeds/importers, mapping to the same Node Type. In this case, you'd want one GUID, as explained in other issues, where basic data comes from one source, and extra data comes from another.

(There must be another issue regarding the same problem, but I can't find ATM)

twistor’s picture

Checking against a bundle gets more difficult.

Most users of Feeds don't have any control over the feed, thus can't control the GUID.

#661606: Support unique targets in mappers

JonMcL’s picture

I'm not as familiar with the code as I was when I wrote the one line patch at #9.

That said, I don't think the patch at #11 is the right approach. As mentioned at #22, you'll end up changing unrelated nodes during a second import.

But if I understand the module correctly, KEEPING source id in the query means that the GUID is still unique within a single source. So you can have multiple feed nodes that produce one type of content node and they will not produce duplicate content nodes. A second source will not step on the GUIDs from the first source. My assumption is that within a single source, the GUIDs will be unique.

Oh and as far as not breaking existing sites, while I don't think it will be an issue, if its a concern then it should become an option that is off for existing configurations, but maybe default to on for new ones. But that is of course for the module maintainers to decide.

blazindrop’s picture

Re: the title change in #23, the original poster is saying GUID should be globally unique (e.g. right now targets are unique to the importer).

"Add support for unique fields to be unique per importer." does not reflect the original poster's request; he is asking for the opposite. No?

sdrycroft’s picture

sdrycroft’s picture

sdrycroft’s picture

Status: Needs work » Needs review

The last submitted patch, 9: feeds-unique_guid_not_working_properly-1539224-9.patch, failed testing.

The last submitted patch, 9: feeds-unique_guid_not_working_properly-1539224-9.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 11: feeds-unique_guid_not_working_properly-1539224-11.patch, failed testing.

nicholas.alipaz’s picture

Here is a patch that adds a setting in the feeds importer processor settings page. It is a simple checkbox to toggle off the checking of the guid per feeds source.

Additionally, the title of the issue was changed to be backwards from the actual request, especially seeing as how the current title is already how feeds works and is what the request is for changing.

nicholas.alipaz’s picture

Title: Add support for unique fields to be unique per importer. » Add support for unique fields to be unique site wide
Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.71 KB
nicholas.alipaz’s picture

Anyone whom can test this would be great.

g089h515r806’s picture

#35 looks nice.

CLEE25’s picture

Patch in #35 did now work as expected. However, if I moved the condition (id) within the if/then -- then it did work as expected, and I could update nodes that were populated by a different importer.

    if ($this->config['guid_unique_per_feed']) {
      $query
	  ->condition('feed_nid', $source->feed_nid)
	  //moved this line from bottom
	  ->condition('id', $source->id);
    }

    $query
      ->condition('entity_type', $this->entityType());
      //line was here
nicholas.alipaz’s picture

CLEE25, I don't see how what you did would be needed. Can you confirm what you mean by "did no[t] work as expected?"

CLEE25’s picture

Hi Nick,

Sure.

I have two feed importers that update the same content.

1)Info
2)Photos

Both are using a global GUID. I had already imported 2300 nodes with the 1) info feed.

I applied the patch. Cleared my cache. Went to the second feed (2)Photos) and unchecked the feeds per source box.

Then I ran the import, and it created NEW nodes (it didn't update the current ones).

So I deleted those.

Then I made the code change above (using the patch in #11 as a reference) , saved. Cleared cache.

Then I ran the import, and it updated the nodes as expected.

Maybe the fact that I already imported content before the patch has something to do with it?

Let me know what other info I can provide. And thanks -- since I did manage to update nodes with a second importer thanks to you!

nicholas.alipaz’s picture

I do believe that the issue was that you had already imported nodes yes. In my testing I am able to determine that there is a little confusion on the user side when swapping back and forth between the feeds per source setting.

If you switch back and forth for that setting you are essentially removing/adding the feed ID from/to the unique id. This means that if you have nodes that are already imported and they have the feed id on the guid as per default in feeds and you disable the feeds per source setting then the items will be imported a second time sans-feed id.

I am not sure what the best solution for bypassing this edge-case would be. You really need to delete all nodes per feeds source and re-import on each to get what is correct. However, I do understand how that might not be a possibility in many cases.

The only other solutions I can think of are as follows:

  1. Rewrite the entries in the db by removing the feed id from them on submit. This seems intensive seeming how there could be multiple feeds per feed importer and they could have tons of imported nodes each.
  2. Disable the form element if there are any imported items in the db for this particular feed imported.
  3. Leave it alone and ignore the edge-case.

Let me know opinions on how you guys would expect it to work. Also, whether I was clear in my explanation or not.

CLEE25’s picture

I would probably suggest #2. Ideally, perhaps with an explanation to the user why it is disabled.

Something like,

Since you have already imported nodes using this feeds import, it must remain unique per feed source. If you need to change this, please delete your content using the import page ->delete tab, and then update this setting by returning to this page.

The reason I suggest this, is that most user are going to come looking for a solution AFTER they have used their first import, and trying to update with a second import. Thinking that the GUID is global, and not unique to each feed.

This makes it crystal clear what needs to happen to make it work.

As always, thank you for your time on this!

JonMcL’s picture

I'm revisiting this issue as I'm updating some modules. I originally wrote the simple #9. However, #34 is a much better solution and is working well for me.

As for the solutions to the edge case proposed in #41, my preference is for solution 3 .. but I'm rough like that :)

I think applying patch in #34 to the project would enhance the adaptability of Feed substantially.

..jon

nicholas.alipaz’s picture

Yeah, I am not convinced #2 should be the solution, as it seems that it would be limiting to many people. I would rather #3 or #1. #3 I feel should provide some very good explanation under the checkbox and #1 would likely be more code than I would personally like to write for something so rarely encountered.

I think maybe the maintainers of feeds should chime in at this point 'cause I would rather not write the code for #1 if it won't get used and a description could better be done with their assistance.

dobe’s picture

Ok I played with this a little. #38 Is correct.

With the verbiage: GUID is unique per Feed source
What that says to me is, if this box is check then the GUID is Unique only at that feed level. So you have to have the the condition of the $query->condition('id', $source->id); within that if statement. $query->condition('id', $source->id); (which is stating that your getting the entity_id that has a source id of a specific feed.

If you keep $query->condition('id', $source->id); out of the if statement. No matter what you do to the query, the condition is that the source id needs to remain the same. Which is not what the verbiage of the checkbox is saying. So if unchecked you don't want that condition in play. Moving the condition as #38 suggest is absolutely correct.

The patch I have attached also includes the changes needing made to FeedsEntityProcessor.inc

-Jesse

hairqles’s picture

The patch works great! Thank you!

I only had one thing to mention. I applied the patch on site where we are using the Features module. After I applied the patch and configured the desired Feed item, disabled the GUID is unique per Feed source checkbox, the settings were only applied after I updated the feature, that stores the Feed item configuration. I'm not sure that this issue happened because of Feeds module or the patch.

moelius’s picture

not working on last dev(

adrien.felipe’s picture

I've been using the patch from #45 for a while now, I have a dozen of feeds importers all updating same contents (nodes, commerce_products and taxonomy terms) and it works like a charm.

I believe I had to tweak it a little to make it work with the feeds dev version I had downloaded at the time (7.x-2.0-alpha8+56-dev) but almost nothing.

" GUID is unique per Feed source "
It would be great to see this feature released in feeds for good, and I'll be glad to help doing so.

nicholas.alipaz’s picture

Status: Needs review » Reviewed & tested by the community

based on two comments above and my own testing marking as RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 45: feeds_guid_nonunique_per_feed-1539224-45.patch, failed testing.

dobe’s picture

Assigned: Unassigned » dobe

I will update this patch soon as I get home.

MegaChriz’s picture

Issue tags: +Needs tests

It would also be great to have an automated test for this feature.

dobe’s picture

Status: Needs work » Needs review
FileSize
2.47 KB

Attached is the updated patch for current dev. For the tests would, would you like me to write a complete new test to check this setting? Or do you want me to add to the current feeds_processor_*.test's?

Status: Needs review » Needs work

The last submitted patch, 54: feeds_guid_nonunique_per_feed-1539224-54.patch, failed testing.

dobe’s picture

Woops Guess I should have ran the tests myself first ;o

Ill get this worked out.

-Jesse

MegaChriz’s picture

@dobe
I think the test class "FeedsMapperUniqueTestCase" in tests/feeds_mapper_unique.test would be a good location to store the test in. Do create a new test method in there, for example: testGUIDGlobalUnique().

Guess I should have ran the tests myself first

It's perfectly fine if tests fail sometimes in issues, that is what the testbot is for, after all! In my case, tests seem to be ran much slower locally than on drupal.org. And while the testbot is doing his job I can work on other things. I (usually) only run tests locally when I need to debug something (and I will only execute the tests that are relevant for the bug) or when developing a test.

dobe’s picture

I messed up the query. Didn't realize that you can't use the variable when you split the Query up. You have to set it to a different variable. Hey, learn something every day. This one passed tests on my local so we will see. I will figure out the testing side of it after I get lunch.

dobe’s picture

dobe’s picture

This patch includes a test. Comments appreciated. Trying to get better at test writing.

MegaChriz’s picture

Status: Needs review » Needs work
FileSize
8.07 KB
4.82 KB

Patch is looking good. I have few things to note about the patch and a few concerns about this feature in general.

First, some notes about the patch.

Patch

The patch looks good code-wise. I found three small things:

  1. +++ b/plugins/FeedsProcessor.inc
    @@ -861,14 +870,18 @@ abstract class FeedsProcessor extends FeedsPlugin {
    +        $select = db_select('feeds_item')
    

    It is more common to call the variable that is about a select query $query instead of $select.

  2. +++ b/tests/feeds_mapper_unique.test
    @@ -74,4 +74,91 @@ class FeedsMapperUniqueTestCase extends FeedsMapperTestCase {
    +    $typename = $this->createContentType(array(), array('alpha' => 'text'));
    

    Creating the "alpha" field is unnecessary here, because this field is not used in this test.

  3. The test could use some more code comments. It took me a while to figure out why the test is doing what it does.

I have fixed the notes above in the attached patch.

Concerns about side effects of this feature

Deleting items created by importer 1 through importer 2

When reading through the test, I got a little confused when I got to the "deleting" part of the test. It's great that you added that piece, by the way! I just wonder how Feeds should react on deleting items that were originally created by an other importer. Right now, when items are imported by importer 1 and then updated by importer 2, importer 2 will own the items that it had updated, but the rest of the items (that were not changed) are still owned by importer 1, even when these items appeared in the feed imported with importer 2. From an user perspective, shouldn't these items still belong to importer 1? I'm not sure, though.

Updating items using different content types

Some confusion can arise when using two importers to update nodes, but each with a different content type (bundle). When importer 1 is set to create content of type "type1" and importer 2 of "type2", then when importer 2 updates nodes created by importer 1, these will remain be of "type1". I don't think importer 2 should try to change the node type, but importer 2 could have configured targets that nodes of type "type1" does not have. This could possibly lead to errors. This concern was made before in #22. Maybe we should judge this as a misconfiguration and will it be enough to document this behaviour.

A tip on writing tests

I'm not sure if you intended the actual behaviour about deleting items or if you wrote the test to match this. Anyway, when writing tests, it's best to think about what is the desired behaviour instead of following the actual behaviour. Sometimes I write my tests first just to get a feeling of how a particular feature should work. Or I (partly) write my feature first and then before testing it manually, I write the test (and eventually adjust the implementation of the feature during test writing).

Setting to "Needs work" because of the noted concerns. In case the actual behaviour is found right, this behaviour should be documented.

dobe’s picture

Thanks for the feedback @MegaChriz!

As for notes #1. Look at line 295ish of the same file. This is why I chose this.

Many of the things brought up are definitely valid. But when you get down to it this feature is desired because a user knows he or she wants to update the same items with different importers. That's why the default remains. Documentation would be a great way to handle most of these concerns. There are additions that could be made to make sure that when feeds "updates items it would check to make sure that the $source->id is the same" This could alleviate the delete problem.

I am unsure the content type issue is really a concern. I don't believe feeds will allow you to update the content type. So the errors that would arise would come from importers with fields that don't exist in the already created item.

lmeurs’s picture

In reaction to Concerns about side effects of this feature > Deleting items created by importer 1 through importer 2, this is already happening when not using a GUID:

  • When using a custom unique mapping target as provided by the commit from #661606-201: Support unique targets in mappers (tested on a standalone import form) or
  • Non-node entities like Commerce Products which use SKU's as unique identifier.

When two importers update the same entity, the Feeds item's owner changes to the last importer (the value of the Feeds item's id database column becomes the name of the importer).

Another side effect is that hashes get updated depending on the data the last importer received. When the importers are being executed one after another on each update cycle, hash checks will always return FALSE, even when imported data has not changed since the last import cycle.

A possible solution would be if one Feeds item could be created per importer and per entity as #2306305: Multiple feeds importer on the same entity should not override each other’s hashes suggests. This way deletion and updating might not be a problem since the Feeds item's owner would never change and the hash is importer depending.

MegaChriz’s picture

Fair enough. Then we should only document this behaviour or wait until #2306305: Multiple feeds importer on the same entity should not override each other’s hashes lands.

Documentation could be something like this:

When updating the same content with multiple importers, deleting items through the importer will only delete the items that were last updated by that importer.
For example: if importer 1 imports item A, B and C and importer 2 updates item B, deleting items through importer 1 deletes items A and C and deleting items through importer 2 will only delete item B.
This behaviour will change when https://www.drupal.org/node/2306305 is fixed.

Suggestions for a better description are welcome.

The documentation could be added to the site builder's guide to Feeds. Perhaps there should also be a note on this on the importer's "Delete items" page (import/[feeds importer]/delete-items)? I'm not sure about that yet.

adrien.felipe’s picture

Patch from #61 work great for me.
Thanks for the work.

Regarding the concerns, I have the same scenarios that lmeurs describes.
Also, the description MegaChriz suggests sounds clear enough to me.

brandonc503’s picture

FileSize
245.78 KB

*deleted as i cant remove account

brandonc503’s picture

*deleted as i cant remove account

adrien.felipe’s picture

Does anyone know why we can't have this non invasive - but crucial - feature to be implemented?

I've been using it for a while and never had any issue with the #61 patch and 7.x-2.0-alpha8+84-dev

eelkeblok’s picture

The patch doesn't apply on the latest version of the module. This seems to be exclusively due to the move of FeedsEntityProcessor to a separate module. Attached is a rerolled patch. I suppose a sister issue needs to be created on the new module. I'll do so and point it to here.

eelkeblok’s picture

Status: Needs work » Needs review

The last submitted patch, 61: feeds-guid-nonunique-per-feed-1539224-61.patch, failed testing.

eelkeblok’s picture

@wayaslij The issue was not yet RTBC. It was at "Needs work", I'm not sure why exactly, possibly because of the mentioned documentation changes. I've now set it to "Needs review", because there is a new patch. When several people note the patch works for them and indicate it is finished, in their opinion, someone (anyone, really) may set it to RTBC. If it reaches that state, it is most likely to be adopted in the development branch and eventually make its way into an actual release.

adrien.felipe’s picture

@eelkeblok thanks for the new patch! (and the feedback :)
I have tested #69 patch with the node processor and the commerce product processor and it works fine. Tested against 4678 products.

Maybe someone else can test it too so we go on RTBC.

dobe’s picture

There are still many issues with this patch that was brought up by MegaChriz. These issues will need to be resolved before this ever gets committed.

eelkeblok’s picture

OK, I thought the only remaining concern was documentation. Maybe someone could update the issue summary to identify remaining issues.

Omar Alahmed’s picture

I think there should be a condition that checks for the bundle type that is generated from the feeds processing. for example I have tow content types have the same GUID, therefore, if we want to make the unique fields to be unique site wide, we should check for the bundle type to make sure that this unique field is for the processed bundle.

mikran’s picture

Status: Needs review » Needs work
+++ b/plugins/FeedsProcessor.inc
@@ -947,14 +956,23 @@ abstract class FeedsProcessor extends FeedsPlugin {
+        else {
+          $query
+            ->condition('n.type', $this->config['bundle'])
+            ->join('node', 'n', 'n.nid = feeds_item.entity_id');
+        }

This breaks all non-node importers

Omar Alahmed’s picture

@mikran you're right, I added now switch case to check the importer entity type:

+        else {
+          switch ($this->entityType()) {
+            case 'node':
+              $query
+                ->condition('n.type', $this->config['bundle'])
+                ->join('node', 'n', 'feeds_item.entity_id = n.nid');
+              break;
+            case 'taxonomy_term':
+              $query
+                ->condition('tv.machine_name', $this->config['bundle'])
+                ->join('taxonomy_term_data', 'td', 'feeds_item.entity_id = td.tid');
+              $query
+                ->join('taxonomy_vocabulary', 'tv', 'td.vid = tv.vid');
+              break;
+          }
+        }
adrien.felipe’s picture

What about any other entities, like Commerce product, or Relation?
Can't this be more global to work with any entity?

Omar Alahmed’s picture

@wayaslij this switch case to filter out the entity bundle in case of the entity type is node or taxonomy, so in case of the other entity types, the query will stay as is (without adding bundle condition to it).

adrien.felipe’s picture

I ran into the case @MegaChriz describes in #64.
Entities should be deleted by their importer, but would not, as randomly belonging to multiple importers (last from where they were updated).

Patch #69 with patch #9 from #2306305-9: Multiple feeds importer on the same entity should not override each other’s hashes solve this problem, and might even solve the main reason this thread was not yet committed.

Sorry @Omar, I haven't had time to try your patch.

joseAyudarte91’s picture

Hello!

Has anyone tried the latest patch (#78) with the 7.x-2.0-beta2 release? I suppose the patches are usually for the dev version but I think @Omar has indicated that it has been tested with 7.x-2.x branch.

I have tried it in my local environmment, but the GUIDs are not unique across different importers for the same bundle.

Many thanks and best regards,

Jose Ayudarte

AgentJay’s picture

I just applied the applied the patch in #78 to the 7.x-2.0-beta2 release and it works great for me.

Infact I was using this fix for a while and thought it had been committed in 7.x-2.0-beta2. I forgot to check and my feeds stopped working correctly.

I'm in favor of committing it ASAP.

Cheers.

alex.cunha’s picture

Applied the patch #78 to the 7.x-2.0-beta3 and it works for me using 4 different importers (each importer is a CSV with sku field and another different field) for the same content type. Nodes with product reference field (sku, commerce).
Thanks

dflitner’s picture

Applied patch in #78 to 7.x-2.0-beta3 and it's working for me. I have two separate feeds with some duplicate content and the duplicates are now getting skipped.

Alex72RM’s picture

Hi all,
because it seems #78 works, is it possible to commit it to dev?

jimmynash’s picture

Applied #78 to 7.x-2.0-beta3 and it seems to work with a separate feed to update nodes imported from another feed.
Just what I was looking for. Thanks!

darksnow’s picture

Patch in #78 works for me. I was able to create a GUID from two fields using Tamper to rewrite the GUID field.

A second feed was then used to update the existing node imported in the first using the same tamper so the GUID lined up.

Exactly what I was looking for, thanks for the hard work on this. I'm not entirely happy using patched code in production though, so any idea when this will be committed?

MegaChriz’s picture

@darksnow
I would like to see the issue noted in #64 to be resolved first.

rkelbel48’s picture

I was able to apply Patch #78 to 7.x-2.0-beta4 and the feeds worked as expected, I can now have several feeds utilizing the same GUID, Tamper also continued to work without issue.

I have a single feed uploading all the products, specs and data. Now I'm able to run another feed that has only 3 columns of data including the GUID, price, and qty to keep the product list updated regularly without having to pull and update all of the other data regularly. Huge time saver for both processing the data down and up.

This was exactly what I was looking for, I'm worried about having a patch in production, especially if there are security updates on an upcoming version that need to be updated. Is this expected to be committed to a branch going forward?

Thank you for the hard work on this patch, it is a life saver.

trentl’s picture

I will add that #78 worked for me (preliminary testing looks positive). I was in need of the ability to use the GUID to update previous feeds attached to a node using a user-defined id as the GUID. Example:

If User imports records with id numbers defined as 1 - 10 and then identifies a portion of the data that requires updates. I want the user to have the ability to update those records as well as add new records at the same time. So if they return and then import data with id numbers of 6 - 15, items with the previous id numbers 6 - 10 will be updated and 11 - 15 will be added. Having the feed_nid as a defined parameter of GUID prevented this action and would just add new nodes with the same user-defined id numbers instead of updating the previous nodes.

Using hook_feeds_after_parse() I add additional data to the GUID which then makes the update specific to that user's feeds and limits its global implications (e.g. overwriting another users id number set unknowingly). Given that my data works with a end-user based role, this patch perfectly fit the issue I was having with new nodes being added with each feed regardless of a defined GUID.

FYI - I have applied this patch to the latest dev version, as I also had another issue that required the latest code given field validation was failing for some of my feed imports when selecting the title field as unique. So far results have been positive. As kibble48 reported, I also have not had any issues with it interfering with Tamper.

Thanks to everyone that contributed. Serous life-saver as well.

stred’s picture

confirmed that #78 is working, the use case of having several importers updating same entities is quite common so this improvement should be part of the module.

cmarcera’s picture

I have been fighting with an importer that has multiple XML files and can contain a GUID multiple times within those files:

Issue details here: https://www.drupal.org/project/feed_import/issues/3008472

Patch #78 did not work for me against beta4. Within the first thousand items, I started getting multiple items with GUIDs that should be unique.

Running out of ideas...

jandewit6’s picture

Was working for me against 7.x-2.0-beta2. After updating to beta4 no longer working. Tried switching back to beta2, beta3 to no avail.

Anyone having a hint on how to get feeds working again with feeds entity reference append?

arnoldbird’s picture

What is the best workaround we can implement in a custom module? Could we use hook_node_presave to create a custom global_guid_checker module? Check the GUID for uniqueness before saving, and don't save if it isn't unique across all nodes? Or all nodes of a particular bundle, if that's your use case. This would not be a particularly elegant solution as it adds a new query for every item imported, but might be OK. Related: https://drupal.stackexchange.com/a/101684/8027

arnoldbird’s picture

I wrote a Global GUID Checker sandbox module to check for global uniqueness among nodes. It could easily be modified if you need to check for uniqueness across all entities. https://www.drupal.org/sandbox/arnoldbird/3103890

In my use case the nodes are created by a feed import. The module throws an exception in hook_node_presave to end the current loop and prevent saving of non-unique nodes, while allowing the batch to continue. This creates lots of unnecessary log messages if you are importing a lot of nodes, so the module also deletes relevant messages from the watchdog log after each feed import.

This solution is a bit of a hack but I can't think of a better way. Please post something to the module's issue queue if you have a better workaround.

michel.settembrino’s picture

Status: Needs review » Reviewed & tested by the community