Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#17 | cache_set_docs-1851886-17.patch | 4.63 KB | Albert Volkman |
#14 | cache_set_docs-1851886-14.patch | 5.05 KB | Albert Volkman |
#14 | interdiff.txt | 698 bytes | Albert Volkman |
#12 | cache_set_docs-1851886-12.patch | 5.05 KB | Albert Volkman |
#12 | interdiff.txt | 1.31 KB | Albert Volkman |
Comments
Comment #1
Albert Volkman CreditAttribution: Albert Volkman commentedHere we go.
Comment #3
jhodgdonThe old "base table not found" test bot glitch, sigh.
Comment #4
jhodgdon#1: cache_set_docs-1851886-1.patch queued for re-testing.
Comment #5
jhodgdonThanks, this is a vast improvement! A few things still need attention:
a) List items such as
need to start with a capital letter and end with a period.
b) Also, when reading the patched documentation, I came to this:
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.
Comment #6
Albert Volkman CreditAttribution: Albert Volkman commentedThink 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.
Comment #7
jhodgdonYeah if there is only one * then the need for the * is certainly reduced!
Comment #8
Albert Volkman CreditAttribution: Albert Volkman commentedThought so :)
Comment #10
Albert Volkman CreditAttribution: Albert Volkman commentedWow, epic fail.
Comment #11
jhodgdonThanks -- 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.
Comment #12
Albert Volkman CreditAttribution: Albert Volkman commentedAh, thought I had removed the asterisk. Agreed on variable storage and comma.
Comment #13
jhodgdonThanks! I still think:
should be "etc.)." but I would be willing to let that go. :) Everything else looks great!
Comment #14
Albert Volkman CreditAttribution: Albert Volkman commentedNo reason to compromise :)
Comment #15
jhodgdonOK, I'll get this one committed when it turns green. Thanks!
Comment #16
jhodgdonTurned green! Committed to 7.x. Thanks Albert!
Comment #17
Albert Volkman CreditAttribution: Albert Volkman commentedMaybe a nice quick one for 6.x as well?
Comment #18
jhodgdonI'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.
Comment #19
PavanL CreditAttribution: PavanL commentedAdded issue summary template.
Comment #20
jhodgdonI 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.