Consistently refer to "menu links" as that, not "menu items", in the UI

@jhodgdon, added screenshots to IS. With the patch there is still one instance of 'menu item' in the 'Expand all menu items' setting for menu blocks:

Also I was unable to trigger the error 'The selected menu item is not under one of the selected menus.' Anyone know a good way to do this? Or can we just verify via the patch? I tried creating a menu link in a node and then updating the content type settings to change the available menus, but the node edit form correctly updated in every case.

Updated patch and some more screenshots.

Interdiff for good measure :)

I'm going to review now. Tagging for Bug Smash!

Thanks for the update. The issue summary says we are supposed to focus on menu_ui yet I do see some code outside of that changed:

• core/modules/menu_link_content/src/Plugin/migrate/source/MenuLink.php
• core/modules/system/src/Plugin/Block/SystemMenuBlock.php
• core/modules/system/system.permissions.yml

So... I checked for any remaining "menu item" and "parent item" that were not in 1) tests, 2) comments, or 3) backend code like schemaDefinition.

If we are to change *any* UI-facing code, then I see a few more things that appear to be shown to the user:

"menu item":

• core/modules/forum/forum.module (forum_help): "...provides a default <em>Forums</em> menu item..."
• core/modules/shortcut/shortcut.module (shortcut_help): "...provides a corresponding menu item..."
• core/modules/toolbar/toolbar.info.yml (description): "...top-level administration menu items and links..."
• core/modules/views/src/Plugin/views/display/Page.php (various places, e.g. in optionsSummary): e.g. $options['menu']['setting'] = $this->t('Parent menu item');

"parent item":

• core/modules/views/src/Plugin/views/display/Page.php (buildOptionsForm): "...The path of a parent item..."

If these shouldn't be included, let me know and I'll just focus on the changes in the patch.

Thanks for working on this! I love efforts to make Drupal more consistent and self-documenting. A menu "link" makes much more sense to mere mortals than an "item".

Patch looks really good (so far). Nothing to complain about that's already here.

However, this line from the summary concerns me:

As a note, the API docs in menu_ui.module nearly consistently call things "menu items", but this issue is not asking for that to change.

IMHO, if the scope of this issue is 'Consistently refer to "menu links" as that, not "menu items"' I'm not sure why we should restrict it to the UI. It's a pet peeve to have the docs diverge from what they're documenting. Why not fix all of this at once?

So NW for either an expanded patch/scope here, or a follow-up to fix everything we're leaving out:

Before:

% grep -ir --exclude='node_modules' --exclude='LICENSE.txt' 'menu item' core | wc
     219    4400   41252

After:

% grep -ir --exclude='node_modules' --exclude='LICENSE.txt' 'menu item' core | wc
     205    4044   37940

A brief example of those 205 hits:

core/lib/Drupal/Core/Menu/MenuLinkTreeInterface.php: * The menu item's LI element is given one of the following classes:

IMHO that's just weird: a class called "MenuLinkTreeInterface" that's talking about a "menu item"... 😉

Thanks/sorry,
-Derek

p.s. I also noticed we've got some hits for "menuitem", too:

 grep -ir --exclude='node_modules' --exclude='LICENSE.txt' 'menuitem' core | wc
      51    5168  203883

p.p.s. Ooof, x-post. Yeah, +1 to #12 - #14. ;)

Okay, sounds reasonable. Follow-up opened as a child issue:

#3159213: Consistently refer to "menu links" as that, not "menu items", in code comments and docs

Needs follow-up 😉

Still NW for #12 and #16.

Thanks,
-Derek

"Patch failed to apply". It's just minor fuzz due to #3155258: Use American English spelling of "gray"

Here's a trivial re-roll. Interdiff is empty, it's only s/grey/gray/ in a context line that's different, so here's a raw diff of the 2 patch files, instead.

I picked up a few more (looks like a lot more but here are a few) and I could make a new patch. Are these in scope?

In system/templates/admin-block-content.html.twig

the array represents an administrative menu item

In seven/templates/admin-block-content.html.twig

* - content: List of administrative menu items. Each menu item contains:
...
* - description: Description of the administrative menu item.

In MenuLinkTreeInterface.php

lib/Drupal/Core/Menu/MenuLinkTreeInterface.php:82:   * The menu item's LI element is given one of the following classes:
lib/Drupal/Core/Menu/MenuLinkTreeInterface.php:83:   * - expanded: The menu item is showing its submenu.
lib/Drupal/Core/Menu/MenuLinkTreeInterface.php:84:   * - collapsed: The menu item has a submenu that is not shown.
lib/Drupal/Core/Menu/MenuLinkTreeInterface.php:85:   * - leaf: The menu item has no submenu.
lib/Drupal/Core/Menu/MenuLinkTreeInterface.php:108:   *   The ID of an item in the storage.

@sime: Thanks, but all of those are comments, and therefore in scope for #3159213: Consistently refer to "menu links" as that, not "menu items", in code comments and docs, not here.

Cheers,
-Derek

Looks good. I've applied the patch. I've visually eye-balled each change in the patch. I grepped code for " item" then went through each reference to menu and confirmed there are no more end user facing references to "menu item".

Looks good to me - reviewing the patch you can see us switching between item and link in the same sentence, let alone across the module.

Committed dcec533 and pushed to 9.1.x. Thanks!