Clean up API docs for menu module

Do you plan to patch this module? If so can you assign the issue to yourself and also add your name to the summary issue? Thanks!

Actually, I don't think this issue is in the list on the summary issue.

I'll take this.

This looks pretty good! Here are a few things I would suggest changing:

a)

 * @param $tree
- *   The menu_tree retrieved by menu_tree_data.
+ *   The menu_tree retrieved by menu_tree_data().
</code
menu_tree -> menu tree array (or maybe array of menu items?)

b)
<code>
+ * @param $type
+ *   The type of operation the form should allow, 'add' or 'edit'.

How about "The type of operation being done" or something like that? "should allow" doesn't seem right.
(Same for the form constructor below -- the one for the menu as a whole as opposed to an item)

c)

+ *   When $type is 'add', an array representing the menu a link will be added
+ *   to; when $type is 'edit', NULL.

a link -> the link [this is a definite link]

d)

+ * Form submission handler for menu_delete_menu_confirm()

No "." at end.

e)

 /**
- * Informs modules that a custom menu was created.
+ * Inform modules that a custom menu was created.
  *
  * This hook is used to notify modules that a custom menu has been created.

This is from menu.api.php... Normally, we document this type of hook (this applies to all three menu hooks) something like this:

Respond to...

e.g. Respond to custom menu creation.

The reason being that we're documenting what the implementing module is doing, not what the hook invoker is doing.

f)

/**
  * Find the depth limit for items in the parent select.
+ *

Find -> Finds

Thanks for your help.

This looks really good! The only things I saw to fix:

a)

 /**
- * Informs modules that a custom menu was deleted.
+ * Rspond to custom menu was deletion.

Typo -- Respond is misspelled... and the sentence doesn't make sense (omit "was").

b)

+ * @param $indent
+ *   Text to prefix indented menu items with.
+ * @param $options
+ *    An array of menu items to be used as options in a select form element.
+ *    This array is updated by reference.

Indentation has extra space in that second @param doc area.

I don't know how you noticed the indentation problem, but nice catch.

@jhodgdon can spot an error in patch whitespace at 20 paces. :)

This is looking good. Read through the patch and applied it to see if anything else is amiss.

Only remaining issue: The .js and .css files don't have @file doc blocks.

I didn't think css and js files were supposed to have @file blocks; I thought they were just for php. The @file block is what adds the file to http://api.drupal.org/api/drupal/files, isn't it? It doesn't seem to be the convention to include js and css. But maybe this just shows that everyone has been doing it wrong?

Well, they've required adding them to CSS files in the CSS cleanup sprint for the HTML5 initiative, and so we started adding those that were missing as well for the docs sprint. We also confirmed that the docblocks are stripped during CSS and JS aggregation, so they do not add any overhead.

Here's the patch with the changes.

Rerolled your awesome patch.

The re-roll in #14 is from an old version (does not include @file headers, has the "Rspond" pointed out in #6 ...).

Mhh ... looks like #12 had them, too.

True, #12 has "Rspond" (so maybe the reroll there was against a previous version?), but it does have the @file headers for the css files.

Not sure how I messed up patch #12 and left out those fixes, but here's another try.

Looks very good and consistent in itself. One really, really, really minor point:

+++ b/core/modules/menu/menu.admin.incundefined
@@ -41,10 +45,19 @@ function theme_menu_admin_overview($variables) {
+ * Path: admin/structure/menu/manage/%menu

Here the path is above the summary.

+++ b/core/modules/menu/menu.moduleundefined
@@ -382,8 +392,13 @@ function menu_parent_options($menus, $item, $type = '') {
+ * Path: admin/structure/menu/parents

The path is below the summary here. Decide for one way?

Niklas means the first paragraph after the one-line summary. I think general practice is to have the Path:-line as the first line after the one-line summary (and that is also how it is in the example in the doxygen standards).

@sven.lauer: Exactly. Thanks for clarifying. Fixed the typo.

Ok, I have fixed the minor point. Thanks for the review.

Awesome, thanks.

+++ b/core/modules/menu/menu.admin.inc
@@ -6,7 +6,11 @@
+ * Path: admin/structure/menu

@@ -41,10 +45,19 @@ function theme_menu_admin_overview($variables) {
+ * Path: admin/structure/menu/manage/%menu

@@ -253,7 +267,26 @@ function theme_menu_overview_form($variables) {
+ * Path:
+ * - admin/structure/menu/manage/%menu/add
+ * - admin/structure/menu/item/%menu_link/edit

@@ -417,7 +454,22 @@ function menu_edit_item_submit($form, &$form_state) {
+ * Path:
+ * - admin/structure/menu/add
+ * - admin/structure/menu/manage/%menu/edit

@@ -483,14 +535,21 @@ function menu_edit_menu($form, &$form_state, $type, $menu = array()) {
+ * Path: admin/structure/menu/manage/%menu/delete

@@ -600,7 +675,14 @@ function menu_edit_menu_submit($form, &$form_state) {
+ * Path: admin/structure/menu/item/%menu_link/delete

@@ -633,7 +721,16 @@ function menu_item_delete_form_submit($form, &$form_state) {
+ * Path: admin/structure/menu/item/%menu_link/reset

@@ -651,7 +748,12 @@ function menu_reset_item_confirm_submit($form, &$form_state) {
+ * Path: admin/structure/menu/settings

I'm very uncomfortable with documenting menu router paths in phpDoc of functions. I'd almost say that's invalid to do. Where was this decided?

+++ b/core/modules/menu/menu.admin.inc
@@ -6,7 +6,11 @@
+ * @see menu_menu()

@@ -41,10 +45,19 @@ function theme_menu_admin_overview($variables) {
+ * @see menu_menu()

@@ -253,7 +267,26 @@ function theme_menu_overview_form($variables) {
+ * @see menu_menu()

@@ -417,7 +454,22 @@ function menu_edit_item_submit($form, &$form_state) {
+ * @see menu_menu()

Likewise, stuffing a @see to a hook_menu() implementation into every page/access/title/whatever function callback is very debatable, too. The function callback and the hook_menu() implementation are only marginally related. Again, where was this discussed?

I see you've found #1315992: No standard for documenting menu callbacks already. I'll reply there.

The standard has been updated:
http://drupal.org/node/1354#menu-callback

So this patch will need an update.

tagging

Working on this at the DrupalCon core office hours sprint...

After a brief delay due to my flight home from Denver, here's a reroll of @xenophyle's patch in #22, based on my understanding of the new standard in http://drupal.org/node/1354#menu-callback and @sun's comments in #24. My very first core patch--keeping fingers crossed that I did everything right!

#30: menu-clean-up-documentation-1326672-30.patch queued for re-testing.

Nice job @johnshortess! Thanks for taking this up.

I did a full textual review of the patch (did not apply it) and found the following things to fix (most of which are oddities related to SimpleTest docs):

1. +++ b/core/modules/menu/menu.moduleundefined
   @@ -295,20 +302,22 @@ function menu_save($menu) {
     *
   - * menu_delete_links() will take care of clearing the page cache. Other modules
   - * should take care of their menu-related data by implementing
   - * hook_menu_delete().
   +
   

   These blank lines should be removed (on either side of the removed hunk).

2. +++ b/core/modules/menu/menu.moduleundefined
   @@ -604,14 +656,20 @@ function menu_node_prepare($node) {
   -  // Find the depth limit for the parent select.
   +  // Finds the depth limit for the parent select.
   

   This change does not need to be made (as it's an inline comment rather than a docblock), so I would remove it.

3. +++ b/core/modules/menu/menu.testundefined
   @@ -5,12 +5,29 @@
   +/**
   + * Represents a test case for menu administration.
   + */
   

   For this, let's say "Tests menu administration" or "Provides tests for menu administration." These classes do not have constructors, so "represents" doesn't fit, I don't think.

