Improve documentation of menu test module

#1: fixed-menu-test-comments-1924108-1.patch queued for re-testing.

Looks like good clean-up to me, @benjy. While we're here why not touch this up?

+++ b/core/modules/system/tests/modules/menu_test/menu_test.moduleundefined
@@ -582,9 +582,6 @@ function menu_test_theme_callback($argument) {
 /**
  * Implement hook_custom_theme().
- *
- * @return
- *   The name of the custom theme to use for the current page.
  */

s/Implement/Implements/

Just a Dreditor bug: #1754036: "undefined" suffix.

I'd say this is ready to go.

BTW @benjy, it's good practice to link to follow-up issues you create from the original issue. You'll get more issue followers (and reviews) that way too :)

Putting this on jhogdon's list

In the case of a test module (a module meant to be used in tests), there is something to be said for having the documentation for a hook implementation actually explain what the particular implementation does, and how it can be used in a test, rather than just using the standard "Implements hook_foo()" comment.

Any thoughts on that? I think it might be useful to add a line to each function and explain what the test module is doing...

That would be great, thanks!

Given the plan in #15...

Um, I don't see any evidence that this hook has been committed?

As far as the commenting in the patch being pretty good, that is an OK point that the comments are there in the code and may not need to be in the doc block. I personally think it's a bit better if they're in the doc block, but since it's a test module I guess it is OK... Generally the philosophy of doc blocks is that you should not have to read the code (or the comments in the code) to find out what the function does.

The patch is fine... I'm looking through the rest of this test module and wondering if it could use additional/better documentation though throughout?

For instance:

a)

/**
 * @file
 * Dummy module implementing hook menu.
 */

First off, it's not really a "dummy" module. It's a module for testing the menu router system. Second, "hook menu" should be "hook_menu()". Third, it tests a lot of other stuff besides hook_menu(). Can we do better? Or maybe take the description out of the .info.yml file (which is at least more accurate)?

b)

/**
 * Implements hook_menu_local_tasks().
 */
function menu_test_menu_local_tasks(&$data, $router_item, $root_path) {
  if (!config('menu_test.settings')->get('tasks.add')) {
    return;
  }

How about adding a line here that describes what this hook implementation does -- something like "If the menu_test.settings configuration 'tasks.add' has been set, adds several local tasks to menu-test/tasks." ? That would help people know what the test module does and how to use it in a test.

c)

/**
 * Dummy callback for hook_menu() to point to.
 *
 * @return
 *  A random string.
 */
function menu_test_callback() {
  return 'This is menu_test_callback().';
}

The @return is not accurate here -- it shouldn't say "random", since it isn't random. How about just saying it returns some text that can be tested for? Also, the first line doesn't start with a verb.

d)

/**
 * Callback that test menu_test_menu_tree_set_path().
 */
function menu_test_menu_trail_callback() {
 

Bad grammar (test -> tests).

etc. There are many more similar to b-d in this file.

Thanks! That's a great start!

A few things to fix up:

a)

-  // @see BreadcrumbTest
+  // @see Drupal\system\Tests\Menu\BreadcrumbTest

If you use namespaces in documentation, they need to start with \ -- this occurs in several places in your patch.

b)

   );
-
+  // Test the access key with a function callback.
   $items['menu_login_callback'] = array(

I wouldn't remove the blank lines here (and in other similar places).

c) There are spaces at the ends of several lines. Configure your text editor to highlight or remove end of line spaces.

d)

 /**
- * Callback that test menu_test_menu_tree_set_path().
+ * Callback that tests menu_test_menu_tree_set_path().
  */
 function menu_test_menu_trail_callback() {

Function documentation first lines should start with verbs.
http://drupal.org/node/1354#functions

We discussed this in IRC. Since the standard for menu callbacks will probably need to be revised for Drupal 8, we probably should not worry about the exact format of the menu callback doc blocks in this module... but we probably won't change the first line standards much. So let's have each of them have a first line like:

Page callback: Displays a list of content.

and documentation of any parameters, as well as the return value (see https://drupal.org/node/1354#menu-callback).

It's looking pretty close, thanks for the new patch! Yes, @param should match the actual parameters, and in this case the name in @param is better than the one in the function signature, so it is good to change the function rather than the documentation block.

So, I think this still needs a very small amount of work and then we can call it good:

a) menu_test_menu_trail_callback() :

+ * Retrieve the current menu path and if the menu path is not empty updates
+ * the menu path that is used to determine the active menu trail.

The verbs in this sentence do not agree with each other: Retrieve... updates. Should be Retrieves/updates or Retrieve/update (I don't care which).

b) menu_test_theme_page_callback():

  * @param $inherited
  *   An optional boolean to set to TRUE when the requested page is intended to
  *   inherit the theme of its parent.
- * @return
+ * @return string

There should be a blank line before the @return. (this applies elsewhere in the patch too)

Also, if you are adding types to the @return, how about to the @param too (bool) -- in which case we could make it fully compliant with our standards (and more concise):
@param book $inherited
(optional) TRUE if the requested page should inherit the theme of its parent.

This looks really good!

One thing: When a param/return can be NULL, we use "null" not "NULL" as the data type:
https://drupal.org/node/1354#types

One other thing: The very first line of the patch...

 /**
  * @file
- * Dummy module implementing hook menu.
+ * Test module providing tests for several menu related hooks.
  */
 

Technically, this module doesn't test anything. It sets up things for the test classes to test. So maybe we should say: "Module that implements various hooks for menu tests." or something like that?

Looks good! I'll get this patch committed.

In a follow-up (or if you want to fix it now that would be fine too), we could/should go back and fix up the rest of the function documentation first-line descriptions so that they follow our standards (one line, starting with a verb in 3rd person, with possible "Foo callback:" prefixes, ending in "."). There are still a few that don't, after that patch:
menu_test_argument_load()
menu_test_other_argument_load()
menu_test_menu_name()
menu_test_static_variable()

I also don't think the 2nd paragraph is needed here (redundant):

 /**
+ * Title callback: Concatenates the title and case number.
+ *
  * Concatenates a string, by using the t() function and a case number.
...
+ * @return string
+ *   A string containing the title and case number.
+ *
+ * @see menu_test_menu().
  */
 function menu_test_title_callback($title, $case_no = 3) {

So... actually, maybe it would be good to fix these before committing the patch?

Excellent, we're almost there! Just a couple of very minor things to fix:

a) In @param/@return data types, we say "false" and "null" (this is to keep all of them lower-case, as they are on php.net and elsewhere), but in text, we use FALSE/NULL. Sorry for the confusion. So:

+ * @return false
+ *   Always return false.

should be

+ * @return false
+ *   Always returns FALSE.

b)

 /**
- * Helper function for the testMenuName() test. Used to change the menu_name
- * parameter of a menu.
+ * Sets a static variable for the testMenuName() test.
+ * Used to change the menu_name parameter of a menu.

Needs a blank line between the first two lines.

An interdiff would have been nice. :)

No worries though -- this looks good. Thanks! I'll get it committed shortly.

The patch unfortunately does not apply today. :(

Regarding interdiffs, if the patch is more than a few lines long, and you make changes, it is nice to include an interdiff for reviewers. For a reroll, this is not really possible/necessary.

Thanks! Looks good still/again. I'll get it committed soon.

Thanks again! Committed to 8.x.