Download & Extend
Feeds » Issues

Remove FeedsFeedNodeProcessor

Posted by pvhee on February 28, 2010 at 8:12pm
Project:Feeds
Version:6.x-1.x-dev
Component:Code
Category:feature request
Priority:normal
Assigned:Unassigned
Status:needs work

Issue Summary

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.

Login or register to post comments

Comments

#1

Posted by pvhee on February 28, 2010 at 11:05pm

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.

#2

Posted by alex_b on February 28, 2010 at 11:51pm

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?

#3

Posted by pvhee on February 28, 2010 at 11:56pm

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.

#4

Posted by pvhee on March 10, 2010 at 12:01pm

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.

AttachmentSize
728534_4_feedsfeed_0.patch 15.26 KB

#5

Posted by alex_b on March 26, 2010 at 4:45pm
Status:active» needs review

Awesome, I need to review...

#6

Posted by alex_b on April 29, 2010 at 12:04am
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.

#7

Posted by pvhee on May 1, 2010 at 9:59am
Status:needs work» needs review

No problem! I've rerolled the patch.

AttachmentSize
728534_7_feedsfeed.patch 13.84 KB

#8

Posted by alex_b on May 16, 2010 at 9:48pm
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

#9

Posted by infojunkie on July 13, 2010 at 7:17pm

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.

#10

Posted by infojunkie on July 27, 2010 at 11:48pm

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.

AttachmentSize
728534_10_feedsfeed.patch 12.38 KB

#11

Posted by alex_b on July 28, 2010 at 12:51pm

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.

#12

Posted by alex_b on July 28, 2010 at 12:52pm
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 :-)

#13

Posted by pvhee on July 28, 2010 at 12:54pm

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

#14

Posted by gcassie on September 11, 2010 at 4:57pm

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.

#15

Posted by alex_b on October 4, 2010 at 1:59am
Title:Make FeedsFeedNodeProcessor extend FeedsNodeProcessor» Remove FeedsNodeProcessor
Version:6.x-1.x-dev» 7.x-2.x-dev
Status:needs work» needs review

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.

AttachmentSize
728534-15_remove_feeds_feed_node.patch 12.23 KB

#16

Posted by alex_b on October 4, 2010 at 1:59am
Title:Remove FeedsNodeProcessor» Remove FeedsFeedNodeProcessor

Ha, sorry.

#17

Posted by alex_b on October 4, 2010 at 2:17am

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

AttachmentSize
728534-17_remove_feeds_feed_node.patch 13.31 KB

#18

Posted by alex_b on October 4, 2010 at 2:24am

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

AttachmentSize
728534-18_remove_feeds_feed_node.patch 13.34 KB

#19

Posted by alex_b on October 4, 2010 at 2:36am

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

#20

Posted by alex_b on October 4, 2010 at 8:52pm
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

#21

Posted by twistor on October 20, 2010 at 3:50am
Status:patch (to be ported)» needs review

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

AttachmentSize
728534-1-port_remove_feeds_feed_node.patch 5.95 KB

#22

Posted by Ayesh on May 22, 2011 at 9:22pm

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

#23

Posted by System Message on May 22, 2011 at 9:42pm
Status:needs review» needs work

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

#24

Posted by Ayesh on May 23, 2011 at 6:53am

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!

#25

Posted by Dave Reid on May 23, 2011 at 1:40pm

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