aggregator_aggregator_parse() updates the feed record when done parsing. This should be handled by the aggregator API. This patch

- moves DB update and according messages from aggregator_aggregator_parse() to aggregator_refresh()
- adds a return value to hook_aggregator_parse() that signals successful parsing
- updates API documentation accordingly

CommentFileSizeAuthor
#9 aggregator.patch826 bytesmfb
#6 402280-6-aggregator_remove_feed_update_from_parser.patch6.55 KBmustafau
aggregator_remove_feed_update_from_parser.patch6.6 KBalex_b

Comments

alex_b’s picture

alex_b commented
Assigned: alex_b » Unassigned
alex_b’s picture

alex_b commented

This patch blocks #293318: Convert Aggregator feeds into entities due to overlapping changes.

alex_b’s picture

alex_b commented

as discussed on IRC - parsers that don't supply all properties would cause aggregator to overwrite not supplied properties with empty values. [edit: this behavior is not introduced by this patch, but this patch moves around code that causes it].

alex_b’s picture

alex_b commented
Status: Needs review » Needs work

It turns out that it would be easier if #293318: Convert Aggregator feeds into entities landed before we fix this. #293318 refactors aggregator_save() so that it could elegantly address #3, too.

mustafau’s picture

mustafau commented
Priority: Normal » Critical

Subscribing.

Without this patch Aggregator is not ready for prime time.

mustafau’s picture

mustafau commented
Status: Needs work » Needs review
StatusFileSize
new 402280-6-aggregator_remove_feed_update_from_parser.patch6.55 KB

Re-rolling.

mustafau’s picture

mustafau commented
Issue tags: +API clean-up

API clean-up

dries’s picture

dries commented
Status: Needs review » Fixed

Committed to CVS HEAD. Thanks!

mfb’s picture

mfb
they or he
commented
StatusFileSize
new aggregator.patch826 bytes

Follow-up bug-fix, error when refreshing if there is no last-modified date:

PDOException: SQLSTATE[HY000]: General error: 1366 Incorrect integer value: '' for column 'modified' at row 1

Apparently there is no test case for a feed without a Last-Modified header.

mfb’s picture

mfb
they or he
commented
Status: Fixed » Needs review

Re: aggregator_test.module serving a feed without a Last-Modified header, I don't see any way to unset this header (it's one of the default headers in bootstrap.inc), although drupal_add_http_header('Last-Modified', 'blah'); does allow me to change it to a non-empty value.

alex_b’s picture

alex_b commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Quick fix
dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

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

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

Issue tags: +Quick fix, +API clean-up

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