This patch adds an addional fieldset on the block configure page which gives you the ability to change the cache setting of this block.

CommentFileSizeAuthor
#12 347350.patch6.42 KBswentel
#11 347350.patch6.36 KBswentel
#10 blockcache_alwayson_fieldset.patch6.22 KBswentel
#8 blockcache_alwayson_fieldset.patch5.73 KBswentel
#8 blockcache_fieldset.patch3.57 KBswentel
blockcache_fieldset.patch3.42 KBswentel

Comments

calebgilbert’s picture

calebgilbert commented

Haven't reviewed this patch specifically, but as someone who helped (very slightly) get block caching in Drupal 6, I can attest to the limited functionality that it enabled (versus say, the blockcache module).

Guess without downloading everything and patching this in so I can see for myself - the only question I have is - does this go far enough to improve Drupal core's block caching? (and if not what else would be helpful?)

mfer’s picture

mfer commented

subscribe.

PixelClever’s picture

PixelClever commented

Subscribe

catch’s picture

catch
he/him
Primary language English
commented
Category: feature » task
Status: Needs review » Needs work

I've been using blockcache_alter module in D6 and it's really handy.

We should update the queries to dbtng while we're in there. And am I going mad or does there need to be a check for block caching being enabled for this form to appear? Or is that too late maybe? If we want it all the time, we should note that block caching will only work if it's enabled at admin/settings/performance.

swentel’s picture

swentel commented

And am I going mad or does there need to be a check for block caching being enabled for this form to appear?

Lets show the form always but show a warning that it will only affect when block caching is enabled, I'll post a patch for that this week.

(off topic, we should really kill hook_access_grants too, but that's for another issue)

PixelClever’s picture

PixelClever commented

In my opinion the per-block cache setting should override the global settings for block caching. In Drupal 6 you can't turn on block caching globally when there are any modules setting access permissions for content. I hate this because it means you have to choose between access control and performance, when there really is no reason we couldn't have both if block caching was controlled on a more granular level. I really like the block_cache_alter module for this very reason. If you can choose which blocks you cache then there is no reason that the access rules should stop you.

catch’s picture

catch
he/him
Primary language English
commented

Yeah with this UI in core, we can remove the disabling of block caching if a node grants module is enabled, and just have a warning instead and/or make everything 'per user' by default if such a module gets switched on.

swentel’s picture

swentel commented
StatusFileSize
new blockcache_fieldset.patch3.57 KB
new blockcache_alwayson_fieldset.patch5.73 KB

Ok, two patches
- first patch shows a warning in the UI and a link to the performance page when block caching is disabled
- second patch removes the block cache setting completely from the performance page and block cache is always enabled

Still keeping it CNW re DBTNG conversion. Also because to hear some opinions which of the two patches looks more logical to go in.

catch’s picture

catch
he/him
Primary language English
commented

I'd probably go for 'always on' - although we need a warning somewhere about the node grants issue switching block caching off -but we also need to fix that oddity for D7 too - which this patch gets us some way towards.

swentel’s picture

swentel commented
Status: Needs work » Needs review
StatusFileSize
new blockcache_alwayson_fieldset.patch6.22 KB

Ok, going for block caching always on. With this patch, the performance page now shows a warning if any modules implementing node grants are enabled. Setting for review as I think we should handle DBNTG and killing node grants in another issue, otherwhise this will end up dead I think.

swentel’s picture

swentel commented
StatusFileSize
new 347350.patch6.36 KB

Reroll of hte patch

swentel’s picture

swentel commented
StatusFileSize
new 347350.patch6.42 KB

Now that block module is optional, the node grants warning should only be shown when block module is enabled.

dries’s picture

dries commented

I'm not convinced the UI belongs in core. If we add this UI, we're asking our users to obtain reasonably in depth understanding of Drupal's cache system, permission system and access control system. For a user to be able to use this comfortably, he needs to be able think as a developer. Someone with the 'administer blocks' permission shouldn't be a Drupal expert. Therefore, it seems better to keep the UI as a contributed module because 90% of the users are not going to need this.

Can't we make this automatic and remove the UI? If we can't, it proofs my theory. :)

swentel’s picture

swentel commented

@Dries: damn, I hate it when you are right :) I'm not going to try and convince you to commit this patch, in fact I'd be really happy to close this issue as this is indeed more a task for a contrib module (which actually exists heh). There are 3 other issues concerning block caching which need attention imo:

  • http://drupal.org/node/235673 - which IMHO needs a backport to D6 too so views can profit from it (see http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/views/plugi... line 86)
  • Kill node grants check (with hopefully a backport to D6 too). There is no issue for this at this point, but I've been talking with catch about this and having another way to check when a module implements the node_grants hook and resetting the block caching completly in such a way a developer can still override it if he wants to, would be *awesome* - and not that hard to code.
  • We need to move the code which adds the 'block cache' fieldset on the performance page from the system module to the block module (since block module is optional now) as Moshe also suggests (see http://drupal.org/node/367380#comment-1438152). No issue for that right now too, but this should be a small no-brainer patch.

I'm following the first one quite closely and I can write patches for 2 and 3 quite soon - let me know if I should post a follup up for the 3rd one on the same issue or not (if you think it's important of course).

btw, Tom Boonen will win tomorrow :) (and everyone not understanding this line, don't worry, it's a Belgian thing heh

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Closed (won't fix)

I think it makes sense to leave the UI in contrib, and look at either a little bit more automation, or at least fixing more TODOs from the original patch. Re-opened and/or created issues for the ones mentioned by swentel.

#186636: Block caching when node access modules are enabled.
#424252: Move block caching UI to block module
#186638: Add a 'keep until' column for cache tables