Is there a reason nodes created by FeedsFeedNodeProcessor are different from nodes created by FeedNodeProcessor?

For example, nodes created by FeedsFeedNodeProcessor are *not* stored in the feeds_node_item table, while nodes created by FeedNodeProcessor are.

However, the only difference between the two is that the first one creates feed nodes while the latter creates simple nodes. Wouldn't it make more sense then if FeedsFeedNodeProcessor subclasses from FeedNodeProcessor (as it is a special case). This way, the FeedsFeedNodeProcessor also has the nice functionality that it is stored as an item. This in turn can be harvested by views to inspect relationships like OPML Feed -> RSS Feed -> Item. In the current implementation, I do not understand how to get from Item back to OPML Feed because of the missing parent-child relationship between OPML Feed and RSS Feed.

I already started introducing that functionality in the FeedsMultiFeedNodeProcessor (http://github.com/pvhee/feeds_multifeeds/) and could turn it into a patch if wanted.

CommentFileSizeAuthor
#21 728534-1-port_remove_feeds_feed_node.patch5.95 KBtwistor
#18 728534-18_remove_feeds_feed_node.patch13.34 KBalex_b
#17 728534-17_remove_feeds_feed_node.patch13.31 KBalex_b
#15 728534-15_remove_feeds_feed_node.patch12.23 KBalex_b
#10 728534_10_feedsfeed.patch12.38 KBinfojunkie
#7 728534_7_feedsfeed.patch13.84 KBpvhee
#4 728534_4_feedsfeed_0.patch15.26 KBpvhee
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pvhee’s picture

pvhee CreditAttribution: pvhee commented

I've coded this idea together in the feeds_multifeeds module: http://github.com/pvhee/feeds_multifeeds/

This is the situation:

  • FeedsMultiFeedNodeProcessor: subclass from FeedsNodeProcessor; exactly the same concept could be applied to FeedsFeedNodeProcessor
  • feeds nodes created this way are also stored in the feeds_node_item table (entirely done by FeedsNodeProcessor), and thus parent-child relationship is respected
  • There are now new opportunities for cleaning the feeds as you can use the relationship to find the children to delete (and they, at their turn might delete their children).
  • both FeedsNodeProcesssor::process and FeedsMultiFeedNodeProcessor::map call node_object_prepare; this is redundant and should be done after the mapping probably? Patch for FeedsNodeProcessor would then be needed
  • unit test added. From the description: 'Import OPML feed with 2 feeds, describing different types of feed nodes. Every feed will create nodes of its assigned type.'

Ideally, the FeedsMultiFeedNodeProcessor would replace the FeedsFeedNodeProcessor (of course, keeping the name FeedsFeedNodeProcessor). I could turn this into a patch for that file if you wish. I'd love to hear what you think about this.

alex_b’s picture

alex_b CreditAttribution: alex_b commented

Is there a reason nodes created by FeedsFeedNodeProcessor are different from nodes created by FeedNodeProcessor?

FeedsFeedNodeProcessor is rather old and could be probably replaced altogether by FeedsNodeProcessor.

Have you tried to patch FeedNodeProcessor with an additional mapping target for the feed URL? Wouldn't that be the most direct way of getting rid of FeedsFeedNodeProcessor altogether?

pvhee’s picture

pvhee CreditAttribution: pvhee commented

Have you tried to patch FeedNodeProcessor with an additional mapping target for the feed URL? Wouldn't that be the most direct way of getting rid of FeedsFeedNodeProcessor altogether?

I now subclassed it to not interfere too much with FeedNodeProcessor but indeed patching it would be sufficient. The extra functionality can easily be integrated there. I'll roll out a patch with some tests as soon as possible.

pvhee’s picture

pvhee CreditAttribution: pvhee commented
FileSize
728534_4_feedsfeed_0.patch15.26 KB

In attachment a patch that makes FeedsFeedNodeProcessor subclass from FeedNodeProcessor instead of FeedsProcessor.

It contains a unit test that imports an OPML listing two RSS feeds. The RSS feeds at their turn will import nodes as well.

I considered integrating this directly in FeedNodeProcessor but some settings are rather specific, e.g. the content type you import into should have an attached importer configuration, deleting the nodes might be unwanted (as you could start a cascade of deletions), etc.

The patch solves the problems outlined in #0, e.g. now nodes imported using this processor follow the same logic as the ones imported using FeedsNodeProcessor.

alex_b’s picture

alex_b CreditAttribution: alex_b commented
Status: Active » Needs review

Awesome, I need to review...

alex_b’s picture

alex_b CreditAttribution: alex_b commented
Status: Needs review » Needs work

The patch doesn't apply anymore. Sorry, this was partly my fault as I was completely backed up with other work until recently. Would appreciate a re-roll.

pvhee’s picture

pvhee CreditAttribution: pvhee commented
Status: Needs work » Needs review
FileSize
728534_7_feedsfeed.patch13.84 KB

No problem! I've rerolled the patch.

alex_b’s picture

alex_b CreditAttribution: alex_b commented
Status: Needs review » Needs work

This patch applied, I get 148 passes, 145 fails, and 0 exceptions:

http://skitch.com/alexbarth/dd3y3/test-result-feeds

infojunkie’s picture

infojunkie
Location Vancouver
CreditAttribution: infojunkie commented

I've been trying this patch and ran into the following superficial problems:

* feeds.plugins.inc: Should modify FeedsFeedNodeProcessor entry to 'parent' => 'FeedsNodeProcessor'.
* After patching, the PHP parser chokes on FeedsFeedNodeProcessor.inc because FeedsNodeProcessor class is not found (maybe due to CTools caching the plugin definitions). I just added the line require_once(drupal_get_path('module', 'feeds') . '/plugins/FeedsNodeProcessor.inc'); to FeedsFeedNodeProcessor.inc, cleared the cache, then removed the line again.

Other than that, this patch seems to work fine. I'll submit an updated patch once I try the tests.

infojunkie’s picture

infojunkie
Location Vancouver
CreditAttribution: infojunkie commented
FileSize
728534_10_feedsfeed.patch12.38 KB

Just for reference, here is the updated patch against today's dev.

If anyone has a fix for the require_once workaround above, please say so.

alex_b’s picture

alex_b CreditAttribution: alex_b commented

After patching, the PHP parser chokes on FeedsFeedNodeProcessor.inc because FeedsNodeProcessor class is not found

That is strange as this class should be found exactly because it is declared as parent in the plugin definition. Can you make sure you cleared all caches with drush cc all or equivalent? I wonder whether there was a change to CTools plugin.inc, but I doubt it, because otherwise not even "FeedsProcessor" or "FeedsPlugin" parent classes would be found.

alex_b’s picture

alex_b CreditAttribution: alex_b commented
Title: Treat feed nodes created by FeedsFeedNodeProcessor as imported feed node items » Make FeedsFeedNodeProcessor extend FeedsNodeProcessor

Renaming for making this issue easier to find - I've at times been looking for it and couldn't dig it up :-)

