Stray cache_clear_all() in aggregator_aggregator_parse()

Cache call removed

+++ modules/aggregator/aggregator.module	15 Oct 2009 21:14:39 -0000
@@ -625,12 +626,21 @@
   if (function_exists('aggregator_expire')) {
     aggregator_expire($feed);
+
+    $clear_cache = true;
+  }

This doesn't look 100% correct because we don't know if data was actually expired. In case of core, it would always return TRUE for example.

Here's an alternative patch, changing aggregator_expire to return true if items were expired

@lambic, #6 doesn't remove the actual cache_clear_all() in question.

It doesn't remove it, it makes it conditional.

hrm... If you look in patches #1 and #3 you'll see that the point of the patch is to remove the cache_clear_all() from aggregator.parser.inc. And make it conditional in aggregator.module.

Tell me if I'm way off base.

Ah sorry, didn't notice there were two files being modified. New patch attached.

Looks good. Would aggregator_expire() be more useful if it returned count($iids)?

Looks good. Would aggregator_expire() be more useful if it returned count($iids)?

That seems reasonable. For example, we don't need to bother clearing the cache if 0 items were expired.

A couple of minor notes about the code comments:

+++ b/modules/aggregator/aggregator.moduleundefined
@@ -654,8 +656,13 @@ function aggregator_refresh($feed) {
+  // Clear the cache if necessary

This comment needs a period.

+++ b/modules/aggregator/aggregator.processor.incundefined
@@ -176,9 +176,13 @@ function aggregator_save_item($edit) {
+ * @return boolean
+ *   true if items were expired, else false

TRUE and FALSE should be capitalized, and there should be a period. However, if we're changing it to an integer, then we could say:

@return int
  The number of items expired from the feed.

Note also that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades).

Tagging as novice for three tasks:

1. Change the return value to 0 or the number of items expired. (Initialize it to 0 where it is currently initialized to FALSE.)
2. Add the comment changes described above.
3. Create a new patch for the new directory structure.

Thanks!

Patch updated per comments in #12

Thanks kathyh.

+++ b/core/modules/aggregator/aggregator.processor.incundefined
@@ -198,6 +202,8 @@ function aggregator_expire($feed) {
       db_delete('aggregator_item')
         ->condition('iid', $iids, 'IN')
         ->execute();
+      $ret++;

This won't actually return the number of items expired from the feed--as far as I can tell, it will only ever be incremented to 1. The executed delete query should return the number of items deleted, I think. I'd test it to make sure. (See http://drupal.org/node/310081).

Speaking of tests, maybe a test that ensures the cache is cleared if items are deleted, and ensures it is not cleared when no items are deleted. A debug message in some test module's hook_flush_caches() would probably do the trick.

Not a problem in D8 anymore