In D7 core has started to misuse its own API call hook_flush_caches to do lots of mainenance work. The same is starting to happening with D6 with recent patches that did go in for 6.22.
As the main conception of hook_flush_caches in contrib and core is now that "you can use this to know when caches are cleared", it is quite clear that system_cron should not call this hook.
If you still want to do something on system_cron, use hook_cron.
The idea is to cache the bins directly after clearing the cache (cache_clear_all() in includes/common.inc) in a cache-variable and have system cron use that one instead of running hook_flush_caches.
Of course if the variable is empty, it would still need to call it, but could then at least cache it.
But this would remove 95% of all unncesseary calls to hook_flush_caches.
I also propose changing the docs to reflect that it is now okay, to use hook_flush_caches to know when the cache is cleared and to officially allow this as there are so many occurences that are using this or will need to use this. This is not an API change, but a change reflecting the reality of how the API is used.
There absolutely is a need for this hook and cleaning up system.module, which is just expiring old cache items would allow doing this.
Best Wishes,
Fabian
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | interdiff.txt | 2.04 KB | David_Rothstein |
| #13 | drupal-system_cron-1191290-13.patch | 3.83 KB | David_Rothstein |
| #11 | drupal-system_cron-1191290-11.patch | 3.55 KB | klausi |
Comments
Comment #1
catchSubscribing. This is a bug as well, the code does not match the documentation at all.
Comment #2
milodescComment #3
fabianx commentedThat is actually a pretty good idea I had there how to solve this long standing problem.
I completely forgot about that though :D.
Comment #4
catchHook doesn't exist in 8.x.
Replaced by:
https://api.drupal.org/api/drupal/core!core.api.php/function/hook_cache_...
And it's not invoked on cron. We get the list of cache bins for garbage collection without any hook invocations.
Moving back to 7.x, still an issue there.
Comment #5
klausiHere we go.
Comment #6
fabianx commentedRTBC - Great patch, klausi!
Comment #7
klausierrrr, yeah - now we only need tests.
I propose that we create a system_cron_test.module that implements hook_flush_caches() and set a variable there. Then we invoke drupal_cron_run() in the test, assert that the variable was set the first time, reset it, call drupal_cron_run() a second time and assert that now the variable does not get set.
Removing backport tag, since there was never a D8 version of this change.
Comment #8
klausiAnd now with test case.
Comment #11
klausiOf course it helps to also include the test module in the patch, sorry.
Comment #12
fabianx commentedRTBC - looks great to me! Tests look good.
Comment #13
David_Rothstein commentedI tested this patch but it didn't quite work correctly because there is no code that clears the cache. So for example, after a programmatic module_enable(), the next cron run won't pick up that module's cache tables. (It's only an issue for programmatic module enables because other methods for turning on modules wind up calling drupal_flush_all_caches() anyway.)
It actually looks like the tests had a workaround for that, so here's a new patch with a cache clear added to the codebase and removed from the tests.
I also changed the code comment a bit to be more general and not to refer to specific contrib modules, since those could change at some point.
It looks good to me otherwise though.
Comment #14
David_Rothstein commentedAlso, do we want to do this (from the issue summary) here?
Comment #15
joelpittetAwesome thanks for testing the programmatic
module_enable()Re #14 I'll let @Fabianx respond to that bit as he proposed it. I'll ping him.
Comment #16
David_Rothstein commentedCommitted to 7.x - thanks!
I think we can leave changing the docs to a possible followup, if desired. For now I'm OK with the scenario where we continue not to recommend running cache clearing operations in that hook... but secretly support it anyway :)
Comment #18
fabianx commented#16: I do agree, that we can keep it inofficial.
I am glad the idea has made it in :).