Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Hello,
Since you use the global 'cache' table for strongarm misc. caches, you don't need to implement the hook_flush_caches()
because Drupal will handle this table by itself.
Comment | File | Size | Author |
---|---|---|---|
#17 | strongarm-6.x-2.x-944970-3.patch | 1.11 KB | pounard |
#17 | strongarm-7.x-2.x-944970-3.patch | 1.08 KB | pounard |
#16 | strongarm-7.x-2.x-944970-1.patch | 754 bytes | pounard |
#16 | strongarm-7.x-2.x-944970-2.patch | 804 bytes | pounard |
#15 | strongarm-6.x-2.x-944970-1.patch | 770 bytes | pounard |
Comments
Comment #1
pounardIf you remove this hook implementation, you will need to fix the
function strongarm_form_system_module_alter()
function in order to manually flush the caches you need to flush there.Comment #2
pounardAnd also the
strongarm_admin_revert_submit()
function.Grep never lies.
Comment #3
pounardPlus, useless cache clear will occur at cron run time if you let this useless implementation, thus wipeout out caches that still are valid. This will cause, maybe minimal, but still useless, cache miss and rebuild occurs.
Comment #4
crea CreditAttribution: crea commentedThis 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.
Comment #5
pounardThis is an old issue, I'll look again at the code.
Comment #6
pounardYes, I know how to read api.drupal.org myself.
See this code (strongarm.module, line 119):
When cache clear all is triggered, the
hook_flush_caches()
is called, right? This code basically does an explicit delete of CIDvariables
andstrongarm
in thecache
BIN when it's called, still following me?But now, look at that (common.inc, around line 3700):
Core flushes the
cache
BIN completely. If I read correctly, your implementation is called during themodule_invoke_all()
right before. You explicitly drop two specific CID which are being wiped out right after.Your implementation is useless (in 6.x-2.0 version, at least), you finally do 2 useless SQL queries.
This makes the ...
... so wrong, because you named your own function like a hook, and basically it doubles what the core already does when it calls the hook, moreover, you call at a place where it is probably already being called at some point.
EDIT: Removed wrong wrong argument here. I totally support crea for this, and strongarm shouldn't itself!
Comment #7
crea CreditAttribution: crea commented@pounard I don't understand why you are arguing with me. I'm against the approach.
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.
Comment #8
pounard@crea Ohhh sorry I totally misread what you said (I though you were arguing my point). Being non native english speaker doesn't help... Ok, nevertheless my points remain true. I'm going to edit my initial post about that, sorry again.
Yes, but this is not false information, you are true about the hook, I acknowledged that. People that made themselves queries in that hook are severely doing it wrong! That's one of the things I'm pointing in my posts, and one other is that the module doesn't respect the hook signature.
Agree with
system_cron()
, it's bad, there is actually a lot of modules that does ugly stuff inhook_flush_caches()
but test if it's actually the cron running them before (that is a really ugly hack).Comment #9
crea CreditAttribution: crea commentedOn a second thought, this is not critical, but still is pretty important.
Comment #10
pounardYes, just a matter of having a clean code base. It's not critical at all, but it's an easy patch, and clean and simple code always roxx :)
Comment #11
Fabianx CreditAttribution: Fabianx commentedEDIT: This of course (as pointed out by crea below) only clears expired items and not the whole cache.
Back to topic:
Anyone up to make a patch for review? Then we can persuade the maintainer to commit this :-).
Best Wishes,
Fabian
Comment #12
pounardTrue! If I have enough motivation I'd do one tonight :)
Comment #13
crea CreditAttribution: crea commented@Fabianx System_cron() expires old items, and strongarm explicitly clears variable cache. There's big difference. See cache_clear_all() function signature.
Comment #14
Fabianx CreditAttribution: Fabianx commented@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.
Comment #15
pounardPatch for 6.x-2.x.
Why is it important to clear variable cache on module page? Does it fix some kind of bug? If so, take the first patch, if not, take the second.
Comment #16
pounardSame comment for D7 version.
Comment #17
pounardSorry, forgot one function call.
EDIT: These are the good one. The forgotten line only affects admin pages.
Comment #18
crea CreditAttribution: crea commentedI 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.
Comment #19
Fabianx CreditAttribution: Fabianx commentedSo, 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:
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:
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
Comment #20
pounardIf this function is really useful, then it was a better name (at least not conflicting with a hook doing the exact same thing).
This path is really a big patch, errors can be in.
This is pretty good historical explanation, but I still don't see the point of keeping that. Seems to be a design error that has been kept from the very beginning without never being questioned.
Comment #21
Fabianx CreditAttribution: Fabianx commentedThis 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
Comment #22
tirdadc CreditAttribution: tirdadc commentedThanks 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.
Comment #23
kenorb CreditAttribution: kenorb commentedClosed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.