Clean up API docs for book

Thanks! It looks pretty good, but not quite perfect... [Note: I didn't verify the accuracy of the @param and @return sections you added -- was just looking at formatting, style, etc.]

a)

 /**
  * @file
- * Default theme implementation for rendering book outlines within a block.
- * This template is used only when the block is configured to "show block on
- * all pages" which presents Multiple independent books on all pages.
+ * Default theme implementation for rendering book outlines within a block. This

All docblocks including @file ones need to start with a one line description.
http://drupal.org/node/1354#files

b) We just decided on a different standard for these things (see #1337282: Lacking doc header standard for forms used as page callbacks):

 /**
- * Builds and returns the book settings form.
+ * Page callback: Form constructor for the book settings form.

Technically, the page callback here is drupal_get_form(), so we don't put "page callback" in the one-line description for the form constructor. Leave the Path: line in and the reference to hook_menu() though.

c)
This is actually a form submission handler:

/**
- * Handle submission of the book administrative page form.
+ * Form validation handler for book_admin_edit().
  *
  * This function takes care to save parent menu items before their children.
  * Saving menu items in the incorrect order can break the menu tree.
  *
- * @see book_admin_edit()
+ * @see book_admin_edit_validate()
  * @see menu_overview_form_submit()
  */
 function book_admin_edit_submit($form, &$form_state) {

d)

+ * @return
+ *   A parent selection form element.
+ *
  */

Extra blank line here at the end.

e)

+ * Updates the bid for a page and its children when it is moved to a new book.

+ * Gets a book menu link by its mlid.

Avoid mlid and bid and nid and stuff like that in text:
bid -> book ID
mlid -> menu link ID

f) I realize you didn't write this originally, but what are the // here for?

+ * The given node is /embedded to its absolute depth in a top level section/.

Probably they should be removed?

g) Page callbacks don't need @return:

 /**
- * Menu callback; show the outline form for a single node.
+ * Page callback: Shows the outline form for a single node.
+ *
+ * Path: node/%node/outline
+ *
+ * @param $node
+ *   The book node for which to show the outline.
+ *
+ * @return
+ *  The rendered book outline form.
+ *
+ * @see book_menu()
  */

Re: #4. http://drupal.org/node/1354#forms has been updated for this. The conclusion we came to was just using the pattern you are in #3; that is: add the path and the @see to the hook_menu() implementation, but not the Page callback: part since the form constructors themselves are technically not the page callbacks. So yeah, we should use that pattern in all of them.

Setting NR for testbot.

This is looking pretty good! I did find a couple of things that I think should be fixed:

a)

