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?
Files: 
CommentFileSizeAuthor
#27 drupal_cache_docs-863428-27.patch1.94 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,409 pass(es).
[ View ]
#24 drupal_cache_docs-863428-24.patch2.2 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 41,309 pass(es).
[ View ]
#24 interdiff.txt2 KBAlbert Volkman
#22 drupal_cache_docs-863428-22.patch1.17 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 41,297 pass(es).
[ View ]
#19 drupal_cache_docs-863428-19.patch1.6 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,394 pass(es).
[ View ]
#15 drupal_cache_docs-863428-13.patch1.71 KBjhodgdon
PASSED: [[SimpleTest]]: [MySQL] 41,293 pass(es).
[ View ]
#13 drupal_cache_docs-863428-13.patch1.71 KBAlbert Volkman
FAILED: [[SimpleTest]]: [MySQL] 41,306 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#13 interdiff.txt1.49 KBAlbert Volkman
#11 drupal_cache_docs-863428-11.patch3.2 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 41,240 pass(es).
[ View ]
#6 863428.patch4.97 KBMile23
PASSED: [[SimpleTest]]: [MySQL] 32,224 pass(es).
[ View ]

Comments

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.

Can I use the & bitwise operator?

  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).

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

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.

Version:7.x-dev» 8.x-dev
Status:Active» Needs review
Issue tags:+needs backport to D7
StatusFileSize
new4.97 KB
PASSED: [[SimpleTest]]: [MySQL] 32,224 pass(es).
[ View ]

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. :-)

Title:Documentation unclear and incomplete in common.incDRUPAL_CACHE_* documentation unclear and incomplete in common.inc

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new3.2 KB
PASSED: [[SimpleTest]]: [MySQL] 41,240 pass(es).
[ View ]

How's this?

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.

Status:Needs work» Needs review
StatusFileSize
new1.49 KB
new1.71 KB
FAILED: [[SimpleTest]]: [MySQL] 41,306 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Ah, gotcha. Like this?

Status:Needs review» Needs work

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

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.71 KB
PASSED: [[SimpleTest]]: [MySQL] 41,293 pass(es).
[ View ]

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.

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

http://drupal.org/node/1186582

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.

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).

Status:Patch (to be ported)» Needs review
StatusFileSize
new1.6 KB
PASSED: [[SimpleTest]]: [MySQL] 39,394 pass(es).
[ View ]

Backported.

Status:Needs review» Reviewed & tested by the community

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

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.

StatusFileSize
new1.17 KB
PASSED: [[SimpleTest]]: [MySQL] 41,297 pass(es).
[ View ]

This patch resolves everything but your binary combination question.

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...

Status:Needs work» Needs review
StatusFileSize
new2 KB
new2.2 KB
PASSED: [[SimpleTest]]: [MySQL] 41,309 pass(es).
[ View ]

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 :)

Status:Needs review» Reviewed & tested by the community

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

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.

Status:Patch (to be ported)» Needs review
StatusFileSize
new1.94 KB
PASSED: [[SimpleTest]]: [MySQL] 39,409 pass(es).
[ View ]

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.

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

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

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.