As we talked about in #828000: Use multiple data sources/parsers to save into the same nodes, I am writing a processor that updates nodes that were created by another feed. However, because feeds_node_item has a primary key of nid, and I want to store multiple hashes per nid, I am going to have to implement my own separate hash table.

I can think of no reason the table couldn't be keyed off of nid, id, feed_id instead (to support multiple feeds working on the same node) if the Feeds codebase was updated accordingly. Would you accept a patch making these changes?

Thanks

CommentFileSizeAuthor
#9 828000-feeds_node_item_primary-6.patch11.23 KBandrewlevine
#5 828000-feeds_node_item_primary-2.patch3.71 KBandrewlevine
#2 828000-feeds_node_item_primary.patch3.61 KBandrewlevine

Comments

alex_b’s picture

alex_b commented

I would accept a patch for that. Could you make sure to add an index for 'nid'? It won't be the sole PK anymore...

andrewlevine’s picture

andrewlevine commented
Status: Active » Needs review
StatusFileSize
new 828000-feeds_node_item_primary.patch3.61 KB

Here is the patch that does this. I wasn't able to run the simpletests because I get "Maximum function nesting level has been reached" with or without the patch.

Anyway, I manually tested and everything seems to continue working. There should be no need to add an index for nid because an index of nid, id, feed_id works just as well as nid (since it is the first column of the index).

alex_b’s picture

alex_b commented

That sucks that tests are not completing for you. I don't have any problems (PHP 5.2, Drupal 6.16). If you get a chance, would be great if you could check whether your test setup complies with with http://drupal.org/node/622700#testing

andrewlevine’s picture

andrewlevine commented

I did make sure my machine complied with that page. I am on Drupal 6.17 though (PHP 5.2). I can open a separate issue on getting the testing working if you'd like. It does seem I have a specific bug with Feeds/Simpletest

andrewlevine’s picture

andrewlevine commented
StatusFileSize
new 828000-feeds_node_item_primary-2.patch3.71 KB

Small change to the patch that makes getHash slightly more flexible (allows you to get the hash for other importers if you pass in an extra argument)

alex_b’s picture

alex_b commented
Status: Needs review » Needs work

#5 - I am fine with changing the schema of feeds_node_item, so that we have more flexibility for custom parsers extending FeedsNodeProcessor. But I'd like to avoid functionality like the one introduced to getHash() in #5 that I assume can easily live in the extending class.

In this sense, also #2 needs work.

Does that work for you?

andrewlevine’s picture

andrewlevine commented
Status: Needs work » Needs review

I'm fine with extending base functions as little as possible. However, now that we are changing the schema to key feeds_node_item off of the three columns, we need to search on those 3 columns when finding a specific row instead of on the one column. I looked through every function in Feeds that references feed_node_item and only 2 needed to be changed:

1. The one line in the load op of _feeds_nodeapi_node_processor
2. getHash()

As far as I can tell, getHash needs to be passed at least nid, feed_nid because feed_nid cannot be inferred from the class alone. If we don't do this we risk pulling the wrong hash from getHash() in the FeedsNodeProcessor.

This means we should use my first patch (the one in #2) to cause the least possible change. Does that sound OK?

andrewlevine’s picture

andrewlevine commented

Found one more bug I have to squash with this patch, but I'd like you to review my comments on #7 when you get the chance :)

andrewlevine’s picture

andrewlevine commented
StatusFileSize
new 828000-feeds_node_item_primary-6.patch11.23 KB

So the snag I ran into was that of course on nodeapi op load, we only get nid and are expected to return feeds_node_item. With multiple feed sources we need to return multiple feed_node_items.

In this latest patch I have simply taken the patch from #2 and added support for multiple feeds_node_item objects per node.

alex_b’s picture

alex_b commented
Status: Needs review » Closed (won't fix)

#9 - This change is getting more far reaching than I had expected :-/

I'd like to avoid introducing an array $node->feeds_node_item when not going the whole nine yards and adding real multiple relationships between item nodes and feed nodes, which I am very reluctant to do. Multiple parents per item node will create all kinds of difficulties for features like sync mode, designated authors etc.

We had optional multiple relationships between item nodes and feed nodes in FeedAPI and it led to a couple of very messy pieces of code.

My recommendation at this point is to implement your use case with a custom parser, maybe not even an extension of FeedsNodeProcessor but a fork.

I am sorry, I didn't see this coming. Let me know if you have further questions. Also, feel free to grab me on IRC.

(BTW, patch does not apply to latest HEAD anymore).