Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Hah, I thought we had put that in there... I guess not though. Good idea...

mr.baileys’s picture

Status: Active » Needs review
FileSize
1.23 KB

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).

mr.baileys’s picture

For reference, the code that pulls the documentation block from the hook_update_N functions is in update_get_update_list()

jhodgdon’s picture

Status: Needs review » Needs work

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:

For each patch which requires a database change add a new hook_update_N() which will be called by update.php. The database updates are numbered sequentially according to the version of Drupal you are compatible with.

Schema updates should adhere to the Schema API: http://drupal.org/node/150215

Database updates consist of 3 parts:
[description of the numbering scheme]

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]
---

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
2.31 KB

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).

jhodgdon’s picture

Status: Needs review » Needs work

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 @endlink

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):

+ * API: @link http://drupal.org/node/180528 http://drupal.org/node/180528 @endlink

d)

+ *   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..."?

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
2.37 KB

Thanks for the thorough review!

  • Fixed the grammatical/typographical/formatting in the opening paragraph.
  • Changed the @link's to @link url title @endlink format. I also uppercased (capitalized?) "Batch API" to be more consistent with "Schema API" and the link target document.
  • Added a comma after "optionally" in the @return section (my preference is to have optionally be the first word.)
  • Added some @see's at the end.
jhodgdon’s picture

Status: Needs review » Needs work

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.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
4.28 KB

Sorry, I seemed to have missed your suggestion about the number formatting in #4, fixed now.

  1. Changed the formatting a bit, since it doesn't make sense to have single-item lists.
  2. I have upgraded the core versions in the numbering examples to 8.x (from 5.x/6.x)
  3. mymodule_update_5200 and mymodule_update_6200 in the old examples were basically the same, with 6200 having more information, so I removed the 5200 example
jhodgdon’s picture

Status: Needs review » Needs work

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

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
4.41 KB

Thanks, all fixed

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, looks great! I'll get it committed shortly.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

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.

mr.baileys’s picture

Status: Patch (to be ported) » Needs review
FileSize
4.42 KB

Backported to D7. Straight re-roll except for the version numbers in the examples.

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

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!

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
721 bytes
4.41 KB

Sorry, re-rolled for D7 and a space removal patch for D8.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1619482-16-hook_update_N-docs-D8.patch, failed testing.

jhodgdon’s picture

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.

jhodgdon’s picture

Status: Needs work » Needs review

duh.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Reviewed & tested by the community
FileSize
4.41 KB

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.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

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").

Automatically closed -- issue fixed for 2 weeks with no activity.