Factor out the caching pattern from forum

This simplifies the forum code a lot (that's currently the only place we use this pattern since #495968: Introduce drupal_render() cache pattern. Start using it for blocks was committed). there's no API change, just factoring out to a helper function. Looks good apart from some comment issues.

+++ includes/common.inc	2009-12-05 14:48:39 +0000
@@ -5155,6 +5155,46 @@ function show(&$element) {
+ * call this function. Executing the query and formatting results should happen
+ * in pre_render. See forum_block_view() and forum_block_view_pre_render() for
+ * an example.

These should be @see a the end of the docblock instead of inline.

+++ includes/common.inc	2009-12-05 14:48:39 +0000
@@ -5155,6 +5155,46 @@ function show(&$element) {
+ *   A DBTNG query object as returned by db_select().

We must have a name for what this which isn't DBTNG? 'Select query object as returned by db_select()" enough?

Apart from that rtbc.

I'm on crack. Are you, too?

Nice.

Please doc the return value of the new function.

+++ includes/common.inc	2009-12-05 14:48:39 +0000
@@ -5155,6 +5155,46 @@ function show(&$element) {
+ *   added to this string and also it will be used in the caching keys in

"is part of the cache key"

+++ includes/common.inc	2009-12-05 14:48:39 +0000
@@ -5155,6 +5155,46 @@ function show(&$element) {
+ *   Optional arguments to be added to the caching keys. Every argument will

"is part of the cache key"

+++ includes/common.inc	2009-12-05 14:48:39 +0000
@@ -5155,6 +5155,46 @@ function show(&$element) {
+ *   automatically be casted to string so practically only numbers and strings

"cast", not casted

+++ includes/common.inc	2009-12-05 14:48:39 +0000
@@ -5155,6 +5155,46 @@ function show(&$element) {
+ *   the arguments for the query is used for caching keys so usually $args is

"in the cache key"

thanks for the tweaks. nice little refactor.

+++ includes/common.inc	2009-12-05 23:50:51 +0000
@@ -5161,6 +5161,53 @@ function show(&$element) {
+  return array(
...
+  return $elements;

Two returns?

+++ includes/common.inc	2009-12-05 23:50:51 +0000
@@ -5161,6 +5161,53 @@ function show(&$element) {
+    '#pre_render' => array($function . '_pre_render'),

+++ modules/forum/forum.module	2009-12-05 23:09:16 +0000
@@ -628,15 +628,8 @@ function forum_block_view($delta = '') {
-     '#pre_render' => array('forum_block_view_pre_render'),
...
+  $block['content'] = drupal_render_cache_by_query($query, 'forum_block_view');

This looks a bit hard to grok... can't we keep #pre_render outside of the caching helper?

I'm on crack. Are you, too?

+++ modules/forum/forum.module	2009-12-05 23:09:16 +0000
@@ -628,15 +628,8 @@ function forum_block_view($delta = '') {
   $cache_keys[] = md5(serialize(array((string) $query, $query->getArguments())));
...
-     '#cache' => array(
-        'keys' => $cache_keys,
-        'expire' => CACHE_TEMPORARY,
...
+  $block['content'] = drupal_render_cache_by_query($query, 'forum_block_view');

This patch also doesn't seem to pass the original arguments to the new helper function?

This review is powered by Dreditor.

+++ includes/common.inc	2009-12-06 20:06:39 +0000
@@ -5155,6 +5155,53 @@ function show(&$element) {
+function drupal_render_cache_by_query($query, $function, $args = array(), $expire = CACHE_TEMPORARY, $granularity = NULL) {

Shouldn't we flip the position of $expire and $granularity?

I'd suspect that setting a custom granularity is more common than setting a custom cache lifetime.

+++ modules/forum/forum.module	2009-12-06 20:06:39 +0000
@@ -622,21 +622,8 @@ function forum_block_view($delta = '') {
-  $block['subject'] = $title;

Why is the block title removed?

This review is powered by Dreditor.

+++ includes/common.inc	2009-12-06 20:07:13 +0000
@@ -5155,6 +5155,52 @@ function show(&$element) {
+ * @param $args
+ *   Optional arguments which are used when building the cache key. Every
+ *   argument will automatically be cast to string so practically only numbers
+ *   and strings should be passed in here. Use nids instead of nodes and so on.
+ *   Note that the arguments for the query is used in the cache key so usually
+ *   $args is not necessary.

It's not clear to me what $args really is and for what it is used without reading the actual code.

+++ includes/common.inc	2009-12-06 20:07:13 +0000
@@ -5155,6 +5155,52 @@ function show(&$element) {
+ * @param $expire
+ *   The cache expire time, passed eventually to cache_set().
+ * @param $granularity
+ *   One or more granularity constants passed to drupal_render_cid_parts().

I still think that most modules and stuff that use this function will rather want to pass custom $granularity and don't care for the $expire at all, but I'll let others chime in on that.

+++ includes/common.inc	2009-12-06 20:07:13 +0000
@@ -5155,6 +5155,52 @@ function show(&$element) {
+ * @param $granularity
+ *   One or more granularity constants passed to drupal_render_cid_parts().
+ * @return
+ *   A renderable array with the following keys and values:
+ *     #query: the passed in $query.
+ *     #pre_render: $function with a _pre_render suffix.
+ *     #cache: an associative array prepared for drupal_render_cache_set().
+ *   

1) There should be a blank line between last @param and @return.

2) See http://drupal.org/node/1354 for proper formatting of Doxygen lists.

3) Trailing white-space here.

+++ includes/common.inc	2009-12-06 20:07:13 +0000
@@ -5155,6 +5155,52 @@ function show(&$element) {
+ * @see forum_block_view()
+ * @see forum_block_view_pre_render()

Why does this generic function point to forum module functions? Can be removed.

I'm on crack. Are you, too?

Those @see lines are helpful pointers to real life usage of this function. We discussed those earlier.

$args is a hard to explain. we have rewritten it a couple times. please supply something better if you think the current one is insufficient.

The @see lines added here are duplication. This function is called directly, so you get the same function reference like for every other function in the API output. To take an arbitrary example: http://api.drupal.org/api/function/drupal_render_cid_parts/7

Its only duplicate when using api docs. When you are coding, it is quite helpful to have functions listed there. You can click into them and 'goto definition' or copy+search.

I think this patch makes sense, and it makes the code of forum module a heck of a lot more legible.

However, the PHPDoc there, while technically accurate, doesn't help me to understand when/where I would actually use this. We removed a comment in Forum module that explained we were caching the altered query to enable caching with node access. But I see no such hints in the PHPDoc for drupal_render_cache_by_query() (incidentally, we should put that comment back so that people reading Forum module know why we're calling drupal_render_cache_by_query()).

If you want to urge developers to use this liberally, you need to explain to them how to do it and when it's appropriate.

Enhanced the PHPDoc a bit, and moved that code comment back to forum.module. This is all I can think of within the confines on PHPDoc. If this were a blog post, we could give elaborate examples and such. Back to RTBC. Please wait for bot before commit.

Is bot broken? That re-test request is being ignored, and status page looks ok - http://qa.drupal.org/pifr/status

just reupload it

I still don't really find the docs sufficient here for new developers, but I would like to get this in before alpha 1 so that contrib can make use of it.

Committed to HEAD.

Please add to the module upgrade guide.

Changing tagging scheme - this goes in the module upgrade guide I guess

This isn't an API *change* but an addition, so IMO it doesn't belong in the Drupal 6/7 module update guide http://drupal.org/update/modules/6/7.

That page just (or at least mostly) documents changes that would break D6 modules. No one could possibly read that page all the way through, and thereby learn about new functions they should be using -- the only way to use that page is to find out what to do when your module breaks, via control-f search for the function that isn't working any more.

So it probably needs to be documented in the module developer's guide or something like that?

I've also filed a follow-up issue because the PHP-doc for this function is not up to our doxygen standards and as webchick said in #24 it also doesn't really explain what the function is supposed to be used for, as far as I can tell.
#1133710: drupal_render_cache_by_query() doc is unclear and not standardized

Leaving this tagged as "Needs Documentation" because there seems to be a sentiment that it should go into some kind of a module developer guide somewhere? If someone can clarify what kind of thing should be written, that might help.

this issue bring a bug which fix in #914382: Contextual links incompatible with render cache

This was just open for documentation, and I don't think, since 4 years have passed, that it's going to happen. The patch itself was applied years ago.