Separate tracking of feeds_selfnode_processor items from other feeds items

This is a tricky one. I think the current behavior is "correct" in that a the node does own itself. But, it can be a pain.

Ahh, it won't switch the items back to being owned by the correct importer, but if you run the master importer they should switch and stay that way.

I realized the correct way to fix this is to create our own tracking table, real patch coming.

I missed this issue before, but as I work through #2023735: Selfnode processing deleting feed_item db rows, leading to duplicates I'm pretty sure it's a duplicate.

The fundamental issue seems to be that Feeds and Feeds Self Node Processor are competing for the same primary key in feeds_item (entity_type+entity_id), so they'll always create two separate records. The patch in #3 doesn't address that.

@twistor, in #5 it sounds like you're suggesting a table something like feeds_self_node_processor_item . I'm happy to help with a patch, though you'd bang it out much more quickly than I :)

Here's an initial, very rough pass at a patch. It creates the new feeds_selfnode_processor_feed_item table, and uses that to track self-processing feed node items.

As well as a general review of the approach, there are 3 todo's marked in the code. Let me know your thoughts!

New patch with a better approach to the process function (my first todo from #8).

@vinmassaro: sounds good -- let's get this working! I'm anxious to deploy it, too, as the dev version fixes other bugs in beta3 and I'd hate to revert. (Secretly hoping that when we get this issue tidied up we might see a new release soon :) )

I made the issue title more general as I believe is appropriate.

This patch gets the feeds item hash from the new tracking table so that it properly only updates the item when its source has changed.

Oops, and we need to override Feeds' hash function so we're comparing apples to apples here and not updating fsnp items unnecessarily. What else am I missing?

@vinmassaro -- great, glad to hear it. This is a big patch -- I hope we get more eyes on it, especially Twistor's! :)

I'm still poking and testing, but am planning to roll this out into production w/ our site's next release.

This is looking really good.

+++ b/sites/all/modules/feeds_selfnode_processor/FeedsSelfNodeProcessor.incundefined
@@ -141,6 +149,29 @@ class FeedsSelfNodeProcessor extends FeedsNodeProcessor {
   /**
+   * Override parent::hash().
+   */
+  protected function hash($item) {
+    static $serialized_mappings;
+    if (!$serialized_mappings) {
+      $serialized_mappings = serialize($this->config['mappings']);
+    }
+    return hash('md5', serialize($item) . $serialized_mappings);

What's the difference between this and the parent hash()?

+++ b/sites/all/modules/feeds_selfnode_processor/feeds_selfnode_processor.installundefined
@@ -0,0 +1,151 @@
+
+// @todo: Need to review this db schema... Should it stay identical to
+// feeds_item?

I don't think it needs to stay the same. We can drop entity_type and entity_id.

+++ b/sites/all/modules/feeds_selfnode_processor/feeds_selfnode_processor.moduleundefined
@@ -15,3 +15,84 @@ function feeds_selfnode_processor_feeds_plugins() {
+// @todo: Instead of calling _feeds_selfnode_processor_item_info_save from our
+// process function, can we disable Feeds implementations of these hooks when
+// we're running Feeds Self Node Processor's implementations of them, so we can
+// let the hooks call _feeds_selfnode_processor_item_info_*?
+
+/**
+ * Implements hook_entity_insert().
+ */
+/*
+function feeds_selfnode_processor_entity_insert($entity, $type) {
+  list($id) = entity_extract_ids($type, $entity);
+  feeds_selfnode_processor_item_info_insert($entity, $id);

Let's just make these methods on the processor, and not attach anything to the entity. That will disable the Feeds hook.

+++ b/sites/all/modules/feeds_selfnode_processor/feeds_selfnode_processor.moduleundefined
@@ -15,3 +15,84 @@ function feeds_selfnode_processor_feeds_plugins() {
+/**
+ * Implements hook_entity_delete().
+ */
+/*
+function feeds_entity_delete($entity, $type) {
+  list($id) = entity_extract_ids($type, $entity);
+
+  // Delete any imported items produced by the source.
+  db_delete('feeds_item_selfnode_processor')
+    ->condition('entity_type', $type)
+    ->condition('entity_id', $id)
+    ->execute();

hook_entity_delete(), should change to hook_node_delete(). This is the only hook we need.

Moving the feed_item management to the processor is something I'm doing in the 3.x branch of Feeds anyway. It's silly to fire a hook when we know exactly what's happening on import anyway.

Thanks, twistor. Updated patch attached...

What's the difference between this and the parent hash()?

Oops -- nothing. :) I thought $this would refer to the FeedsSelfNodeProcessor object as opposed to the FeedsProcessor object such that $this->config['mappings'] would differ, but that's not the case, is it. Removed.

I don't think it needs to stay the same. We can drop entity_type and entity_id.

So this has also killed the table's primary key and altered most of the indexes as well. I've added a new primary key for feed_nid -- not sure if that's what we want, or if we want to tweak any other indexes.

Let's just make these methods on the processor, and not attach anything to the entity. That will disable the Feeds hook.

Done.

hook_entity_delete(), should change to hook_node_delete(). This is the only hook we need.

Implemented.

Alright, how's this sitting now? :)

Oops, didn't mean to change Component.

@vinmassaro -- Thanks for the re-roll and I'm glad it's working for you. I look forward to this getting committed (or any more feedback from twistor).

Would this patch solve the issue of Feeds Self Node Processor creating duplicate items? Please correct me if I'm on the wrong path..

@t_hall -- it might... See my comment #7 and linked issue above. If that matches your scenario, you're in the right place (and you should try out the patch in this issue!). If not, you'll probably want to create a new issue and describe your situation in detail.

There's a bug in the patch from #19.

The getHash() method returns an object, not the hash value as a string, so therefore the hash check will always fail since we are comparing a string against an object. This caused a massive headache for me because I was mapping an image field in my feed importer. A new copy of the image got added on every update until my server ran out of disk space.

Here is a re-roll of the patch where the getHash() method returns the hash from the DB as a string.

There's no reason to track the URL and GUID since we don't do unique fields.