Sandbox page: http://drupal.org/sandbox/nkschaefer/1216400

The Advanced Aggregator module is designed to make Drupal's core Aggregator module (in version
7 and beyond) more useful, flexible, and scalable. In short, it makes Aggregator feeds "fieldable," adds additional Views integration for Aggregator feeds and items, checks and logs errors (bad HTTP status codes and parse errors) encountered when importing, adds helpful UI components to promote scalability, and provides an "upgrade path" for users who formerly depended on the Feeds module.

History:

In Drupal 6.x, the core Aggregator module was not very flexible. Many users turned to the Feeds module, which was able to treat both RSS feeds and their imported items as nodes, for flexibility. The Feeds module introduced the concept of pluggable fetchers, parsers, and processors, introduced flexibility, and allowed CCK fields to be attached to Feed nodes and imported items. The many options provided flexibility, but created confusion for some users.

Changes made to the Aggregator module and Drupal core in version 7.x have made some Feeds module features unnecessary for many users. The concept of "fieldable entities" allows Aggregator feeds themselves to have attached fields, and keeping imported feed items out of the node table will lead to a performance gain for users with many feeds. Additionally, the simpler UI and smaller set of options makes Aggregator easier to configure for many users.

Aggregator lacks flexibility and scalability in a few key areas, though, so this project seeks to add on to the Aggregator module to make it more suitable for a wider variety of users.

The Advanced Aggregator module does the following:

  1. Exposes core Aggregator feeds as fieldable entities. Users can now add fields to Aggregator
    feeds via a tab at the admin/config/services/aggregator page.
  2. Adds a Views relationship that allows users to access properties of parent Aggregator feeds
    for views built on child Aggregator items. This means that each imported feed item can now
    access fields added to its parent feed.
  3. For users with lots of feeds, makes the Aggregator feed administration page (admin/config/services/
    aggregator) scalable by adding pagination and only showing 50 feeds at a time. Additionally,
    checkboxes are provided that will allow users to delete multiple feeds at a time, and a search
    box is provided to search through feeds by title.
  4. A table is provided that will store broken and/or redirected Feed URLs. Users can choose to
    run a mass check of feed URLs on the site, using Batch API, which will ping the URL of each
    stored feed and store any problems in the log table. Users can view a report based on this
    table via a view provided by this module, and a form is also provided through which users can
    bulk-update redirected URLs.
  5. A utility is provided for users who (like myself) had formerly relied on the Feeds module and
    had lots of Feed nodes. The utility converts the feed nodes to core Aggregator feeds and attempts
    to preserve any Field API data associated (if you're upgrading from Drupal 6, be sure to
    upgrade all CCK data to Field API first). Once you've done this, you can safely uninstall
    Feeds and drop its tables.
  6. An alternative fetcher and parser are provided via the Aggregator module's hooks. The parser
    is designed to avoid a problem that can arise with importing feeds: title and author fields
    can contain too much data and result in database errors when insertion is attempted. The parser
    provided by this module (Advanced parser) lets the default parser act, then truncates the title
    and author fields, on word boundaries, to the maximum allowable size in the database.

    The fetcher is designed to log errors to the provided error log. The default fetcher simply
    calls drupal_set_message(), which is not useful for sites with lots of feeds importing on cron
    runs. This fetcher (Advanced fetcher) logs all bad (non-200-level) HTTP status codes to the error
    log, along with redirected URLs (if a 300-level status code is encountered).

    Additionally, the parser sometimes encounters errors when trying to parse a document as RSS XML.
    If the parser encounters an error, it also stores the error in the log, where administrators
    can view it later.

CommentFileSizeAuthor
#8 coder-result.txt13.66 KBklausi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ccardea’s picture

I just took a quick look at your project.

Licensing: No licensing problems.
Module duplication: The author has adequately addressed any concerns about duplication with the Feeds module.

The waiting time for a full review can be 4 -6 weeks. You can help reduce the waiting time by contributing to the code review process. Please visit http://groups.drupal.org/code-review for details on how to participate.

nkschaefer’s picture

Priority: Normal » Major

Elevating status due to project application workflow guidelines at http://drupal.org/node/539608.

nkschaefer’s picture

Priority: Major » Critical

