Starting on line 95:

**
 * Constants defining cache granularity for blocks and renderable arrays.
 *
 * Modules specify the caching patterns for their blocks using binary
 * combinations of these constants in their hook_block_info():
 *   $block[delta]['cache'] = DRUPAL_CACHE_PER_ROLE | DRUPAL_CACHE_PER_PAGE;
 * DRUPAL_CACHE_PER_ROLE is used as a default when no caching pattern is
 * specified. Use DRUPAL_CACHE_CUSTOM to disable standard block cache and
 * implement
 *
 * The block cache is cleared in cache_clear_all(), and uses the same clearing
 * policy than page cache (node, comment, user, taxonomy added or updated...).
 * Blocks requiring more fine-grained clearing might consider disabling the
 * built-in block cache (DRUPAL_NO_CACHE) and roll their own.
 *
 * Note that user 1 is excluded from block caching.
 */

1. In the first example, what does it actually mean, in terms of Drupal caching, to use DRUPAL_CACHE_PER_ROLE | DRUPAL_CACHE_PER_PAGE? More examples are needed (look at error handling in php.ini for a good example of good documentation)
2. What is DRUPAL_CACHE_CUSTOM, and what am I supposed to do with it?
3. What's the difference between DRUPAL_CACHE_CUSTOM and rolling your own and DRUPAL_NO_CACHE and rolling your own?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

robertDouglass’s picture

Furthermore, advice such as "The DRUPAL_CACHE_PER_ROLE and DRUPAL_CACHE_PER_USER constants should not be combined with the bitwise OR operator (|), as the two caching modes are mutually exclusive." (from Pro Drupal Development) would be useful here.

robertDouglass’s picture

Can I use the & bitwise operator?

apaderno’s picture

  1. Can I use the & bitwise operator?

    Considering the values used for DRUPAL_CACHE_PER_ROLE, DRUPAL_CACHE_PER_USER, DRUPAL_CACHE_PER_PAGE, and DRUPAL_CACHE_GLOBAL, the value of DRUPAL_CACHE_PER_USER & DRUPAL_CACHE_PER_PAGE is 0.

  2. What is DRUPAL_CACHE_CUSTOM, and what am I supposed to do with it?

    The constant DRUPAL_CACHE_CUSTOM is described as

    The block is handling its own caching in its hook_block_view(). From the perspective of the block cache system, this is equivalent to DRUPAL_NO_CACHE. Useful when time based expiration is needed or a site uses a node access which invalidates standard block cache.

  3. What's the difference between DRUPAL_CACHE_CUSTOM and rolling your own and DRUPAL_NO_CACHE and rolling your own?

    The difference is that using DRUPAL_CACHE_CUSTOM, the module is using its own block cache it handles with custom code; when a module uses DRUPAL_NO_CACHE, it says the block should not be cached (and it doesn't implement a custom cache).

robertDouglass’s picture

Thanks kiamlaluno. Want to take a crack at turning that into a patch for the D7 code comments in common.inc?

jhodgdon’s picture

This documentation block is also not useful as it is. Although it starts with /**, it will not appear on api.drupal.org, because it isn't documenting anything (it isn't followed by a function, constant, etc., and it isn't a @defgroup, etc.).

So it needs to be either incorporated into one of the contsants' doc (and then the others can have a line saying something like "See WHATEVER_CONSTANT for more information on using this constant), or it needs to be made into a group.

Mile23’s picture

Version: 7.x-dev » 8.x-dev
Status: Active » Needs review
Issue tags: +Needs backport to D7
FileSize
4.97 KB

Seems like there should already be a caching API group, but there isn't. Sadly, I don't have time to properly research and write about it, so here's a patch where one constant gets the larger treatment, and the others get 'See that other one for more details.'

Also a poke in the eye of block.api.php, which omits DRUPAL_CACHE_CUSTOM from hook_block_info(), causing confusion here: #1200794: Block Example involving custom caching

Having just read the code, I seriously doubt there will ever be a consequence to distinguishing between DRUPAL_NO_CACHE and DRUPAL_CACHE_CUSTOM, and anything with the word 'custom' in it should be removed from Drupal forever. But nevertheless, here we are. :-)

Mile23’s picture

Title: Documentation unclear and incomplete in common.inc » DRUPAL_CACHE_* documentation unclear and incomplete in common.inc
jhodgdon’s picture

Status: Needs review » Needs work

The docblocks in this patch do not conform to our docblock standards:
http://drupal.org/node/1354

In particular, they all need to start with a one-line summary of the particular item that is being documented. That's as far as I got in reviewing the patch. There are standards on that page for how to document constants.

Mile23’s picture

This leads me to two conclusions: 1) The original documentation shouldn't have passed muster, and 2) Someone needs to define a group, probably in cache.inc, since there is no way to create a one-sentence summary of both the need for DRUPAL_NO_CACHE and also a more general discussion of these flags.

jhodgdon’s picture

The original doc probably wasn't reviewed properly. I can't help that. Sorry. But this patch is not going in until the doc conforms to our doc standards. Sorry.

And yes, there probably does need to be a group.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
3.2 KB

How's this?

jhodgdon’s picture

Status: Needs review » Needs work

Looks pretty good, thanks!

But we need to define a @defgroup, and then all the generic "about block caching" text should be taken out of the BLOCK_NO_CACHE docs and put in there, rather than linking all the other constant docs to BLOCK_NO_CACHE.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
1.49 KB
1.71 KB

Ah, gotcha. Like this?

Status: Needs review » Needs work

The last submitted patch, drupal_cache_docs-863428-13.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.71 KB

Perfect! I'm not sure what that test failure was... I'm going to upload the patch again to preserve that test result (so I can file another issue) and I'll get the patch committed shortly.

Albert Volkman’s picture

Yeah, it seems like a false-positive. I had that same issue over here-

http://drupal.org/node/1186582

jhodgdon’s picture

I filed an issue for that test failure:
#1783656: Intermittent test failure in testBulkImportUpdateExisting
Since you've seen it elsewhere I'll up the priority.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks! Committed to 8.x. Needs port for 7.x (I'm not sure if the constants are exactly the same and in any case this patch does not apply there as it is).

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.6 KB

Backported.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good -- I'll get it committed shortly -- thanks!

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work

I committed this patch to 7.x, thanks!

But I realized we need to go back to 8.x now that we have this nice doc group and fix up the docs a bit. The group docs currently say:

/**
 * @defgroup block_caching Block Caching
 * @{
 * Constants that define each block's caching state.
 *
 * Modules specify the caching patterns for their blocks using binary
 * combinations of these constants in their hook_block_info().
 * DRUPAL_CACHE_PER_ROLE is used as a default when no caching pattern is
 * specified. Use DRUPAL_CACHE_CUSTOM to disable standard block cache and
 * implement
 *
 * The block cache is cleared when the 'content' cache tag is invalidated,
 * following the same pattern as the page cache (node, comment, user, taxonomy
 * added or updated...). Blocks requiring more fine-grained clearing might
 * consider disabling the built-in block cache (DRUPAL_NO_CACHE)
 * and roll their own.
 *
 * Note that user 1 is excluded from block caching.
 */

Obviously that first paragraph is missing something... Or actually probably that last partial sentence should be removed (since that information is in the DRUPAL_CACHE_CUSTOM doc block).

Also, looking at the DRUPAL_CACHE_CUSTOM block that is there now, I think we should remove the line that says it's the same as DRUPAL_NO_CACHE, given the information in comments at the top of that issue. It's not the same -- custom means the module is implementing its own system (as stated in the docs now), and no cache means no caching. They are semantically different; let's not confuse the issue by saying they're the same, even if the block system does the same thing with them. What I'm talking about is this sentence from the DRUPAL_CACHE_CUSTOM docs:

 * From the perspective of the block cache system, this is equivalent to
 * DRUPAL_NO_CACHE.

Finally, I think that the statement in the group docs about binary combinations should be modified slightly. The no cache and custom constants are negative and they aren't intended to be binary-combined at all. I'm also not sure what the binary combinations would mean.. for instance what would global | per role really do in the block system? Global says it's the same for every user and page, but per role says it can change per role... So this whole idea of binary combination needs to be figured out.

Albert Volkman’s picture

This patch resolves everything but your binary combination question.

jhodgdon’s picture

Yes, that looks good so far. One typo:

+ * This settings is seful when time based expiration is needed or a site uses a

seful -> useful. doh! :)

