? aggregator_todo.patch Index: aggregator/aggregator.info =================================================================== RCS file: /cvs/drupal-contrib/contributions/modules/new_aggregator/aggregator/aggregator.info,v retrieving revision 1.1 diff -u -r1.1 aggregator.info --- aggregator/aggregator.info 30 Jun 2008 23:35:23 -0000 1.1 +++ aggregator/aggregator.info 2 Jul 2008 18:27:31 -0000 @@ -1,7 +1,7 @@ -; $Id: +; $Id$ name = Aggregator -description = "Aggregates syndicated content" +description = "Aggregates syndicated content." package = Core - optional version = VERSION core = 7.x Index: aggregator/aggregator.module =================================================================== RCS file: /cvs/drupal-contrib/contributions/modules/new_aggregator/aggregator/aggregator.module,v retrieving revision 1.2 diff -u -r1.2 aggregator.module --- aggregator/aggregator.module 1 Jul 2008 15:22:04 -0000 1.2 +++ aggregator/aggregator.module 2 Jul 2008 18:27:31 -0000 @@ -21,6 +21,9 @@ * Implementation of hook_menu(). */ function aggregator_menu() { + // @todo: shouldn't we better use a loader here? There could be a problem when a new content type is being + // created(?) + // @todo: let's call this not Feed but Aggregator foreach (node_get_types('types', NULL, TRUE) as $type) { $items["admin/build/node-type/$type->type/feed"] = array( 'title' => 'Feed', @@ -31,6 +34,7 @@ 'parent' => "admin/build/node-type/$type->type", ); } + // @todo: call this only Aggregator $items['admin/content/aggregator'] = array( 'title' => 'Feed aggregator', 'description' => "Configure which content your site aggregates from other sites, how often it polls them, and how they're categorized.", @@ -81,6 +85,7 @@ /** * Implementation of hook_perm(). + * @todo; let's try to drop these permissions and stick with node permissions only. */ function aggregator_perm() { return array( @@ -127,6 +132,7 @@ /** * Provides a content-type tab form. * Also collects the parsers and the processors settings. + * @todo: move this to aggregator_admin.inc to save code lines in main module. */ function aggregator_settings_form() { $type = arg(3); @@ -162,6 +168,8 @@ '#type' => 'fieldset', '#title' => $module, ); + // @todo: I think a aggregator_settings hook is not necessary. + // Add on modules could just form_alter their additional settings into this form to the same effect. if (drupal_function_exists($module . '_aggregator_settings')) { $form[$module] = array_merge_recursive($form[$module], module_invoke($module, 'aggregator_settings', $type)); } @@ -295,6 +303,7 @@ /** * Refresh the feed. * @todo: unify with aggregator_feed_refresh(). + * @todo: Should page callbacks start with _? * * @param $node * Feed node or array of feed nodes. @@ -322,6 +331,10 @@ /** * Detects available feeds in a HTML document. + * @todo: Shouldn't this and aggregator_download() live in syndication_parser? + * I know we've discussed this, but it would be great if we could have the aggregtor module + * a bit more 'feed type agnostic' - what about putting these functions into an include file + * syndication_parser.inc that holds all worker functions and can be included by other modules * * @param $site * The HTML site. Index: aggregator_light/aggregator_light.install =================================================================== RCS file: /cvs/drupal-contrib/contributions/modules/new_aggregator/aggregator_light/aggregator_light.install,v retrieving revision 1.1 diff -u -r1.1 aggregator_light.install --- aggregator_light/aggregator_light.install 30 Jun 2008 23:35:24 -0000 1.1 +++ aggregator_light/aggregator_light.install 2 Jul 2008 18:27:31 -0000 @@ -1,5 +1,5 @@ '. t('The Syndication Parser module provides a processor to the Aggregator. It produces fast, lightweight items from the feed items.'); return $output; } @@ -26,6 +27,8 @@ if (user_access('access news feeds')) { if ($op == 'list') { $block = array(); + // @todo: Let's only build one "Latest posts" block that shows the current feed's + // latest posts - the approach here doesn't scale well. $result = db_query('SELECT nid FROM {aggregator_feed} ORDER BY nid'); while ($node = db_fetch_object($result)) { $node = node_load($node->nid); @@ -46,6 +49,7 @@ $items[] = l(check_plain($item->title), $item->link); } // @todo: theming + // @todo: check plain might screw up special HTML characters such as & $block['subject'] = check_plain($node->title); $block['content'] = theme('item_list', $items); return $block; @@ -84,7 +88,16 @@ * Decides if the item is new or not. * The decision is based on the original URL of the article. * This is the only simple and somewhat reliable way to do. + * @todo: I'm not sure if I agree with that :) it can actually get more complex + * and depending on what you're trying to aggregate you can use many things in your feed item to + * determine duplicity - hence: + * @todo: + * We might want to go back to a hook_aggregator_process operation 'unique' that + * allows any enabled processor to do additional duplicate checks. + * Another option, but less attractive, could be to make it the processor's responsibility to + * provide an API for deduping to other modules. * Lots of feed use the guid totally wrong. + * * @param $nid * Feed-node * @param $item Index: syndication_parser/syndication_parser.module =================================================================== RCS file: /cvs/drupal-contrib/contributions/modules/new_aggregator/syndication_parser/syndication_parser.module,v retrieving revision 1.3 diff -u -r1.3 syndication_parser.module --- syndication_parser/syndication_parser.module 30 Jun 2008 23:35:24 -0000 1.3 +++ syndication_parser/syndication_parser.module 2 Jul 2008 18:27:31 -0000 @@ -12,6 +12,8 @@ * Large pieces of the parsing code originally come from aggregation module (mistknight) * * @todo: add namespaces support, enclosures, exact date handling (UTC timestamp, local string + timezone) + * @todo: put private functions that are only used when hooks are called into seperate + * syndication_parser.inc file to save weight. */ /**