Posted by joachim on June 6, 2012 at 2:42pm
4 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | documentation |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | needs backport to D7 |
Issue Summary
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.
Comments
#1
Hah, I thought we had put that in there... I guess not though. Good idea...
#2
The 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).
#3
For reference, the code that pulls the documentation block from the hook_update_N functions is in
update_get_update_list()#4
That'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]
---
#5
Makes 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).
#6
I agree about the term "patch"... a couple of grammatical/typographical/formatting things to fix:
a)
+ * For each change that requires on or more actions to be performed when+ * updating a site, add a new hook_update_N() which will be called by
+ * update.php.
- on -> one ["requires *one* or more actions].
- need comma before "which".
b)
+ * task list. Schema updates should adhere to the Schema API: @link http://drupal.org/node/150215 http://drupal.org/node/150215 @endlinkSince 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):
+ * API: @link http://drupal.org/node/180528 http://drupal.org/node/180528 @endlinkd)
+ * Optionally update hooks may return a translated string that will be+ * displayed to the user after the update has completed. If no message is
+ * returned, no message will be presented to the user.
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..."?
#7
Thanks for the thorough review!
@link url title @endlinkformat. I also uppercased (capitalized?) "Batch API" to be more consistent with "Schema API" and the link target document.#8
Looking good, thanks!
On this read, I saw:
a)
+ * newlines and used as the description for the update on the pending update+ * task list. Schema updates should adhere to the
Should this be "pending updates task list"? [with S on updates]
b) I think this section:
* Database updates consist of 3 parts:* - 1 digit for Drupal core compatibility
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.
#9
Sorry, I seemed to have missed your suggestion about the number formatting in #4, fixed now.
mymodule_update_5200andmymodule_update_6200in the old examples were basically the same, with 6200 having more information, so I removed the 5200 example#10
This 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
+ * @link http://drupal.org/node/150215 Schema API.@endlink+ * @link http://drupal.org/node/180528 Batch API.@endlink
b) e.g. means "for example", and needs to be followed by a comma:
+ * - 1 digit for your module's major release version (e.g. is this the 8.x-1.*+ * (1) or 8.x-2.* (2) series of your module?)
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:
+ * - 2 digits for sequential counting, starting with 00*
* The 2nd digit should be 0 for initial porting of your module to a new Drupal
* core API.
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
#11
Thanks, all fixed
#12
Thanks, looks great! I'll get it committed shortly.
#13
The 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.
#14
Backported to D7. Straight re-roll except for the version numbers in the examples.
#15
There's an extra space in this line:
+ * and they get all 70xx and 72xx updates, but not 71xx updates, because(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!
#16
Sorry, re-rolled for D7 and a space removal patch for D8.
#17
Thanks! 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.
#18
The last submitted patch, 1619482-16-hook_update_N-docs-D8.patch, failed testing.
#19
OK, 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.
#20
duh.
#21
OK, 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.
#22
That'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").
#23
Automatically closed -- issue fixed for 2 weeks with no activity.