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.

CommentFileSizeAuthor
#12 564686-12-aggregator_remove_feed_update_from_fetcher.patch6.15 KBalex_b
#10 564686-10-aggregator_remove_feed_update_from_fetcher.patch3.41 KBalex_b
#7 564686-7-aggregator_remove_feed_update_from_fetcher.patch3.26 KBmustafau
#2 564686-aggregator_remove_feed_update_from_fetcher.patch3.24 KBmustafau

Comments

alex_b’s picture

alex_b commented

Right. Subscribing.

mustafau’s picture

mustafau commented
Status: Needs work » Needs review
StatusFileSize
new 564686-aggregator_remove_feed_update_from_fetcher.patch3.24 KB

Status: Needs review » Needs work

The last submitted patch failed testing.

mustafau’s picture

mustafau commented
Issue tags: +API clean-up

API clean-up

alex_b’s picture

alex_b commented

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

mustafau’s picture

mustafau commented

No. Tests were ok when I submitted this patch.

mustafau’s picture

mustafau commented
StatusFileSize
new 564686-7-aggregator_remove_feed_update_from_fetcher.patch3.26 KB
mustafau’s picture

mustafau commented
Status: Needs work » Needs review
dries’s picture

dries commented 
+++ 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

alex_b commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 564686-10-aggregator_remove_feed_update_from_fetcher.patch3.41 KB

Patch looks good.

I fixed the indentation issue.

RTBC

alex_b’s picture

alex_b commented
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

alex_b commented
Assigned: alex_b » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 564686-12-aggregator_remove_feed_update_from_fetcher.patch6.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

dries commented
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.

Issue tags: +API clean-up

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.