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

Files: 
CommentFileSizeAuthor
#17 cache_set_docs-1851886-17.patch4.63 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#14 cache_set_docs-1851886-14.patch5.05 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,784 pass(es).
[ View ]
#14 interdiff.txt698 bytesAlbert Volkman
#12 cache_set_docs-1851886-12.patch5.05 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,627 pass(es).
[ View ]
#12 interdiff.txt1.31 KBAlbert Volkman
#10 cache_set_docs-1851886-10.patch5.06 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,580 pass(es).
[ View ]
#10 interdiff.txt1.34 KBAlbert Volkman
#8 n-z-clean-up-1317628-36.patch43.76 KBAlbert Volkman
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch n-z-clean-up-1317628-36_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 interdiff.txt14.53 KBAlbert Volkman
#6 cache_set_docs-1851886-6.patch5.09 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,570 pass(es).
[ View ]
#6 interdiff.txt3.84 KBAlbert Volkman
#1 cache_set_docs-1851886-1.patch3.8 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,768 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new3.8 KB
PASSED: [[SimpleTest]]: [MySQL] 39,768 pass(es).
[ View ]

Here we go.

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review

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

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.

Status:Needs work» Needs review
StatusFileSize
new3.84 KB
new5.09 KB
PASSED: [[SimpleTest]]: [MySQL] 39,570 pass(es).
[ View ]

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.

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

StatusFileSize
new14.53 KB
new43.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch n-z-clean-up-1317628-36_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thought so :)

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.34 KB
new5.06 KB
PASSED: [[SimpleTest]]: [MySQL] 39,580 pass(es).
[ View ]

Wow, epic fail.

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.

Status:Needs work» Needs review
StatusFileSize
new1.31 KB
new5.05 KB
PASSED: [[SimpleTest]]: [MySQL] 39,627 pass(es).
[ View ]

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

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!

StatusFileSize
new698 bytes
new5.05 KB
PASSED: [[SimpleTest]]: [MySQL] 39,784 pass(es).
[ View ]

No reason to compromise :)

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Fixed

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

Version:7.x-dev» 6.x-dev
Status:Fixed» Needs review
StatusFileSize
new4.63 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

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

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.

Issue summary:View changes

Added issue summary template.