Oops, I guess that page says "critical" for 4+ weeks; it's been 7.

klausi’s picture

Priority: Critical » Normal
Status: Needs review » Needs work

* Git release branch missing, see http://drupal.org/node/1015226
* README.txt is missing
* Remove all old CVS $id tags, not needed anymore.
* "Implementation of hook_uninstall()." this is now called "Implements hook_uninstall()". See http://drupal.org/node/1354#hookimpl
* Remove aggregator_advanced_install(), the schema installation is done automatically in Drupal 7.
* Remove empty function aggregator_advanced_label_callback().
* "case 307:" indentation in this case statement is broken

nkschaefer’s picture

Status: Needs work » Needs review

Okay - thanks for reviewing this.

I fixed what you pointed out:

  • Renamed README to README.txt
  • Removed all old $Id tags
  • Removed hook_install() and hook_uninstall() (didn't realize these were unnecessary in Drupal 7)
  • Changed all instances of "Implementation of..." in comments to "Implements..."
  • Removed the unused/empty function aggregator_advanced_label_callback()
  • Fixed indentation/removed use of tab characters in the switch statement mentioned
  • Created a 7.x (dev) branch in git
klausi’s picture

Status: Needs review » Needs work
  • wrong git branch name: "7.x" should be "7.x-1.x"
  • lines in README.txt should not exceed 80 characters
  • "// Add our own validation and submit handlers. These will call the necessary functions to allow" Comments should not exceed 80 characters
  • "@param $fid the feed's ID" the parameter description should be on a new line, see http://drupal.org/node/1354#functions
  • aggregator_advanced_views_default_views(): Move this hook to a file called MODULENAME.views_default.inc
  • aggregator_advanced_check_all_feeds_complete(): if you document parameters then also add a description, otherwise this is useless.
  • aggregator_advanced_handler_status_code::render(): the "break;" statement for each case should be indented.
  • "return t($text)": not good. You should never use t() to translate variables. Add a t() call to the $text assignments in the first place.
nkschaefer’s picture

Status: Needs work » Needs review

Made all the changes you suggested:

Reformatted comments, shortened all lines to 80 characters or less, made sure t() isn't called on variables, fixed indentation in case statements, moved default views hook into its own file, and created a 7.x-1.x branch.

--Nathan

klausi’s picture

Status: Needs review » Needs work
FileSize
13.66 KB

Review of the 7.x-1.x branch:

  • Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
  • Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
  • ./aggregator_advanced.module: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
      // Add our own validation and submit handlers. These will call the necessary functions to allow
      // Also need to take over the delete function, so we can get the Field Attach API to delete
      // We can't let the Aggregator module handle deleting feeds on its own, because we need to take
      ... many more in many files ...
    
  • ./aggregator_advanced.module: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
    
    function aggregator_advanced_uri_callback($entity) {
    
  • ./includes/aggregator_advanced.bulk.inc: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
    
    function aggregator_advanced_convert_feed_nodes_aux($num_to_check, &$context) {
    
  • ./includes/aggregator_advanced.admin.inc: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
    
    function aggregator_advanced_redirected_url_form_submit(&$form, &$form_state) {
    --
    
    function aggregator_advanced_redirected_url_confirm_form(&$form, &$form_state, $op, $fids) {
    
  • ./aggregator_advanced.module: The description for the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
    402- *  the feed's ID
    404- *  the feed's URL
    406- *  an HTTP status code returned by drupal_http_request (integer)
    408- *  (optional) the URL returned as the redirected address, if a 300-level
    
  • ./includes/aggregator_advanced.bulk.inc: The description for the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
    33- *  the number of RSS feed URLs to ping on each function call
    35- *  an array of information maintained by Drupal across multiple function calls
    --
    90- *  the Aggregator feed ID
    92- *  the URL of the aggregator feed
    --
    95- *  an integer specifying, roughly, what the outcome of the URL check was.
    

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

MiSc’s picture

@nkschaefer has been contacted to ask if the application is abandoned.

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

http://drupal.org/node/894256

MiSc’s picture

Status: Needs work » Closed (won't fix)

The application has been closed. If you would like to reopen it, you are free to do so.
See http://drupal.org/node/894256#abandonedtwoweekscontact