Posted by killes@www.drop.org on July 7, 2006 at 2:34pm
| Project: | Drupal core |
| Version: | x.y.z |
| Component: | base system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | killes@www.drop.org |
| Status: | closed (fixed) |
Issue Summary
I've long wanted to make blocks easier cachable. The attached patch outlines some ideas I have about this. The patch isn't 100% complete yet, but should work if you only use forum.module blocks not blog blocks or statistics blocks.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| block_cache.patch.txt | 5.68 KB | Ignored: Check issue status. | None | None |
Comments
#1
Dries pointed out that people might use query rewriting without doing node access stuff. He also wanted to deled the "active forums" block cache when new comments were posted.
#2
No reason to limit this to blocks...
#3
In general I +1 this patch. I need to actually test it; hopefully I'll be able to do that later this weekend.
#4
I haven't dug into the drupal caching mechanism yet, but to me this patch looks like a lot of special cases have to be put in by hand for each block. Could this go into more of an API (even if it's a little less efficient) managed by the blocks module? I.e. in hook_block you flag certain of your blocks as cachable, and either call cache_clear_all('block:forum:', TRUE), or have a block-specific wrapper for this sort of call?
#5
Doing this in the block module will be very hard. The problem that our cache system has is that it simply clears the cache based on some rather general assumptions and as a result does it too often. By doing the caching inside the functions which generate the cached content we knoe exactly what is going on and can build caches in a much better way.
Also, this patch is no longer restricted to blocks.
#6
Any new changes to this patch? I'm thinking about how to make patter use of the chaching mechanism in regards to teh forum navigation elements: http://drupal.org/node/70578
#7
Well, I guess the patch could use some testing. Here is a version for 4.7.
#8
This patch includes caching for the navigation elements as suggested.
#9
was buggy, we need a "per node" cache.
#10
Ok, I'll try it out.
One quick comment regarding the node_access() cehck. Could this cause some problems, since an access control module that is disabled may leave lots of entries in node_accss?
Also, I will make a patch ASAP for HEAD (and 4.7?) to get the forum topic navigation code out of the theme function and into either a public helper function or maybe into forum_load().
#11
if a node_access module leaves stale data in the node_access table we should get the author to clean up the mess. ;)
I am running the latest patch at drupal.org and the forum_get_navigation stuff is run much less often now, roughly only half of the time.
#12
Ok, well I think we'll have to have words with several of them then ;-)
I just tested with taxnomy access contol, and going through the deactivation procedure still leaves all the data in the node_access table. This doesn't really makes sense, since if you truncate the table and put the default row back in, when you re-activiate the module it recreates the data.
I'll file an issue vs. TAC. I think maybe OG has this issue as well.
#13
the last patch was still buggy, we need to invalidatethe cache if we get new comments.
#14
still buggy, why is $comment once an object and an array sometimes else?
#15
I'm working on pulling db query and cache code out of the theme_forum_topic_navigation function.
also, forum_admin_configure() needs to have a submit function defined that will expire the cached navigation items if the ordering is changed.
I'll take a stab at that too.
#16
Oh wait, I see now. The md5 on the $sql as part of the cache ID alleviates the need to clear the cache when the order is changed.
The attached patch (for 4.7) moves the logic for finding the prev/next forum topics out of the theme function.
Upside- this is now theme independent. A site with multiple themes and different overrides per theme for this function can now use the same cached values via the new helper function.
Downside- added calls to serialize/unserialize.
#17
I think maybe a case for clearing the cache is missing in forum_update(). What happens if the forum topic is edited and moved to a different forum?
The answer- with this code everything is broken.
I'm not sure how to deal with this. My inital thought was to check against the forum category returned by node_load(), but this is rather hackish since it relies on that data being stale and still in the static variable.
Alternatives? Maybe have to query the {forum} table?
Attached patch fixes this in the crudest possible way- invaliding all the cached values.
#18
found a separate bug in function forum_submit(&$node).
If I choose "leave a shadow copy", any changes to the taxonomy *except* the forum category will be discarded. New issue: http://drupal.org/node/76339
#19
Please review this patch:
http://drupal.org/node/76339#comment-119894
I've structured the bug fix in part to make it easy to have the old and new taxonomy term IDs available in forum_submit for the purpose of selectively clearing the cache. Thus, I'd like to see this sort of patch applied first before working more on this caching patch.
#20
subscribe. will test later
#21
Peter, I have reviewed the issue you mention. Does this mean that you'll re-roll a new patch here once the other one is committed?
#22
Yes, I will certainly be willing to work on this patch more when the forum patch is committed, and assuming Gerhard is still interested in following this avenue. Also, this patch will have to be changed some if splitting the cache table goes ahead: http://drupal.org/node/72290
#23
@killes- please review this related patch: http://drupal.org/node/76339#comment-119894
It was already committed to HEAD.
#24
Ok, since the above-referenced patch for forum.module is in, attached is a patch that does the cache invalidation in forum_submit().
The only other place where it might be worth adding some caching logic would be in function forum_get_topics for the case $user->uid=0.
#25
oops- forget to remove a line of debugging code. Use attached patch (for Drupal 4.7) instead.
#26
fyi, there is now a node_access rebuild button where a site adminh can clean his table after disabling the last node_access module. actually, the button is safe to use anytime.
#27
function forum_delete(&$node) {db_query('DELETE FROM {forum} WHERE nid = %d', $node->nid);
+ cache_clear_all('block:forum:', TRUE);
+ cache_clear_all('result:forum:forum_get_forums:0:', TRUE);
+ cache_clear_all("result:forum:forum_get_forums:1:$node->tid:", TRUE);
+ cache_clear_all("result:forum:forum_topic_navigation:$node->tid:$node->nid:", TRUE);
Looking through the patch, the last line above should probably be:
+ cache_clear_all("result:forum:forum_topic_navigation:$node->tid:", TRUE);#28
I am closing this one on favour of a new, improved approach, see: http://drupal.org/node/80951