Improve code comments for static caching API

Patch includes drupal_cache naming, and drupal_cache_reset node on reset. The extra text for reset needs to be reviewed particularly, i'm not confident that is the right wording.

This line runs longer than 80 characters.

+ * Should more than one variable need caching, append the variable to the function or method

fixed comments to up to 80 chars long.

Tagging for documentation team review. Also:

+ * Should more than one variable need caching, append the variable to the 
+ * function or method with ':'.

I found this confusing until I read it three times. Perhaps reword as:

+ * If more than one variable needs to be cached, append each name to the
+ * function or method name with a ':' character, as in the following example.

The following construct lacks a clear antecedent for the indefinite pronouns "This" and "It".

+ *   This should rarely, if ever, be omitted.  It is only for extreme cases
+ *   where all data is being reset, such as with simpletest.

Please reword as "This parameter should only be omitted ..." or "This parameter should never be omitted except ..." and describe the exact case where it would be appropriate to clear all static variables.

Also, this should be 8.x

And then backported.

works in D7 and D8

@#10 by Mac_Weber on October 18, 2011 at 3:48pm:

+ * naming scheme "__CLASS__ . '::' . __METHOD__" should be used in
+ * substitute for __FUNCTION__.

The pharase "should be used in substitute for" sounds awkward and non-standard to me. Please replace with "should be used instead of" or "should be use in place of" or else swap the order around and use "should be replaced with" or find another phrasing that sounds comfortable to native English speakers.

Otherwise the patch looks fine to me, but of course jhodgon has the final say.

I agree. Sounds better.

+++ b/includes/bootstrap.incundefined
@@ -3116,6 +3116,18 @@ function registry_update() {
+ * If more than one variable needs to be cached, append each name to the ¶

Trailing whitespace here.

Also, note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

Tagging as novice for the task of rerolling the Drupal 8.x patch.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC

Per comment #13, re-roll for D8/core, remove whitespace, merge to one patch

#14: improve_static_cache_comments-422352-14.patch queued for re-testing.

+++ b/core/includes/bootstrap.inc
@@ -3154,6 +3154,18 @@ function registry_update() {
+ * In cases where drupal_static needs to be called from a singleton class, the
+ * naming scheme "__CLASS__ . '::' . __METHOD__" should be used in place of
+ * __FUNCTION__.

For consistency, I'm wondering if:

__FUNCTION__

should be in double quotes since that is used for:

"__CLASS__ . '::' . __METHOD__"

?

Also, wouldn't an example be good here too or is the description sufficient? Or, perhaps combined with the following example?

+++ b/core/includes/bootstrap.inc
@@ -3154,6 +3154,18 @@ function registry_update() {
  * In a few cases, a function can have certainty that there is no legitimate
  * use-case for resetting that function's static variable. This is rare,
  * because when writing a function, it's hard to forecast all the situations in

This isn't part of the changes, but the wording:

"...a function can have certainty that there is no legitimate use-case..."

seems very awkward to me. I'm not even sure what is really meant. Perhaps, it means something like this?

"There are rarely legitimate use cases for resetting a function's static variable."

Programmers should provide the ability to reset static function variables unless there can be no legitimate use-case for resetting them. These rare exceptions should be justified by inline documentation.

The text in #19 from @pillarsdotnet is human-readable :)

included suggestions from #18 an #20

+ * it's ok to use the "static" keyword instead of the drupal_static() function.

In general we avoid contractions and indefinite pronouns. Perhaps this would be better:

+ * the "static" keyword may be used instead of the drupal_static() function.

corrections as suggested on #23

  *   Name of the static variable to reset. Omit to reset all variables.
+ *   This parameter should never be omitted; except in extreme cases where all
+ *   data is being reset, such as with simpletest.

Should wrap at 80 columns. Maybe delete the "Omit to reset all variables." sentence? Or maybe change to "Omit to reset all variables, but only in extreme cases such as with simpletest."

Otherwise RTBC, I think, but of course jhodgdon has the final word.

#24: improve_static_cache_comments-422352-24.patch queued for re-testing.

The semicolon does not belong in the following sentence:

This parameter should never be omitted; except in extreme cases where all data is being reset, such as with simpletest.

The sentence is self-contradictory. Never has no exceptions.

Consider something like "This parameter should only be omitted in extreme cases..."

Given #1577902: [META] Remove all usages of drupal_static() & drupal_static_reset() , I think we should leave D8 alone and move this to D7.

For D8 it's good to have bad docs on these functions. Ideally, none at all. So that people stop using it. ;)

Think I have enough of a handle on the issue to be able to comment now.
Given that most of the remaining calls to drupal_static are in hooks that are not going to be removed prior to D8 release, I think we're going to have to live with these functions being in core, and therefore need do need some kind of documentation.
I would argue that we (is it we already), I mean someone, should change the doc to reflect that yes, these functions are here, but please don't use them. Please use OOP and protected properties, instead.

I agree with @sun at #29. Changing version.

looks like it tried to run the last patch against 7.x
I'll switch it back in a minute, after the test run.

+++ b/core/includes/bootstrap.inc
@@ -1122,97 +1122,8 @@ function drupal_classloader_register($name, $path) {
+ * See https://wwww.drupal.org/node/159902

@@ -1267,6 +1178,9 @@ function &drupal_static($name, $default_value = NULL, $reset = FALSE) {
+ * See https://wwww.drupal.org/node/159902

One two many w's in that domain name.

And really, these are deprecated, wow. I use them all over the place! Glad I found this to read, thanks @Mac_Weber

Oh also the page is a 404 https://www.drupal.org/node/159902

Drupal 8 it looks like this may be a duplicate of #2260187: [PP-1] Deprecate drupal_static() and drupal_static_reset()

For Drupal 7 I don't think we should be treating this as depracated and would consider close-won't fix/duplicate this issue as we don't have OOP as much in D7 to fallback on.

Marking won't fix based on #40