Updated: Comment #19

Problem/Motivation

The documentation in includes/cache.inc in 7.x for cache_set uses a bunch of lists, but they are not formatted according to our standards:
http://drupal.org/node/1354#lists

Proposed resolution

Note that the line immediately before a list should end in : (so you might have to add a new sentence to act as a list header).

Remaining tasks

Task needs work.

User interface changes

The line immediately before a list should end in :

API changes

None.

Original report by jhodgdon

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Albert Volkman’s picture

Status: Active » Needs review
FileSize
3.8 KB

Here we go.

Status: Needs review » Needs work

The last submitted patch, cache_set_docs-1851886-1.patch, failed testing.

jhodgdon’s picture

The old "base table not found" test bot glitch, sigh.

jhodgdon’s picture

Status: Needs work » Needs review

#1: cache_set_docs-1851886-1.patch queued for re-testing.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks, this is a vast improvement! A few things still need attention:

a) List items such as

 * - smaller bins mean smaller database tables and allow for faster selects and
 *   inserts

need to start with a capital letter and end with a period.

b) Also, when reading the patched documentation, I came to this:

...
 * might break functionality or are extremely expensive to recalculate. These
 * will be marked with a (*). The other bins expired automatically by core.
 

I was wondering... where are they marked with a *? Then way down lower I finally came to the list of bins (which is in the right place now), which is where the * markings are.

So maybe we could remove that sentence from where it is now (and add a verb to the following sentence "...other bins *are* expired..."). And then we could add something to the list header in @param $bin, so it would say something like:

@param $bin
The cache bin to store the data in. Valid core values (with bins that should not be flushed before their expire time marked with *) are:

c) There should not be a blank line between @param $bin and @param $expire.

d) This is not the fault of your patch, but with the new formatting it's very obvious that some of the cache bins have spaces in the names and some have underscores. So I looked at the callers of cache_set() in core and found:
cache [that's the default]
cache_block
cache_bootstrap
cache_field
cache_filter
cache_form
cache_menu
cache_page
cache_path

Render elements can also set their cache bin, and the ThemeRegistry and CacheArray classes can set their bins (both of these classes default to 'cache' as the bin though).

Anyway, it looks like they should all have _ in the names, and we're missing cache_bootstrap in the list. Also, it appears that cache_update is managed separately by update.module in function _update_cache_set() and associated functions, so we probably shouldn't mention it as part of cache_set(). Perhaps a @see _update_cache_set() would be more appropriate?

And the bootstrap cache is used for: the class registry, the system list of modules, the list of which modules implement which hooks, and the Drupal variable list.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
3.84 KB
5.09 KB

Think I addressed all the issues outlined in #5. Also included an interdiff, but not sure how helpful that is in on a small patch as such.

With the removal of cache_update(), we now only have one asterisk. I wonder if I should just make a note of that in cache_form() instead.

jhodgdon’s picture

Yeah if there is only one * then the need for the * is certainly reduced!

Albert Volkman’s picture

FileSize
14.53 KB
43.76 KB

Thought so :)

Status: Needs review » Needs work

The last submitted patch, n-z-clean-up-1317628-36.patch, failed testing.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
1.34 KB
5.06 KB

Wow, epic fail.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks -- much better! Just a couple of things left to fix:

- I think the (*) on cache_form should be removed.
- Both the generic 'cache' and 'cache_bootstrap' say that they are used for variable storage. Looking at variable_set(), it looks to me as though 'cache_bootstrap' is correct.
- I think there should be a comma before "etc" (and "etc" should be "etc.") in the 'cache' item.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
1.31 KB
5.05 KB

Ah, thought I had removed the asterisk. Agreed on variable storage and comma.

jhodgdon’s picture

Thanks! I still think:

+ *     locale date, list of simpletest tests, etc).

should be "etc.)." but I would be willing to let that go. :) Everything else looks great!

Albert Volkman’s picture

No reason to compromise :)

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

OK, I'll get this one committed when it turns green. Thanks!

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Turned green! Committed to 7.x. Thanks Albert!

Albert Volkman’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Needs review
FileSize
4.63 KB

Maybe a nice quick one for 6.x as well?

jhodgdon’s picture

Status: Needs review » Needs work

I'm not sure we can blindly port the D7 patch to D6 -- in particular, I think the list of what belongs in what cache bin is quite likely different in D6. And there is no simpletest in D6 core. So this patch is not OK as it is... someone needs to research what goes in what bin for d6 and make a more appropriate patch.

PavanL’s picture

Issue summary: View changes

Added issue summary template.

jhodgdon’s picture

Version: 6.x-dev » 7.x-dev
Status: Needs work » Closed (fixed)

I am cleaning up old 6.x documentation issues. At this point, we are not spending effort fixing them. Sorry.

This one started as 7.x and was fixed, so moving back there.