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.
API page: http://api.drupal.org/api/drupal/modules%21system%21system.api.php/funct...
Enter a descriptive title (above) relating to hook_update_N, then describe the problem you have found:
The first line (AFAICT) of the hook implementation's docblock is used as a UI text at update.php. The docs should explain this.
Comment | File | Size | Author |
---|---|---|---|
#21 | 1619482-16-hook_update_N-docs-D7.patch | 4.41 KB | jhodgdon |
#19 | 1619482-16-hook_update_N-docs-D8.patch | 721 bytes | jhodgdon |
#16 | 1619482-16-hook_update_N-docs-D7.patch | 4.41 KB | mr.baileys |
#16 | 1619482-16-hook_update_N-docs-D8.patch | 721 bytes | mr.baileys |
#14 | 1619482-14-hook_update_N-docs.patch | 4.42 KB | mr.baileys |
Comments
Comment #1
jhodgdonHah, I thought we had put that in there... I guess not though. Good idea...
Comment #2
mr.baileysThe update task description is actually the entire docblock for the function, stripped of newlines, '*' and '/' (causing it to look like one long line/paragraph). Patch attached to add this information, and also clarified the difference between the message returned from hook_update_N (displayed after task completion) and the documentation block (displayed on the pending task list).
Comment #3
mr.baileysFor reference, the code that pulls the documentation block from the hook_update_N functions is in
update_get_update_list()
Comment #4
jhodgdonThat's correct -- thanks for being careful and precise!
A couple of minor things that I think should be addressed:
a) The wording could use "the" in "... used as *the* description of..." I think?
b) Placement of the new paragraph -- as it is in this patch, it's coming between the explanation of how to compose update numbers and the line saying "never re-number" (which seems related to numbering).
I think maybe instead we should redo the top part of the documentation slightly and put it there. Currently, it says:
How about if we make it say something more like this:
---
For each patch that requires a database change, add a new hook_update_N() to your module's .install file, which will be listed as an available update when update.php is run. [new text about docblock goes here] Schema updates should adhere to the Schema API: http://drupal.org/node/150215
Implementations of hook_update_N() are named (module name)_update_(number). The numbers are composed of three parts:
[existing description of numbering scheme]
---
Comment #5
mr.baileysMakes sense, updated patch attached. I did slightly reword the opening paragraph since the term "patch" is not really appropriate (if I update a module for one of my sites, I don't first roll a patch and then apply it).
Comment #6
jhodgdonI agree about the term "patch"... a couple of grammatical/typographical/formatting things to fix:
a)
- on -> one ["requires *one* or more actions].
- need comma before "which".
b)
Since this goes way past 80 characters, move the @link ... @endlink to the next line... Oh... Actually, there is no point to this @link at all, since the link text is the URL. So either make it @link Schema API (url) @endlink, or remove the @link entirely.
c) This @link...@endlink is also pointless (not your fault, it was here before):
d)
Do you think we should have a comma after "optionally"? or maybe just leave this word out (since the sentence already has "may" in it, and the next sentence makes it even clearer)? or move it to say "Update hooks may optionally return..."?
Comment #7
mr.baileysThanks for the thorough review!
@link url title @endlink
format. I also uppercased (capitalized?) "Batch API" to be more consistent with "Schema API" and the link target document.Comment #8
jhodgdonLooking good, thanks!
On this read, I saw:
a)
Should this be "pending updates task list"? [with S on updates]
b) I think this section:
still needs to be made clearer that it is talking about how to compose the N in hook_update_N() (as it is, when I get to this line, I think it is telling me how to code a database updates, and then it turns out it is just talking about naming the function). I had a suggestion in comment #4 about how to rework this.
Comment #9
mr.baileysSorry, I seemed to have missed your suggestion about the number formatting in #4, fixed now.
mymodule_update_5200
andmymodule_update_6200
in the old examples were basically the same, with 6200 having more information, so I removed the 5200 exampleComment #10
jhodgdonThis looks great! There are just a couple of very minor things to fix:
a) I think we normally have a space before @endlink: http://drupal.org/node/1354#links
b) e.g. means "for example", and needs to be followed by a comma:
c) List items need to end in . by convention http://drupal.org/node/1354#lists
d) I think the separate paragraph here should be combined with the list item:
e) I'm not sure about the @see batchapi at the end. Were you trying to link to:
http://api.drupal.org/api/drupal/core!includes!form.inc/group/batch/8
? If so, it should be @see batch
Comment #11
mr.baileysThanks, all fixed
Comment #12
jhodgdonThanks, looks great! I'll get it committed shortly.
Comment #13
jhodgdonThe patch in #11 is committed to 8.x, thanks! I think we should port it to 7.x (change the 8.x references to 7.x), although I can get it to apply to 7.x with some fuzz.
Comment #14
mr.baileysBackported to D7. Straight re-roll except for the version numbers in the examples.
Comment #15
jhodgdonThere's an extra space in this line:
(before the "and")
but other than that it looks perfect.
Actually, that extra space is in the 8.x version too. Can we get a quick one-line patch to fix that in 8.x and a new patch for 7.x? Then I can get them both committed. Thanks!
Comment #16
mr.baileysSorry, re-rolled for D7 and a space removal patch for D8.
Comment #17
jhodgdonThanks! That looks good. The 7.x patch will fail tests when attached to an 8.x issue.... I'll get the 8.x one committed shortly, and then we can re-upload the 7.x one and re-test, just to make sure there isn't a syntax problem we didn't notice.
Comment #19
jhodgdonOK, that's odd. The user login test should not fail due to a patch that removes one space from a documentation line. Must be something wrong somewhere else... So that this test failure doesn't vanish into the ether, I'm going to re-upload the 8.x patch.
Comment #20
jhodgdonduh.
Comment #21
jhodgdonOK, I got the space removal patch committed to 8.x. Re-uploading the 7.x patch from #16, and assuming tests pass, I'll get that committed shortly.
Comment #22
jhodgdonThat's committed to 7.x now too.
The docblock text is not used in the same way in 6.x in the UI, so I guess I'm inclined to leave the 6.x documentation alone. If someone thinks we should fix that, we can file a separate issue (and it would be in the Documentation project, since the hook docs for D6 live in the Documentation project git repository; issue component would be "API documentation files").