Posted by jhodgdon on November 27, 2012 at 3:55pm
4 followers
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | documentation |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | Novice |
Issue Summary
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
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).
Cleaning up the documentation would be a good Novice project I think?
Comments
#1
Here we go.
#2
The last submitted patch, cache_set_docs-1851886-1.patch, failed testing.
#3
The old "base table not found" test bot glitch, sigh.
#4
#1: cache_set_docs-1851886-1.patch queued for re-testing.
#5
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.
#6
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.
#7
Yeah if there is only one * then the need for the * is certainly reduced!
#8
Thought so :)
#9
The last submitted patch, n-z-clean-up-1317628-36.patch, failed testing.
#10
Wow, epic fail.
#11
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.
#12
Ah, thought I had removed the asterisk. Agreed on variable storage and comma.
#13
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!
#14
No reason to compromise :)
#15
OK, I'll get this one committed when it turns green. Thanks!
#16
Turned green! Committed to 7.x. Thanks Albert!
#17
Maybe a nice quick one for 6.x as well?
#18
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.