Jeff, here is cleaned up version of blockcache.module. IMO this module is promising and I will work on it if time permits.
I have some questions additionally:
- Why didn't you use _block_rehash() for generating the current list of implemented blocks?
- I was wondering if we really have to duplicate each block - wouldn't it be possible to use hook_block() to assign a cached block output instead of the uncached by leveraging the module weight field in the system table?
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | blockcache.cleanup_0.patch | 3.32 KB | sun |
| #2 | blockcache.cleanup.patch | 7.19 KB | sun |
Comments
Comment #1
jjeff commentedI think you forgot to attach the patch/module.
The problem with _block_rehash() is that the module is requesting a list of blocks during a request for a list of blocks. So I wanted to avoid a recursive infinite loop. That being said, I had originally written the module to just query the blocks table (which will have been recently rehashed by the block module), but I took this out when I was tracking down a bug. Perhaps I'll try putting it back in, but things seem to be working fine as they are right now.
As far as not duplicating blocks, the current solution is what seemed to be the cleanest. I would actually prefer to do a (major) hook_form_alter() on the block listing form and add a "cached" checkbox column to the table. But this would be a lot of work because the block configuration forms would also need to be altered AND then we'd need to figure out how to switch the enabled blocks to the cached version. All in all, it would be a cleaner user interface, which I'm all for... but I thought for the first version of the module, I'd just get the concept "out there" without spending weeks hacking through form_alter...
Comment #2
sunSorry, here is the patch.
Will definitely have a look on the integrated form_alter way.
Comment #3
jjeff commentedNot sure why but I couldn't get the patch to apply. I've also made several updates to the module including the addition of per-user caching. Not sure if it's still necessary, but if you'd like, please reroll this patch against the new HEAD version of the module. Thanks.
Comment #4
sunOld patch was against 4.7, my fault.
New patch is against CVS. You've indeed changed some lines identically :). Here comes the rest.
Comment #5
jjeff commentedCommitted. Thanks!
Comment #6
(not verified) commented