hook_update_N() doc needs an overhaul

This NULL default value for $sandbox is not needed anyway, there will always be a $sandbox passed in.

Should be fixed in D7 first, then backported - even though it's not an actual bug in D7 since we require PHP5, but it's a doc inaccuracy.
Trivial fix, setting straight to RTBC.

re #4: In D7 hooks documentation is included in core files.

I updated contributions/docs/developer/hooks/install.php for D6

Hm. I actually find the proposed patch makes the documentation less clear. $something = NULL is a universal way of communicating that a function parameter is optional. That you have to explicitly spell it out in inline comments below that indicates that we're doing something funny and non-standard.

I lean towards won't fix on this for D7. What am I missing?

Well, [mymodule]_update_xxx() is the basic, well known form of most update functions (the ones that are not multipass), and you find this form in all of core and contrib. Browsing through .install files, it's also a good way to recognize which ones are multistep or not.

It seems strange that the doc should say different and that we start to see [mymodule]_update_xxx(&$sandbox = NULL) for non-multipass updates flourish in contrib.

Besides, the incoming $sandbox param is *never* NULL, it's just that it can and should be overlooked by 'simple' update funcs. So I'm not even sure the current doc is accurate.

As webchick already said using a default value in a function declaration documents that the parameter is optional

... in the sense of "the caller might not provide the argument". This is not the case here. The doc is wrong and misleading even for D7.

Hm. Ok. Well then let's at least move this documentation up into the @param stuff in the PHPDoc rather than in the function body where no one will read it.

Committed to CVS HEAD.

re webchick #9: Fair enough. That doc needed an overhaul anyway. What about that one ?
Side note: I removed any reference of the batch API. The update 'engine' internally relies on batch API, but update authors should not really have to bother with this IMO.

Patch applies cleanly and everything makes sense

Not part of the patch, but part of the documentation, should the text after @endlink be wrapped?

Currently:

 * Further information about releases and release numbers:
 * - @link http://drupal.org/handbook/version-info http://drupal.org/handbook/version-info @endlink
 * - @link http://drupal.org/node/93999 http://drupal.org/node/93999 @endlink (Overview of contributions branches and tags)
 * - @link http://drupal.org/handbook/cvs/releases http://drupal.org/handbook/cvs/releases @endlink

e.g the contributions branches and tags.

Can we change the first line to something a bit more descriptive than "Perform a single update."? What kind of update?

Also, why is the 2nd paragraph suddenly mentioning "patches", and grammatically, the "which" there needs a comma before it.

Next section:

Database updates consist of 3 parts:

This should say "Numbering for update functions consist..." instead of "Database updates consist"

Also, can we take "simply" out of all of the code comments in the hook function body?

One more thing:

 Substract 1 to disregard uid 0...

That comment does not follow Drupal coding standards, and also "subtract" is misspelled.

Reviving this issue... Patch needs a reroll for sure, and issues mentioned above need to be fixed. Also it may not conform to current docs coding standards.

#11: system_hook_update_N.patch queued for re-testing.

Re-roll, issues addressed, spelling fixed. Some issues have already been resolved e.g. #1619482: hook_update_n() docs should explain magic with docblock text being used inUI

D'oh.

Thanks for the new patch(es)!

A few thoughts:

a) I don't think I like the first line change. Some update update hooks do database updates, and some do other types of updates. I also don't think we need "module-related", since for instance some core updates in system.install are not related to particular modules. [Ooops. Looking back I see that I requested the first line to be changed in comment #13... but anyway I still don't think this particular change is great.]

b) Why are we removing

-  // For non-multipass updates, the signature can simply be;
-  // function hook_update_N() {
-

? I think we should maybe leave that there, and maybe we should add a note to the $sandbox @param docs to omit the parameter entirely if you don't need it?

c)

+  // To display a message to the user when the update is completed, return it.
+  // If you do not want to display a completion message, return nothing.
+  return t('The update did what it was supposed to do.');

Can we make a more realistic example here of a good message to return? Same with the exception.

d)

-    // We'll -1 to disregard the uid 0...
+    // Subtract 1 to disregard uid 0...

Let's follow our own docs standards. Comments should end with . not .... :)

We did a lot of changes to the hook_update_N() docs for 8 a while back on another issue. At this point I think I'll just close this issue; if someone cares deeply about updating 7's docs feel free to reopen this really old issue and make a new patch for 7.