+ * Access callback: Determines if the user can remove nodes from the outline.
+ *
+ * Path: node/%node/outline/remove
+ *
+ * @param $node
+ * The node to delete.
+ *
+ * @see book_menu()
*/
function _book_outline_remove_access($node) {

Technically, $node is not being deleted, just removed from the outline, right?

Similarly, in this case $node could be being added, deleted, etc., not just saved:

+ * This common helper function performs all additions and updates to the book
+ * outline through node addition, node editing, node deletion, or the outline
+ * tab.
  *
- * Performs all additions and updates to the book outline through node addition,
- * node editing, node deletion, or the outline tab.
+ * @param $node
+ *   The node that is being saved.

b)

 /**
- * Return an array with default values for a book link.
+ * Returns an array with default values for a book link.
+ *
+ * @param $nid
+ *   The node ID of the node whose menu link is being created.
+ *
+ * @return
+ *   The default values for a book link.
  */
 function _book_link_defaults($nid) {

This threw me a bit, because it calls what it's working on a "book link" twice and a "menu link" once -- maybe standardize on one or the other? Also, you could say "The ID of the node" rather than "the node ID of the node".

c) Generally in text, it is preferable to use "ID" rather than things like "mlid", "nid" etc. Such as here:

 /**
+ * Gets a book menu link by its mlid.
+ *
  * Like menu_link_load(), but adds additional data from the {book} table.
  *
  * Do not call when loading a node, since this function may call node_load().
+ *
+ * @param $mlid
+ *   The mlid of the menu item.

mlid -> menu link ID

Also here (id -> ID):
An integer representing the node id (nid) of the node to export.

d) This could use a few more words (filled -> filled in; and link translated => and the link translated), and what is $item?
+ * A menu link, with $item['access'] filled and link translated for rendering.

e)

+ *   A string encoding the type of output requested. The following types are
+ *   currently supported in book module:
  *   - html: HTML (printer friendly output)
- *
  *   Other types may be supported in contributed modules.

Umm... First off, list items should end in ".", and "printer friendly" should be printer-friendly. But... is 'html' actually giving you printer-friendly output -- that seems wrong, as normally HTML is browser-friendly?

I guess that might be a separate issue and not part of the cleanup but it jumped out at me.

I still don't get why HTML output should be labeled "printer-friendly"?

EDIT/added: I mean, if it's just producing ordinary HTML output, just say that?

Well, printer-friendly HTML is usually stripped of images, layout, etc. and styled with CSS for print media rather than browsers. That's what the print module does. I didn't even know this feature was in core, though... I thought the "printer-friendly version" links were from the print module.

See book_export_html().

Ah. OK then, that makes sense. Let's just call it "printer-friendly HTML" then, not "HTML (printer friendly output)".

RE #10 - From book_menu:

 $items['book/export/%/%'] = array(
    'page callback' => 'book_export', 
    'page arguments' => array(2, 3), 
    'access arguments' => array('access printer-friendly version'), 
    'type' => MENU_CALLBACK, 
    'file' => 'book.pages.inc',
  );

That is the URL that those printer-friendly links at the bottom of book pages are going to. Live and learn!

"Printer-friendly HTML" looks good to me. I read the patch (didn't apply) and found a few more things:

1. +++ b/core/modules/book/book.admin.incundefined
   @@ -169,7 +186,15 @@ function _book_admin_table($node, &$form) {
   - * Recursive helper to build the main table in the book administration page form.
   + * Recursively helps build the main table in the book administration page form.
   

   This is a little odd; as written, the summary says that we're helping recursively, not building recursively. In any case, how about simplifying it to:

   Builds the main table in the book administration form.

2. +++ b/core/modules/book/book.moduleundefined
   @@ -84,7 +84,12 @@ function book_permission() {
   - * Inject links into $node as needed.
   + * Injects links into a book page as needed.
   

   This is also a little confusing. How about:

   Adds relevant book links to the node's links.

3. +++ b/core/modules/book/book.moduleundefined
   @@ -940,14 +1015,22 @@ function book_node_prepare($node) {
   - * Form altering function for the confirm form for a single node deletion.
   + * Implements hook_form_FORM_ID_alter().
   + *
   + * Alters the confirm form for a single node deletion.
     */
    function book_form_node_delete_confirm_alter(&$form, $form_state) {
   

   Should we add an @see here to node_delete_confirm()?

4. +++ b/core/modules/book/book.moduleundefined
   @@ -1092,10 +1181,12 @@ function _book_toc_recurse($tree, $indent, &$toc, $exclude, $depth_limit) {
   + *   An array of menu link ID, title pairs for use as options for selecting a
   + *   book page.
   

   Maybe "An array of menu link ID and title pairs"?

5. +++ b/core/modules/book/book.moduleundefined
   @@ -1210,7 +1302,7 @@ function book_type_is_allowed($type) {
   + * Updates book module's persistent variables if the machine-readable name of a
     * node type is changed.
   

   I believe this should begin, "Updates the Book module's persistent variables..."

6. +++ b/core/modules/book/book.pages.incundefined
   @@ -55,16 +62,16 @@ function book_export($type, $nid) {
   + * (otherwise empty) <div> elements corresponding to depth 0 and depth 1.
   

   This should just be <div> elements (no escaping). (I noticed this when I looked up this function on api.d.o earlier.)

Thanks!

2 days to next Drupal core point release.

RE #14/15 item 4 - I think the suggestion in #15 makes for the clearest documentation, even if it isn't plain English.

Looks good! Let's get this in. :)

Looks good to me too. Committed/pushed to 8.x. Thanks!

Thanks for the port, and sorry for the delay in reviewing!

Anyway, this patch looks mostly good. The only thing that we need to cut out is these lines:

+ *
+ * @see book_menu()

At least on the other D7 ports, these lines pointing back to the hook_menu() are "part of a new standard" (for menu callbacks) and are not supposed to be backported. I'm not sure if I understand the logic, but that's what we've been doing. So if you can take those out, we can OK this patch. Thanks!

Oh, one more thing. These types of lines:

  * Menu callback; prints a listing of all books.

Should change ; to :

* Menu callback; prints a listing of all books.

...And additionally capitalize "Prints." :) (The reasoning for allowing that backport is that the summaries are already more or less in line with the "new" standard, so we can reformat them a little and get away with backporting them.)

Looks like little work is remaining but stuck since ages, so let me just pick it up :-)
re-rolled patch according to #26, #27

Uh oh. I reviewed the D7 patch in #28 and found a few things, but they need to be fixed in the Drupal 8 version actually (and then put into the D7 patch too):

a) in book.pages.inc, the first line description of book_export_html() needs to start with a verb:

/**
 * This function is called by book_export() to generate HTML for export.

b) in book.module, this isn't quite right:

/**
 * Implements hook_form_BASE_FORM_ID_alter().
 *
 * Adds the book fieldset to the node form.
 *
 * @see book_pick_book_nojs_submit()
 */
function book_form_node_form_alter(&$form, &$form_state, $form_id) {

Should say Implements hook... for node_form() on the first line. Just a bit farther down, there's another one:

 /**
 * Implements hook_form_FORM_ID_alter().
 *
 * Alters the confirm form for a single node deletion.
 *
 * @see node_delete_confirm()
 */
function book_form_node_delete_confirm_alter(&$form, $form_state) {

--- c) And then in the D7 patch, there is one other thing that will need to be fixed:

@@ -181,10 +189,12 @@ function book_outline_form_submit($form, &$form_state) {
 }
 
 /**
- * Menu callback; builds a form to confirm removal of a node from the book.
+ * Menu callback; Form constructor to confirm removal of a node from a book.
  *
- * @see book_remove_form_submit()
+ * @param $node
+ *   The node to delete.
  *
+ * @see book_remove_form_submit()
  * @ingroup forms
  */
 function book_remove_form($form, &$form_state, $node) {

Take out "Menu callback;" from the first line. [that is already correct in D8]

So: first a D8 patch to fix those two items, then once that is committed, we also need a D7 patch to fix the three items there.

Attached patch fix point 'a' & 'b' for D8

Thanks, but not quite right...

On (b)... Currently, it says:
Implements hook_form_BASE_FORM_ID_alter().
It needs to follow
http://drupal.org/node/1354#hookimpl (second example).

And for (a) it needs to start with "Generates" not "Generate".

Updated, thanks.

oops

#32: book_clean-up-doc-D8-1327484-31.patch queued for re-testing.

Thanks! Patch in #32 committed to Drupal 8.

Moving back to D7, where the patch in #28 needs these fixes, plus (c) from #29.

Here it the updated patch according to #29.

oops, wrong name though :-|

Looks good! Committed to 7.x. :)

In one go :-) cool, thanks.

For reference, I opened up #1807688: Further clean up of API docs for Book module for further documentation revisions to the Book module.