I also noticed this at the end of the group docs:

* added or updated...). Blocks requiring more fine-grained clearing might
* consider disabling the built-in block cache (DRUPAL_NO_CACHE)
* and roll their own.

I think this should suggest DRUPAL_CACHE_CUSTOM, right?

Regarding the binary combination wording... I looked in core at all existing hook_block_info() implementations, and there are two (book and profile modules) that use this combination:

DRUPAL_CACHE_PER_PAGE | DRUPAL_CACHE_PER_ROLE

Which seems to mean that the block has to be cached by combination of page and role... hah! I found the function where this caching stuff is done: drupal_render_cid_parts()

So. It looks like you can combine either per page or per user with per role (oops, edit) per role or per user with per page (end of edit), but that is the only combining that actually does anything, and what it does is to make sure that the unique cache ID contains both the user/role and the page URL (which is what you'd expect).

So how can we say this? Maybe replace this:

* Modules specify the caching patterns for their blocks using binary
* combinations of these constants in their hook_block_info().

with something like this:

Modules specify how their blocks can be cached in their hook_block_info() implementations. Caching can be turned off (DRUPAL_NO_CACHE), managed by the module declaring the block (DRUPAL_CACHE_CUSTOM), or managed by the core Block module. If the Block module is managing the cache, you can specify that the block is the same for every page and user (DRUPAL_CACHE_GLOBAL), or that it can change depending on the page (DRUPAL_CACHE_PER_PAGE) or by user (DRUPAL_CACHE_PER_ROLE or DRUPAL_CACHE_PER_USER). Page and user settings can be combined with a bitwise-binary or operator; for example, DRUPAL_CACHE_PER_ROLE | DRUPAL_CACHE_PER_PAGE means that the block can change depending on the user role or page it is on.

Is that too wordy? It's going on a group/topic page so I think it's OK...

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
2 KB
2.2 KB

Typo fixed. You're right, that should be DRUPAL_CACHE_CUSTOM, I think. Also, I don't think that's too wordy. Pretty useful for someone who is looking for information on these caching options. Great job :)

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

This now seems ready to go... I'll get it committed. Thanks!

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks! Committed to 8.x. This one needs a reroll for 7.x.

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.94 KB

Re-rolled.

Status: Needs review » Needs work
Issue tags: -Needs backport to D7

The last submitted patch, drupal_cache_docs-863428-27.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7

#27: drupal_cache_docs-863428-27.patch queued for re-testing.

jhodgdon’s picture

Status: Needs review » Fixed

Thanks! Committed to 7.x. I think this one is now done. :)

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.