4. +++ b/core/modules/menu/menu.testundefined
   @@ -5,12 +5,29 @@
      protected $profile = 'standard';
   -
   +  /**
   +   * A user with various administrative privileges.
   +   */
      protected $big_user;
   +  /**
   +   * A user with no administrative privileges.
   +   */
      protected $std_user;
   +  /**
   +   * An array representing a menu.
   +   */
      protected $menu;
   +  /**
   +   * An array of menu items.
   +   */
      protected $items;
   

   For these, let's make sure to add a blank line after each variable before the next docblock.

5. +++ b/core/modules/menu/menu.testundefined
   @@ -21,6 +38,9 @@ class MenuTestCase extends DrupalWebTestCase {
   +  /**
   +   * Implements DrupalWebTestCase::setUp().
   +   */
   
   @@ -581,11 +634,13 @@ class MenuTestCase extends DrupalWebTestCase {
   +  /**
   +   * Returns information about the menu node tests.
   +   */
   
   @@ -594,6 +649,9 @@ class MenuNodeTestCase extends DrupalWebTestCase {
   +  /**
   +   * Implements DrupalWebTestCase::setUp().
   +   */
   

   At present, we exclude docblocks for setUp() and getInfo() (and there isn't actually a defined interface for them) so these hunks can be removed. References: http://drupal.org/node/325974, #338403: Use {@inheritdoc} on all class methods (including tests)

6. +++ b/core/modules/menu/menu.testundefined
   @@ -29,7 +49,8 @@ class MenuTestCase extends DrupalWebTestCase {
   +   * Logs in users, adds menus and menu links, and tests menu functionality
   +   * through the admin and user interfaces.
   
   @@ -492,13 +540,13 @@ class MenuTestCase extends DrupalWebTestCase {
   -   * Fetch the menu item from the database and compare it to the specified
   +   * Fetches the menu item from the database and compares it to the specified
       * array.
   
   @@ -530,9 +581,11 @@ class MenuTestCase extends DrupalWebTestCase {
   +   * Verifies that the logged in user has the desired access to the various menu
   +   * nodes.
   

   Let's try to reword these so they fit on one line that is 80 chars or less.

7. +++ b/core/modules/menu/menu.testundefined
   @@ -278,12 +305,17 @@ class MenuTestCase extends DrupalWebTestCase {
   +   * Adds a menu link using the menu module UI.
   
   @@ -331,12 +364,16 @@ class MenuTestCase extends DrupalWebTestCase {
   +   * Verifies a menu link using the menu module UI.
   
   @@ -366,7 +403,14 @@ class MenuTestCase extends DrupalWebTestCase {
   +   * Changes the parent of a menu link using the menu module UI.
   
   @@ -379,9 +423,10 @@ class MenuTestCase extends DrupalWebTestCase {
   +   * Modifies a menu link using the menu module UI.
   
   @@ -423,9 +470,10 @@ class MenuTestCase extends DrupalWebTestCase {
   +   * Deletes a menu link using the menu module UI.
   

   Should read:
   ...using the Menu module UI.
   Reference: http://drupal.org/node/604342

8. +++ b/core/modules/menu/menu.testundefined
   @@ -402,10 +447,12 @@ class MenuTestCase extends DrupalWebTestCase {
   +   *   Original title for menu link.
   

   I'd add an article here ("the menu link").

9. +++ b/core/modules/menu/menu.testundefined
   @@ -492,13 +540,13 @@ class MenuTestCase extends DrupalWebTestCase {
   +   *   The menu item id.
   

   "ID" should be capitalized.

10. +++ b/core/modules/menu/menu.testundefined
    @@ -581,11 +634,13 @@ class MenuTestCase extends DrupalWebTestCase {
    - * Test menu settings for nodes.
    + * Represents a test case for menu settings for nodes.
    

    For this one, I think "Tests menu settings for nodes" would be the best summary.

11. +++ b/core/modules/menu/menu.testundefined
    @@ -609,7 +667,7 @@ class MenuNodeTestCase extends DrupalWebTestCase {
    +   * Tests creating, editing, deleting menu links via node form widget.
    

    I'd say "...creating, editing, and deleting menu links..."

Finally, one other thing to add might be @return for some of the page callbacks (I think some but not all were missing them). In #1315992: No standard for documenting menu callbacks we decided that we should document the return values since a page callback might return either a render array or a string.

For @johnshortess or whoever rolls, the next patch, it would be good to include an interdiff along with the full patch, so that we can easily review the changes made. Thanks!

Thanks, xjm. Here's a new patch, and an interdiff.

Okay, let's try this again. New patch, less whitespace.

Thanks @johnshortess! The changes in the interdiff look good to me. I did notice a few more things when I reviewed it this time:

1. +++ b/core/modules/menu/menu.test
   @@ -667,7 +658,7 @@ class MenuNodeTestCase extends DrupalWebTestCase {
   -   * Tests creating, editing, deleting menu links via node form widget.
   

   I think this should be "via the node form widget"?

2. +++ b/core/modules/menu/menu.module
   @@ -295,20 +302,20 @@ function menu_save($menu) {
   + * Modules should always pass a fully populated $menu when deleting a custom
   + * menu, so other modules are able to output proper status or watchdog messages.
   

   For clarity, I'd reword this a little (remove the comma and say "so that other modules are..." rather than "so other modules are...").

3. +++ b/core/modules/menu/menu.module
   @@ -398,8 +409,20 @@ function menu_parent_options_js() {
   +
    function _menu_get_options($menus, $available_menus, $item) {
   

   I think the extra blank line should not be added here.

4. +++ b/core/modules/menu/menu.test
   @@ -194,8 +217,10 @@ class MenuTestCase extends DrupalWebTestCase {
   +   * @param $menu_name
   +   *   The menu name.
   
   @@ -278,12 +303,17 @@ class MenuTestCase extends DrupalWebTestCase {
   +   * @param integer $plid
   +   *   The parent menu link ID.
   +   * @param string $link
   +   *   The link path.
   +   * @param string $menu_name
   +   *   The menu name.
   
   @@ -315,9 +345,10 @@ class MenuTestCase extends DrupalWebTestCase {
   +   * @param string $menu_name
   +   *   The menu name.
   
   @@ -331,12 +362,16 @@ class MenuTestCase extends DrupalWebTestCase {
   +   * @param array $parent
   +   *   The parent menu link.
   +   * @param object $parent_node
   +   *   The parent menu link content node.
   

   It looks like these parameters are optional, so the descriptions should begin with (optional) and then mention the default value at the end. Reference: http://drupal.org/node/1354#param-return-data-type

5. +++ b/core/modules/menu/menu.test
   @@ -667,7 +658,7 @@ class MenuNodeTestCase extends DrupalWebTestCase {
   -   * Tests creating, editing, deleting menu links via node form widget.
   

   I think this should be "via the node form widget"?

As before, an interdiff would be helpful with the updated patch. Thanks!

+++ b/core/modules/menu/menu.testundefined
@@ -331,12 +362,16 @@ class MenuTestCase extends DrupalWebTestCase {
+   * @param array $parent
+   *   The parent menu link.
+   * @param object $parent_node
+   *   The parent menu link content node.

I noticed something else when debugging #1468420: Submenus of a parent menu item that links to the front page (<front>) do not show when viewing the site's front page.. These two arguments default to NULL, so as I mentioned before they should be documented as optional and have the default value specified.
In this particular case, though, it appears that (while these second two arguments are optional), if you specify one, then you must specify both. So, let's add that information to the docblock.

I'm going through these issues and un-assigning any without activity for several weeks. If you still want to work on this issue, please feel free to assign it back to yourself. Thanks!

Here's a reroll of the one above. Let's see if this passes, then I can add the comments.

#39: menu-cleanup-doc-1326672-38.patch queued for re-testing.

DAMMIT. I'll re-reroll when I get to work tomorrow. Trying to do a few of these in between projects this week.

patch from #35 rerolled.

About the comments in #36, it looks like the link about @param defaults has it on the same line, rather than at the end. Can you clairify which is correct and I"ll add that to the patch with an interdiff.

I've seen things like this

  /**
   * Add a menu link using the menu module UI.
   *
   * @param integer $plid
   *   (optional) Parent menu link id. Default 0
   * @param string $link
   *   (optional) Link path. Default '<front>'
   * @param string $menu_name
   *   (optional)  Menu name. Default 'navigation'
   * @param string $expanded
   *   (optional)  Whether menu is expanded. Default true
   * @param string $weight
   *   (optional)  Menu weight
   * @return array
   *   (optional) Menu link created.
   */
  function addMenuLink($plid = 0, $link = '<front>', $menu_name = 'navigation', $expanded = TRUE, $weight = '0') {
    

The formatting in #46 looks good, aside from some places where there are two spaces after (optional) and some descriptions that don't have a "." at the end. Also, we usually say "Defaults to 0." rather than just "Default 0" -- generally, we try to use natural English wording rather than being terse.

Thanks, jhodgdon. This needs another reroll before any more changes can be made. I'll get on that as soon as I can.

status as per #47/#48

No additional fixes, just a reroll to apply to current head.

Thanks! I started over reviewing this patch, and it looks pretty good. A few things before it is ready to commit:

a) menu.admin.inc

/**
- * Recursive helper function for menu_overview_form().
+ * Helps menu_overview_form() build the menu overview form.
  *
  * @param $tree
- *   The menu_tree retrieved by menu_tree_data.
- * @param $delta
- *   The default number of menu items used in the menu weight selector is 50.
+ *   The array of menu items retrieved by menu_tree_data().
+ *
+ * @return
+ *   An array containing a tree of menu item form elements.
  */
 function _menu_overview_tree_form($tree, $delta = 50) {

Why was @param $delta taken out here? I agree that the doc isn't good, but we need something. :)

b) This patch is ... odd. There are two different sections for several of the files (menu.admin.inc, menu.module, etc.). I only reviewed the first sections... please redo the patch.

c) There are code changes in here -- please don't make code changes part of this API docs cleanup effort. Example from one of the tests:

-    $this->admin_user = $this->drupalCreateUser(array(
+    $this->adminUser = $this->drupalCreateUser(array(

At this point, I stopped reviewing... the patch has some stuff in it that I think just doesn't belong on this issue?

Please take out the code changes, such as:

+  /**
+   * An administrative user.
+   *
+   * @var
+   */
+  protected $adminUser;

I know you are trying to be helpful, but this effort is *only* for documentation header changes, as noted above. There is a separate effort underway for cleaning up other coding standards issues, if you want to get invovled -- #1518116: [meta] Make Core pass Coder Review -- and we really are trying to keep these different clean-up efforts contained and limited to what they are supposed to cover.

Thanks!

Lars: I believe there's already an issue on that "coder clean-up" for the Node module -- please post your patch there (more relevant; it's not related to this effort).

There is still a code addition at the top of this patch -- where is this coming from?

public static $modules = array('menu');
 
+  /**
+   * An administrative user.
+   *
+   * @var
+   */
+  protected $admin_user;
+

I took a quick scan through the rest of the file... it looks pretty good on first glance (obviously it will need a more careful review)... I did notice that one added @file block (CSS file) needed a blank line after it:
http://drupal.org/node/1354#files

And technically, this type of change (and re-wrapping in-code comments) is out of scope for this issue and belongs on the "Bring core up to coding standards" issue instead:

-  $order = array_flip(array_keys($form_state['input'])); // Get the $_POST order.
-  $form = array_intersect_key(array_merge($order, $form), $form); // Update our original form with the new order.
+
+  // Get the $_POST order.
+  $order = array_flip(array_keys($form_state['input']));
+  // Update our original form with the new order.
+  $form = array_intersect_key(array_merge($order, $form), $form);

These changes also have more possibility of conflicting with other (code) patches, so maybe you could move those hunks over to #1533252: Make menu module pass Coder Review instead so that the code cleanups are contained there?

Thanks!

RE: what is in scope for this issue -- see the issue summary above or the issue summary for the meta-issue that this is part of... haven't we been through this on some other issues?

Anyway, basically, what's in scope is things in API docs comments (between /** */ that is), with param/return types specifically excluded. Anything that is not in an API doc comment is definitely excluded, and the main cleanup goals are listed in the bullet points of this issue summary. So. It would be great, if you want to contribute patches to the "API Docs Cleanup" issues (which is most welcome!), if you could confine your patches to just /** */ comments. Thanks!

On to reviewing this patch -- it's mostly great! I'd like to see a couple of things in added documentation fixed before it gets committed though:

a) The word "id" is a psychological concept (ego, superego, id), while "ID" is an abbreviation for "identifier". Such as here:

+*   (optional) Parent menu link id. Defaults to zero.

Should be "link ID".

b) It would be great if the revised documentation lines followed better grammar, such as:

* Tests standard menu functionality using navigation menu.
    */
   function doStandardMenuTests() {

==> needs "the" in "using the navigation menu" [this appears several times in the patch]

+   * Tests the addition of custom menu using CRUD functions.
    */
   function addCustomMenuCRUD() {

==> needs "a' in "of a custom menu using..."

+   * Attempts to add menu link with invalid path or no access permission.
==> "add a menu link with an invalid..."

+   * Fetches the menu item from database and compares it to specified array.
==> "from the database..."

etc.

c) The word "Boolean", used in documentation text, should always be capitalized, such as here:

+   *   (optional) A boolean indicating whether the meny should be expanded.

d) I didn't look at the code to be sure, but:

/**
- * Menu callback; Build the form presenting menu configuration options.
+ * Form constructor for the menu configuration form.
  *
- * @ingroup forms
  * @see menu_configure_submit()
+ * @see menu_menu()
+ * @ingroup forms
  */
 function menu_configure($form, &$form_state) {

If this is actually a page callback used in menu_menu(), it should be documented as "Menu callback: Form constructor for...". If it isn't, it probably doesn't need @see menu_menu().

e) Need a blank line after @file doc block: [menu.js too]

+++ b/core/modules/menu/menu.admin.js
@@ -1,3 +1,7 @@
+/**
+ * @file
+ * Adds menu options to the node type form.
+ */
 (function ($) {

f) Hooks in api.php file -- verb tense standard is different (and incorrect in the original and this patch). See
http://drupal.org/node/1354#hooks

g)

/**
- * Page callback.
- * Get all the available menus and menu items as a JavaScript array.
+ * Page callback: Gets all the available menus and menu items.
+ *
+ * The menus and menu items are output as a JavaScript array.
+ *
+ * @return Symfony\Component\HttpFoundation\JsonResponse
+ *   A JSON response object.
+ *
+ * @see menu_menu()
  */
 function menu_parent_options_js() {

This seems... slightly confusing or contradictory? I think if all the information about the return value was put into the @return, it would be clearer, because it would probably say something like "A JSON response object containing the menus and menu items as a JavaScript array" or something like that. As it is, one line seems to say it's just returning a JS array, and the other says it's a JSON object.

h)

/**
- * Helper function to get the items of the given menu.
+ * Gets the items of the given menus.
+ *
+ * @param $menus
+ *   An array of menu names and titles, such as from menu_get_menus().
+ * @param $available_menus
+ *   An array of menu names and titles of menus that $item can be assigned to.
+ * @param $item
+ *   The menu item for which to generate a list of parents. If
+ *   $item['mlid'] == 0 then the complete tree is returned.
+ *
+ * @return
+ *   An array of menu items to be used as options in a select form element.
  */
+
 function _menu_get_options($menus, $available_menus, $item) {

- There's a stray blank line between doc and function
- Generally this doc doesn't make the function clear to me and I'd have to read the code to figure out what's going on (never a sign of great documentation!). For instance, the return value says it's an array of menu items... I don't think so. Menu items elsewhere are arrays, and this can't be an array of menu item arrays if it's to be used as a select? Then param $item mentions it's generating a list of parents, but elsewhere it's not talking about parents... I'm not sure which is correct but something's wrong... I guess maybe it's generating a list of *possible* parents to assign $item to? but I'm not sure. Anyway, I think it needs a bit more attention to really document what the parameters and return value are.

i) The next function in the patch, _menu_parents_recurse(), has the same problem about mixing up an options list with menu arrays in its $options param docs.

j)

+ *   (optional) If FALSE return only user-added menus, or if TRUE also include
+ *   the menus defined by the system.  Defaults to TRUE.

Stray extra space (this is the last line of the patch, not sure what function it is documenting)

Setting patch to "needs work" since #66 says it doesn't address review item (h) yet.

Coming back to this for the first time since shortly after the sprint at DrupalCon Denver. Here's a reroll that includes my first attempt at #65 h).

I apologize, but I will probably not have time to review/commit this patch until January. :(

No problem! I have some time to devote to core this month and saw this piece of low-hanging fruit that I'd worked on back in March. In the meantime I'll be looking through the queue to see what else I can help out with while I have some free cycles.

I had some time today to review this patch... again, sorry for the delay!

It is mostly good! I found a few problems that need to be fixed:

a)

   public static $modules = array('menu', 'test_page_test');
-
+  /**
+   * The profile to use with this test class.
+   *
+   * @var string
+   */
   protected $profile = 'standard';

The blank line should not have been removed between these member variables. This problem appears several times in the patch.

b)

+   * @var
+   */
   protected $big_user;

@var needs to be followed by the data type of the member variable. This problem appears several times in the patch.

c)

/**
-   * Add custom menu using CRUD functions.
+   * Tests the addition of a custom menu using CRUD functions.
    */
   function addCustomMenuCRUD() {

I don't think this method actually performs tests -- I think it is a helper function that just adds a menu. So the documentation should probably stay how it was (with a verb change from Add to Adds). Also applies to the addCustomMenu() method that follows, deleteCustomMenu(), etc.

d) In addMenuLink(), the default value for $menu_name in the documentation does not match the function signature. Actually, it would be better to omit all of the "Defaults to xyz" text in the documentation, because it makes the documentation less likely to be correct, at least if the default is easily seen in the function signature. I would only include a "Defaults to..." if the default is fairly complicated to explain and is not obvious from the function signature, or if the default has some special meaning, like "Defaults to xyz, meaning that ...".

e)

+   * @return array
+   *   The cretaed menu link.
    */
   function addMenuLink($plid = 0, $link = '<front>', $menu_name = 'tools', $expanded = TRUE, $weight = '0') {

created is misspelled in the @return

f)

+ * @ingroup forms
  */
 function menu_overview_form_submit($form, &$form_state) {

Form submission handlers should not have "@ingroup forms" in their documentation. That is only for the form generation functions.

g)

+++ b/core/modules/menu/menu.admin.js
@@ -1,3 +1,8 @@
+/**
+ * @file
+ * Adds menu options to the node type form.
+ */
+ s
 (function ($) {
 

Typo -- that line containing "s".

h) Very bottom of the patch:

/**
- * Return an associative array of the custom menus names.
+ * Returns an associative array of the custom menus names.

"menus" should either be menu (if it is being used as an adjective) or menus' (if it is possessive).

Okay, I think this patch takes care of all of the items noted in #76. I also removed one hunk that changed documentation for a function (hook_block_info(), IIRC) that was removed in #1880620.

Much better, thanks! There are still a couple of little issues to resolve:

a) MenuTest.php file:

*
    * @var array
    */
-  public static $modules = array('menu', 'block');
 
+  public static $modules = array('menu', 'block');
+  /**

It looks like this adds a blank line between the docblock and what it is documenting (it shouldn't), and then removes the blank line between this and the next docblock (it shouldn't).

b) later in that file, the addMenuLink() function:

/**
-   * Add a menu link using the menu module UI.
+   * Tests the addition of a menu link using the menu module UI.

I don't think it is really a "test" -- it's more of a utility function. We normally just document methods as "Tests..." if they start with the word test. I would leave the documentation first line saying "Adds a menu link..." rather than changing to "Tests the addition of a menu link".

c) menu.admin.inc - menu_overview_form()

/**
- * Form for editing an entire menu tree at once.
+ * Menu callback: Form constructor for the menu overview form.

Should be "Page callback:" not "Menu callback:". Same problem in menu_edit_item(), menu_edit_menu(), menu_configure().

d) menu.admin.inc - menu_edit_menu_submit()

/**
- * Submit function for adding or editing a custom menu.
+ * Form submission handler for menu_edit_menu()

Missing . at end of line.

Okay, here we go again. It was kind of a hairy re-roll because of menus becoming entities. I skimmed through #1814916, but I think I need a full night's sleep before I can wrap my brain around all of the changes there. The patch applies, but it probably needs a review to make sure all of the docblocks are still current. In particular, there are a couple of @see directives pointing to menu_edit_menu(), which I don't think exists any longer.

#79: 1326672-79-menu-docs.patch queued for re-testing.

That is true: the function menu_edit_menu() does not appear to exist in Drupal 8. Setting this to Needs Work on that basis. I haven't reviewed the rest of the patch yet in detail, but it would be good if someone could check to see if the notes in #78 have been addressed.

First a re-roll of #79.

Actually, it appears that all of the items in #78 have been addressed. This is ready for a review.

Great patch so far!

Still lots of coding standards issues not covered here though as well as the function changes as mentioned in #82.

menu.module line 240

/**
 * Implements hook_menu_insert()
 */

needs a full stop at the end.

menu.admin.inc line 56

/**
 * Page callback: Form constructor for the menu overview form.
 *
 * Shows a custom menu's menu links that are accessible to the current user and
 * relevant operations. Allows editing an entire menu tree at once.
 *
 * This form constructor can be integrated as a section into another form. It
 * relies on the following keys in $form_state:
 * - menu: A loaded menu definition, as returned by menu_load().
 * - menu_overview_form_parents: An array containing the parent keys to this
 *   form.
 * Forms integrating this section should call menu_overview_form_submit() from
 * their form submit handler.
 *
 * @param $menu
 *   An array defining a custom menu.
 *
 * @see menu_menu()
 * @see menu_overview_form_submit()
 * @ingroup forms
 */
function menu_overview_form($form, &$form_state) {

The @param is incorrect.

and line 479

/**
 * Menu callback; Check access and present a confirm form for deleting a menu
 * link.
 */
function menu_link_delete_page(MenuLink $menu_link) {

should be on one line or in short/long format.

Happy to help out here if this is not being progressed further at the moment?

I appreciate the review @Pete B! Here's a patch to fix those issues.

These issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.

Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!