pvhee’s picture

pvhee CreditAttribution: pvhee commented

Indeed, I never had the problem with the patch of #7, the class was found because it is the parent.

gcassie’s picture

gcassie CreditAttribution: gcassie commented

One really great thing this patch makes possible is to take an XML file of FeedAPI information generated with Views and import it as Feeds sources, giving you another migration option instead of http://github.com/developmentseed/feedapi2feeds.

alex_b’s picture

alex_b CreditAttribution: alex_b commented
Title: Make FeedsFeedNodeProcessor extend FeedsNodeProcessor » Remove FeedsNodeProcessor
Version: 6.x-1.x-dev » 7.x-2.x-dev
Status: Needs work » Needs review
FileSize
728534-15_remove_feeds_feed_node.patch12.23 KB

After having finally time to revisit this issue, I think we should actually remove FeedsFeedNodeProcessor. This will require 25 lines of code in FeedsNodeProcessor and get rid of 226 lines of stale FeedsFeedNodeProcessor code.

See patch attached.

alex_b’s picture

alex_b CreditAttribution: alex_b commented
Title: Remove FeedsNodeProcessor » Remove FeedsFeedNodeProcessor

Ha, sorry.

alex_b’s picture

alex_b CreditAttribution: alex_b commented
FileSize
728534-17_remove_feeds_feed_node.patch13.31 KB

- Forgot to remove FeedsFeedNodeProcessor from plugin definition. Fixed.
- Fixed a small but fatal typo where a variable was not initialized.

alex_b’s picture

alex_b CreditAttribution: alex_b commented
FileSize
728534-18_remove_feeds_feed_node.patch13.34 KB

- Fixed another small typo that was causing FeedsNodeProcessor to not find the correct content type setting.

alex_b’s picture

alex_b CreditAttribution: alex_b commented

All tests passing now. I am going to commit this soon-ish.

alex_b’s picture

alex_b CreditAttribution: alex_b commented
Version: 7.x-2.x-dev » 6.x-1.x-dev
Status: Needs review » Patch (to be ported)

#18 is committed now. Needs porting to 6.x

http://drupal.org/cvs?commit=431452

twistor’s picture

twistor CreditAttribution: twistor commented
Status: Patch (to be ported) » Needs review
FileSize
728534-1-port_remove_feeds_feed_node.patch5.95 KB

Tests pass, (not counting ones that didn't beforehand).

Ayesh’s picture

Ayesh CreditAttribution: Ayesh commented

No further discussion here?
and no patch will be ported?

Status: Needs review » Needs work

The last submitted patch, 728534-1-port_remove_feeds_feed_node.patch, failed testing.

Ayesh’s picture

Ayesh CreditAttribution: Ayesh commented

All tests failed!
Actually I tried to patch http://drupal.org/node/728534#comment-3257292 manually, to 6.x.1.0 version but there is a mismatch.
Also, there is no 6.X.Dev version available and it looks D6 branch is abandoned!

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

@Ayesh: Patches should be created against the 6.x-1.x branch from a Git repository clone.

twistor’s picture

twistor CreditAttribution: twistor as a volunteer commented
Issue summary: View changes
Status: Needs work » Closed (outdated)