Move feed parser to includes/feed.inc

I've done this in the feedparser module (currently work in progress in CVS).

http://cvs.drupal.org/viewcvs/drupal/contributions/modules/feedparser/

I've not added a refactored aggregator.module just yet, but it's on the list of things to do.

Eventually it would be good to move the parsing inc file into Core for use by everything. The plan is to expand the .inc file to handle ATOM feeds in addition to RSS.

I'm going to put this patch on hold for 1-2 days. I'm going to create a subdirectory for each of the modules first. Then the aggregator.module and aggregator.inc can live in the same directory.

Dries: do you think that it's a good idea to keep the aggregator.inc and aggregator.module tightly coupled? It seems that one of the key advantages of separating them is allowing other modules to completely replace aggregator.module without reimplimenting the parser. Moving this file into /includes means that it can be considered a 'given' for other modules to build on without requiring that the existing aggregation UI, etc be present.

Dries - I think feed parsing shouldn't be dependent on aggregator... general feed (RSS & Atom) parsing (as well as feed writing!) have applications in contrib outside of aggregator...

my $0.02CAD - but excited to see this moving forward.

Sure, moving it to includes makes sense too. :)

I've not actually touched much/if any of the parsing code i moved out of the aggregator.module.

I got the feeling such activity was a bit too radical for Core so soon so went about putting it into a contrib module first. I'm also going to be working with Drewish on this - he has nominated himself for ATOM support upgrades to the parser.inc file.

I agree with that approach: if we split out the inc file, efforts to improve the pure parsing side of things can focus on the core include file The need to include separate parsing libraries in other projects is exactly why this patch helps.

Looks okay. I'm fine with feed.inc being put in the includes directory. But we should only include it as necessary (when feeds are parsed). And there are a couple lines comment out for posterity's sake. We don't care about posterity.

How's the work around this patch code?

Moving to cvs.

I have updated the patch for CVS HEAD.

Changes are:

- Moved drupal_retrieve_feed() into common.inc.
- Implemented tests for feed.inc.
- Change _feed_parse_feed() to feed_parse().
- Rename aggregator_parse_feed() to aggregator_save_items().

Two of the tests fail because parser can not parse "<link href="http://example.org/"/>".

There is a new aggregator module in patch review. The current one is not long for this world. Please review the other if possible. Thanks.

I am aware of the new aggregator patch. I have done my part of the review for it.

This is another approach to improving the current aggregator. If somehow new aggregator can not make it's way into the core this will be here as a resort.

Updated the patch.

Improvements over #12:
Better error handling.
Support for alternate feed parsing libraries.
Test cases merged into one.

This one was committed to CVS HEAD earlier today. Thanks all.

@Dries: This patch is not committed. The one that was committed is: #16282: OPML Import for Aggregator

Oops, this wasn't committed yet. Updated the wrong issue.

Re-roll.

Latest patch is broken. Re-rolling.

Added following functions to feed.inc

function feed_set_data($data = NULL) {}
function feed_get_data() {}
function feed_get_hash() {}
function feed_set_headers($headers = array()) {}
function feed_get_headers() {}

When trying to update a feed in aggregator after adding it:

* There is no new syndicated content from gmplanet.
* The feed from gmplanet seems to be broken, because of error " Invalid document end".

* notice: Undefined property: stdClass::$code in /s**s/development.**.ca/htdocs/d1/modules/aggregator/aggregator.module on line 503.
* notice: Undefined property: stdClass::$code in /s**s/development.**.ca/htdocs/d1/modules/aggregator/aggregator.module on line 504.

Needs work.

Robin

Updated.

@Robin Monks: Can you try this one with another feed?

Why aren't you using simpleXML?

True, since we're requiring PHP 5.2 or higher this version, SimpleXML is available for this. I agree that would be a good step, but, I don't see why it couldn't be done after this patch hits HEAD. I think adding any more complexity to this patch will make it miss D7.

I'll be able to test later this afternoon, thanks mustafau!

Robin

Aron just posted a patch that replaces the existing parser with SimpleXML: http://drupal.org/node/302245 . It also moves parsing to an include file, but does not move it to includes/ - yet.

#26: That patch is back to code needs work right now, they probably need a hand over there ;-)

Anyways:

This patch has been tested and WORKS! http://robinmonks.com/feed worked perfectly. Great work mustafau! This patch is now RTBC. I'd personally like to see this go in first, then get ported to SimpleXML, but, the powers that be will decide. This patch also survived the "aggregator" simpletest, 207 passes, 0 fails, 0 exceptions.

Robin

Also note, this is a task, not a feature request AFAIK.

With more documentation.

Sorry, just found a new bug, if I update the same feed twice in a row I get this:

* notice: Undefined property: stdClass::$items in /srv/w*a/htdocs/d1/modules/aggregator/aggregator.module on line 472.
* notice: Undefined property: stdClass::$code in /srv/w*a/htdocs/d1/modules/aggregator/aggregator.module on line 504.
* notice: Undefined property: stdClass::$code in /srv/w*a/htdocs/d1/modules/aggregator/aggregator.module on line 505.

* There is no new syndicated content from Robin Monks' Blog.
* The feed from Robin Monks' Blog seems to be broken, because of error " Invalid document end".

This is a fresh head install, no previous data to mess anything up.

Robin

#27 - Indeed it would be good to move first on this patch and then on SimpleXML - don't know how compatible the two approaches are - curious to see Aron's response here.

I unified the two patches (#30 and SimpleXML for agg). aggregator.module tests are ok for me. Only one parser feed fails (no DC ns support currently for channel elements). Please test it.

sub

I needed to kill some time. This is the result.

Very simplistic. Just move some functions from aggregator.parser.inc to include/feed.inc. Now we have a general feed parser and a way to override default parser.

HEAD is broken.

#35: feed_inc-72915-35.patch queued for re-testing.

Is this still an issue?

This is now all in classes and probably no longer relevant.