In my module.install file, I have:

  $menu = array(
    'menu_name' => 'nux_conf',
    'title' => $t('NUX Config'),
    'description' => $t('NUX User items.'),
  );
  menu_save($menu);

Following these steps:

  1. Enable the module
  2. Disable the module
  3. Try to delete the menu created from the menu admin page, I get the issue below.

Error: The machine-readable name must contain only lowercase letters, numbers, and hyphens.

The "menu_name" box is in red, but disabled. I'm unable to edit the menu name. I have to go to the database tables and update the name in two tables. I'm then able to delete the menu.

CommentFileSizeAuthor
#18 menu-names-phpdoc-993134.patch540 byteslyricnz
#15 menu-names-phpdoc-993134.patch539 byteslyricnz
#13 menu-names-phpdoc-993134.patch533 byteslyricnz
#5 menu-names-phpdoc-993134.patch495 byteslyricnz
#3 invalid_menu_names_993134_3.patch869 byteslyricnz
#2 invalid_menu_names_993134_2.patch870 bytesadam.hammouda
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tazzytazzy’s picture

tazzytazzy CreditAttribution: tazzytazzy commented

Oh, and the uninstall fails too:

/**
 * Implements hook_uninstall().
 */
function ahx_conf_uninstall() {
  // Delete the development menu.
  $ahx_conf_menu = menu_load('ahx_conf');
  menu_delete($ahx_conf_menu);
}
adam.hammouda’s picture

adam.hammouda CreditAttribution: adam.hammouda commented
Version: 7.0-rc1 » 7.x-dev
Status: Active » Needs review
FileSize
invalid_menu_names_993134_2.patch870 bytes

The menu_save function now throws an exception if you attempt to save an uninitialized, unnamed or invalidly named menu. This seems to be the appropriate behavior to take in this case - as something akin to drupal_set_message(..., 'error') would not terminate the menu creation process (and modifying the method definition to return a TRUE/FALSE would not seem inline with currently established drupal conventions.)

lyricnz’s picture

lyricnz CreditAttribution: lyricnz commented
FileSize
invalid_menu_names_993134_3.patch869 bytes

I can reproduce this issue.

Really pedantic thing: I presume you copied the regex from menu_edit_menu(), but if we're just looking for valid/invalid, we don't need to match > 1 character, so you don't need the '+' in the regex. That's the only change in this patch.

Also, even with the exception thrown, this doesn't actually stop the module being enabled.

lyricnz’s picture

lyricnz CreditAttribution: lyricnz commented
Status: Needs review » Needs work

After discussing this with the community, we felt that this was a classic case of garbage-in => garbage-out (which Drupal explicitly doesn't validate). The correct solution to this situation is to document the valid format of menu_name.

lyricnz’s picture

lyricnz CreditAttribution: lyricnz commented
FileSize
menu-names-phpdoc-993134.patch495 bytes

Attached a phpdoc change that describes the valid characters.

lyricnz’s picture

lyricnz CreditAttribution: lyricnz commented
Status: Needs work » Needs review

CNR

lyricnz’s picture

lyricnz CreditAttribution: lyricnz commented

Just so we're clear, I'm no longer supporting being more defensive about the parameters to menu_save(), just more explicit about what is allowed in the phpdoc, especially since it's not a classic "machine name" (which is alpha-numeric plus underbar).

cafuego’s picture

cafuego CreditAttribution: cafuego commented
Status: Needs review » Reviewed & tested by the community

Makes sense to me.

scor’s picture

scor CreditAttribution: scor commented

#5: menu-names-phpdoc-993134.patch queued for re-testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Component: menu system » documentation
Status: Reviewed & tested by the community » Needs review

Moving to the documentation queue for review.

Is the description there sufficient? I'm not sure if we use regex patterns elsewhere for describing allowed values. I'm fine with it, since it's developer-facing text, but just want to make sure we're being consistent.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

I don't know of any other examples of using regexp in function docblocks, and would prefer it to be written out in words.

lyricnz’s picture

lyricnz CreditAttribution: lyricnz commented
Status: Needs work » Needs review
FileSize
menu-names-phpdoc-993134.patch533 bytes
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

Better!
A few little things to fix:
a) - is a hyphen, not a dash. I don't think you need to put the character '-' in there if you call it a hyphen.
b) Drupal punctuation standards: list of 3 items has , before the 'and'. So: apples, bananas, and oranges.
c) Maybe say "composed of lowercase letters..." (add "composed of") and leave out the "only"?

lyricnz’s picture

lyricnz CreditAttribution: lyricnz commented
Status: Needs work » Needs review
FileSize
menu-names-phpdoc-993134.patch539 bytes

You have no idea how little I care about this issue :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

hyphen -> hyphens and it is done... thanks for caring enough to at least make a new patch! :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Title: Unable to delete menu with underscore » menu_save needs to document the form of the menu name

fixing title to reflect what we're actually doing here

lyricnz’s picture

lyricnz CreditAttribution: lyricnz commented
Status: Needs work » Needs review
FileSize
menu-names-phpdoc-993134.patch540 bytes
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

Thanks! That's good now.

7.x/8/x please...

lyricnz’s picture

lyricnz CreditAttribution: lyricnz commented

FYI the text in the UI (D6) that corresponds to this validation is "This name must contain only lowercase letters, numbers, and hyphens, and must be unique."

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Well, the d7 text in your patch also includes the word "unique", so I think we've covered it.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x and 7.x. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D7

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

Issue tags: +Needs backport to D7

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.