aggregator_aggregator_fetch() updates the feed record when done fetching. This should be handled by the Aggregator API.

Without this Aggregator is not ready for prime time.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alex_b’s picture

Right. Subscribing.

mustafau’s picture

Status: Needs work » Needs review
FileSize
3.24 KB

Status: Needs review » Needs work

The last submitted patch failed testing.

mustafau’s picture

Issue tags: +API clean-up

API clean-up

alex_b’s picture

Does #2 need to be applied together with #402280-6: Parser should not update aggregator_feed record ?

mustafau’s picture

No. Tests were ok when I submitted this patch.

mustafau’s picture

mustafau’s picture

Status: Needs work » Needs review
Dries’s picture

+++ modules/aggregator/aggregator.module	14 Oct 2009 19:12:13 -0000
@@ -627,6 +632,14 @@
+      drupal_set_message(t('There is no new syndicated content from %site.', array('%site' => $feed->title)));

Small indentation issue -- two spaces too many.

alex_b’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.41 KB

Patch looks good.

I fixed the indentation issue.

RTBC

alex_b’s picture

Assigned: Unassigned » alex_b
Status: Reviewed & tested by the community » Needs work

Taking another look, I see this patch is creating a soft expectation that the fetcher uses HTTP (http_response) - we should avoid assumptions about the fetching method aggregator.module .

A simple TRUE or FALSE as return value of the fetching stage would be sufficient feedback for making aggregator invoke the parsing stage or not.

Will take a stab at this.

alex_b’s picture

Assigned: alex_b » Unassigned
Status: Needs work » Needs review
FileSize
6.15 KB

- Adds a TRUE/FALSE return value to fetching stage (consistent with parsing stage)
- Renders check for http_response obsolete, thus removes the HTTP assumption in aggregator.module that #10 would have introduced.

Further:

- Removes duplicate computation of md5 hash
- Removes some minimal white space from api.php.

Re-test of 564686-12-aggregator_remove_feed_update_from_fetcher.patch from comment #12 was requested by sun.core.

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -API clean-up

Automatically closed -- issue fixed for 2 weeks with no activity.