Useless implementation of hook_flush_caches()

This is not only useless: this is an api breakage. Hook_flush_caches() is an informational hook to tell Drupal names of cache tables (bins) which needs to be flushed (to invalidate & expire old items). Any module could run this hook, killing variable cache in the process. This can put great stress on the system.

@pounard I don't understand why you are arguing with me. I'm against the approach.

A lot of modules does, they are poorly code, you shouldn't be dependent of bad/tired/dump developers.

Please, read system_cron() and don't spread this false information. Yes you may not like the API, but its there: hook_flush_caches() implementations should return cache table names. You are not supposed to make any assumtptions about it. Any module can run this hook just to get names. This doesn't mean the module is going to actually flush the cache.

drupal_flush_all_caches() is not the reason of the problem. Yes, hook_flush_caches() is useless together with drupal_flush_all_caches(). It is because of the system_cron() this stuff is really evil. System module expires old cache entries on cron and uses hook_flush_caches() to get target cache table names. So depending on cron frequency variable and strongarm cache will be emptied, leading to higher load when rebuilding the stuff.

On a second thought, this is not critical, but still is pretty important.

function system_cron() {
   /* ... */
  $core = array('cache', 'cache_block', 'cache_filter', 'cache_page', 'cache_form', 'cache_menu');
  $cache_tables = array_merge(module_invoke_all('flush_caches'), $core);
  foreach ($cache_tables as $table) {
    cache_clear_all(NULL, $table);
  }
}

EDIT: This of course (as pointed out by crea below) only clears expired items and not the whole cache.

cache_clear_all with empty 'cid' will only clear expired items.

Back to topic:

Anyone up to make a patch for review? Then we can persuade the maintainer to commit this :-).

Best Wishes,

Fabian

@Fabianx System_cron() expires old items, and strongarm explicitly clears variable cache. There's big difference. See cache_clear_all() function signature.

@crea You are absolutely right, the cache_clear_all function is a little confusing in its usage. I'll update my post.

Yep, clearing variables cache each time has a huge performance impact.

Best Wishes,

Fabian

PS: Added Performance tag.

I suspect motivation for implementing hook_flush_caches() was to react on global cache clear event, cause it's the only way to do that without hacking core.

So, I took a look at the "git annotate log" to find out the history of these changes.

Here is some more information on the issue.

* strongarm_flush_caches was implemented intentionally (it seems).

* strongarm_flush_caches was first called strongarm_invalidate_cache().

* This was changed in fc10987f by yhahn with comment of: #533354: Implement "weakarm"'ing of variables: Implement weakarm mode for strongarm.

Before it was only clearing the strongarm cache, now it also clears the variables cache:

Excerpt of diff:

 /**
- * Invalidates the strongarm cache.
+ * Implementation of hook_flush_caches().
  */
-function strongarm_invalidate_cache() {
+function strongarm_flush_caches() {
+  cache_clear_all('variables', 'cache');
   cache_clear_all('strongarm', 'cache');
 }

EDIT: Hm, nope, hook_flush_caches was there before, but it was just calling strongarm_invalidate_cache.

OKAY: The hook_flush_caches was already there in the initial commit of strongarm module, so we get no explanation whatsforever for this.

Here also a first attempt was made to clear the strongarm cache:

+  // Clear strongarm & variable caches on modules page.
+  if ($form_id == 'system_module') {
+    strongarm_flush_caches();
   }

Later this was just re-engineered to use the system_module_alter call we now have.

Not really much, I know, but perhaps the above linked issue explains more.

EDIT: It does not, but I think yhahn just took over the old decisions the code had made from beginning on and just extended it ...

Best Wishes,

Fabian

I suspect motivation for implementing hook_flush_caches() was to react on global cache clear event, cause it's the only way to do that without hacking core.

This could well be, but would not make any sense with just clearing "strongarm" cid. (which was in the original patch).

I do agree with pounard that this has slipped in accidentally and been dragged along from the start and is not needed, but has never been challenged before.

Best Wishes,

Fabian

Thanks for investigating this. I need to find out a bit more about why certain things were done that way, but I'll definitely keep this fix in mind.

Closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.