Menu machine-name validation error (The menu name can't be longer than 27 characters)

I experience the same problem and here are more info :

the problem occurs when the the NAME of the menu (and not the TITLE of the menu) is more than 22 characters length. I've made several tests, and when the name of the menu is 23 characters length or over the problem occurs.

Note that the length of 22 characters is 27 - 5. And 5 seems to be the length of the menus prefix "menu-". Can it help ?

In the case of ag888 it is certainly the same problem because according to the link "http:///?q=admin/build/menu-customize/menu-articles-additional-menu" that is used to manage the menu we can see that the name of the menu "articles-additional-menu" is 24 characters length.

I've also been creating some menus with long names and found them to have this same bug. I took a look through the code and whilst I'm new to contributing, I think I can see the problem.

When first creating a new menu the menu_edit_menu_validate function prepends the menu name with the string 'menu-', which is then stored to the db.

The validation script for checking the length of the menu name (which is limited to 32 characters in the table) checks for a string that is not greater than 27 characters, set by the value of MENU_MAX_MENU_NAME_LENGTH_UI. Since this validation is always called when submitting the menu settings form, whether creating a new menu or editing an existing one, it is the case that the value for 'MENU_MAX_MENU_NAME_LENGTH_UI' needs to be adjusted depending on whether saving or editing the form. When saving the a new menu we are checking against the value of 27 characters, when editing the form we need to be checking against the value of 32 characters because we have to allow for the 'menu-' string having been added.

Since the menu name is not something that can be changed when editing the menu it would be simpler to bypass validation of the menu name in any state other than 'create new menu'. However, I do not know if this is best practice since it is making presumptions about how this function is being used. I have therefore written a patch that adjusts the value max length based on mode.

Here is a patch that is for the modules/menu/menu.admin.inc file. I am a beginner and I'm not sure the patch is working as easily as it should in terms of locating the file, when I tested it I had to prompt it with which file to update. I'll try again at making a better path. Any help would be appreciated thanks.

Ditto, same bug here. Everything I use is the very latest and greatest as of today ie Drupal 6.6 etc and this is the first Drupal 6 site I am doing.

Except for this little bit:

This "BUG!" was reported on August 8, 2008. It is now 18 November 2008 and the status on this is still unassigned. There is patch code and it is yet to be reviewed.

AFAIK the menu system is a critical part of Drupal and is certainly a core function.

Is someone going to "fix" this?

Yes, I am also surprised that a bug in the menu system like this isn't getting a little more attention... I guess folks are using short titles! It is my first patch and I am keeping an eye on the thread to see if I'm doing things the right way. Please review someone?

thanks

patch review.... please someone? :-)

Subscribing.

Please note this also appears to happen in the bookmark module for Drupal 6.

I tried to delete a bookmark with a name of about 22 characters and it threw up an error message and I was unable to delete.

I applied the same principle to modifying the file bookmarks.admin.inc as described in the patch created by kjay and it seemed to work.

Also note that I changed the defined variable MENU_MAX_MENU_NAME_LENGTH_UI in the bookmark module to BOOKMARK_MENU_MAX_MENU_NAME_LENGTH_UI otherwise it keeps the value of 27 set in menu.module not the 23 set in bookmarks.module!

Not sure how to properly make a patch, but code changes in bookmarks.admin.inc as follows:

if (preg_match('/[^a-z0-9-]/', $item['menu_name'])) {
form_set_error('menu_name', t('The menu name may only consist of lowercase letters, numbers, and hyphens.'));
}

// adjust max name length based on insert or edit
$name_length = BOOKMARK_MENU_MAX_MENU_NAME_LENGTH_UI;
if (!$form['#insert']) {
$name_length = BOOKMARK_MENU_MAX_MENU_NAME_LENGTH_UI + 9;
}
if (strlen($item['menu_name']) > $name_length) {
form_set_error('menu_name', format_plural(BOOKMARK_MENU_MAX_MENU_NAME_LENGTH_UI, "The menu name can't be longer than 1 character.", "The menu name can't be longer than @count characters."));
}

if ($form['#insert']) {

And make sure you replace the other instance of MENU_MAX_MENU_NAME_LENGTH_UI with BOOKMARK_MENU_MAX_MENU_NAME_LENGTH_UI in the same file and in bookmarks.module

This bug was first noticed in April 2008 in Drupal version 6.2 and several users have confirmed it.
Kjay did a great job in providing us with a patch, which has been tested, but it has not been incorporated as of now (D6.8). I have applied the patch code lines manually to my 6.8 installation and it does the job perfectly. I have not tested the automated 'apply patch' procedure http://drupal.org/patch/apply

I am not sure of the next step to get this implemented. It is a very simple code correction, but if not done it really causes havoc, and you end up with un-deletable duplicate menus.

Patch applies with no fuzz on D6.8 – and works as promised.
It sounds like this patch has been reviewed – too early to mark RTBC?

Should first be committed to Drupal 7 and then to Drupal 6, so we ensure that the fix is in the latest version and we are not going to have any regressions.

I'm 6.10 and the bug is still not fixed. But the patch works. Thanks, kjay!

(off topic) The numbering system for Drupal revisions is also a bit confusing. 6.10 is more advanced than 6.9? (/off topic)

Patch against HEAD attached. I don't think the fix in #3 is the best way to resolve this: the machine name for the menu is only entered during the insert phase, so it does not make sense to validate it on the edit screens. Hence there is no need to work with 2 different max length settings.

Also tagging this "Review Exchange": you scratch my back, I'll scratch yours. I'll happily review a patch of similar size for someone who reviews this one.

This will need a test for D7.

Do you mean a regression test (verifying that the incorrect validation message is no longer displayed after change) or just testing the validation message in general?

mmm, didn't mean to set it back to CNR....

Ok, new patch, this time with some tests included:

1. Extra test adding a custom menu with a name that is longer than MENU_MAX_MENU_NAME_LENGTH_UI (and verify that the validation error is returned)
2. Extra assertion on the regular add custom menu test to ensure that the validation message is not show when menu_name length = MENU_MAX_MENU_NAME_LENGTH_UI.
3. Changed the regular add custom menu test to use the maximum length for menu_name: this way we automatically test that the situation described in this issue does not (re-)occur.

Removing the Review Exchange tag. All our reviews work the same way. It's just implied.

Cool. I wasn't aware that this was an unwritten rule in the community.

@kjay: I just noticed that you offered the solution of removing the validation on anything but inserts:

Since the menu name is not something that can be changed when editing the menu it would be simpler to bypass validation of the menu name in any state other than 'create new menu'. However, I do not know if this is best practice since it is making presumptions about how this function is being used. I have therefore written a patch that adjusts the value max length based on mode.

So the answer is yes, in this case I think moving the validation to inside the "insert" logic is the best thing to do :)

Is this fixed in 6.10? I'm seeing the same error message. Can't rename, or delete existing menus ..... If someone could advise, it would be appreciated.

Thanks.

It is not fixed in 6.10, but you can use the patch in #13 to make the change. The patch in #17 makes the same change to the executable code but also makes changes to the .test file which you will not have, so that is why I suggest you use the patch in #13.

Actually, it is just a move of one line 'if ($form['#insert']) {' so you could edit function menu_edit_menu_validate() in /modules/menu/menu.admin.inc manually.

Jonathan

I have tested the patch given in #17 on my D6.10 site. Obviously I have to skip the first file to be patched as I dont have the menu.test file. But the second change works fine, accepting the fuzz of 2. It is exactly the same code change as in #13.

I have tested the change by adding menus with machine length of 21, 22, 26, 27 and can edit the description afterwards, all fine.
Entering a new menu with machine name length 28 gives the error message as expected.

It seems that we are all agreed that this correction is the proper way to fix it, my manual testing in D6 is fine, and it passed the automated D7 tests. So I have marked this as 'reviewed & tested'.

Jonathan

bump.

Committed to CVS HEAD. Thanks!

I think there is a request to look at this for D6 too...

Needs a re-roll to remove the test hunk.

Bump, forgive the bump, this is plaguing a number of my Drupal sites, it would be great to have this in the next D6 release.

@jmburnz You can download the patch in #13 which works with D6, or you can make the change manually as described in #22, but its easier to download the pacth, then you can re-apply it to every Drupal upgrade until it is included in the official release.

Yes, I have patched a few of my sites that have this issue, but some I am loath to because I have to hand them off to someone else, but I'll do what it takes right now - patch it is!

Here's the patch for Drupal 6.

Yes, this patch installs cleanly in D6.14 and the change is identical to the previous patches tested on D7 and D6, as above (see my testing in #23).
But do you know why did this one not get submitted for automated testing?

@jonathan1055: AFAIK automated tests are only for Drupal 7+ patches. Can you please mark this as "reviewed & tested by the community"?

OK with me.

thank you for the patch... working

Newbie to Drupal and Open Source in general here, so please forgive my process ignorance and likely lack of decorum...If I am reading the posts and statuses correctly it looks like the issue is agreed upon but there are two approaches to the fix offered? I see no reference to an automated patch in the offing, so I have applied the changes by hand as noted in the diff as provided in in #31 above.

This passed the UCT Battery (unsophisticated customer test that didn't batter me *innocent grin*)

It cleared up the problem I was experiencing with no visible side effects. THANK YOU ALL!

For future reference, is there a onestop reference where I can learn how to participate in the process appropriately? For instance, should I have patiently waited for some kind of formal announcement of an auto install patch? Mostly, I don't want to patch up my code in a way that will interfere with future releases...is there an established set of best practices I should be following?

My immediate reaction was that this does not do length validation on editing menu items anymore. The reason it was in the general part of the function was to always validate length. So you could update to a name longer then allowed when editing a menu? Wait, no, the form item already has a #maxsize property set. So then, is it not displaying two error messages when the length exceeds the allowed characters?

If it does not do two messages, then seems like we miss validation on editing an item, if it does, looks like the initial submission is clumsy. Either way, we have stuff to improve here I think.

@Gabor:

Thanks for your feedback:

My immediate reaction was that this does not do length validation on editing menu items anymore.

This field (Menu name) is only present on the "insert" form. When editing you are not allowed to change the machine name, and the form element is missing. If I recall correctly, this is the root cause of the issue: insert allows you to add menu names up to 27 characters, and after validation prepends the machine name with "menu-" before inserting it in the database. The edit form then does not allow you to change the menu name (the machine name element is missing entirely), but it does try to validate the machine name, resulting in an error since the machine name is now 27 + 5 characters ==> too long.

Hence the reason for moving the length validation to inside the insert logic...

Wait, no, the form item already has a #maxsize property set. So then, is it not displaying two error messages when the length exceeds the allowed characters?

Actually it doesn't, but you do bring up an interesting point, why isn't Form API taking care of validating the field length. Turns out #maxsize is not a valid property for a textfield, it should actually be #maxlength. Changing #maxsize to #maxlength and removing the length validation from menu.admin.inc entirly fixes this issue and keeps validation intact.

I'll try and roll a D7 patch later today...

Patch attached which removes the validation and relies on Form API length validation. Slightly altered test to accomodate the changed error message.

Still present in D6 ofcourse... D6 patch attached, just to get some activity back in the thread ;-)

#39 still applies for D7

Patch hasn't been added to HEAD yet I guess... But then again, there are other more urgent issues I bet :-)

Can I get some specific direction on how to apply this patch? I've found this section of code in the menu.admin.inc file, but I'm not sure if it should all be replaced, or if I should add only the patch at the bottom of the document, etc. etc.

 $form['menu_name'] = array(
      '#type' => 'textfield',
      '#title' => t('Menu name'),
      '#maxsize' => MENU_MAX_MENU_NAME_LENGTH_UI,
      '#description' => t('The machine-readable name of this menu. This text will be used for constructing the URL of the <em>menu overview</em> page for this menu. This name must contain only lowercase letters, numbers, and hyphens, and must be unique.'),
      '#required' => TRUE,
    );
    $form['#insert'] = TRUE;
  }
  $form['#title'] = $menu['title'];
  $form['title'] = array(
    '#type' => 'textfield',
    '#title' => t('Title'),
    '#default_value' => $menu['title'],
    '#required' => TRUE,
  );
  $form['description'] = array(
    '#type' => 'textarea',
    '#title' => t('Description'),
    '#default_value' => $menu['description'],
  );
  $form['submit'] = array(
    '#type' => 'submit',
    '#value' => t('Save'),
  );

  return $form;
}

/**
 * Submit function for the 'Delete' button on the menu editing form.
 */
function menu_custom_delete_submit($form, &$form_state) {
  $form_state['redirect'] = 'admin/build/menu-customize/'. $form_state['values']['menu_name'] .'/delete';
}

/**
 * Menu callback; check access and get a confirm form for deletion of a custom menu.
 */
function menu_delete_menu_page($menu) {
  // System-defined menus may not be deleted.
  if (in_array($menu['menu_name'], menu_list_system_menus())) {
    drupal_access_denied();
    return;
  }
  return drupal_get_form('menu_delete_menu_confirm', $menu);
}

/**
 * Build a confirm form for deletion of a custom menu.
 */
function menu_delete_menu_confirm(&$form_state, $menu) {
  $form['#menu'] = $menu;
  $caption = '';
  $num_links = db_result(db_query("SELECT COUNT(*) FROM {menu_links} WHERE menu_name = '%s'", $menu['menu_name']));
  if ($num_links) {
    $caption .= '<p>'. format_plural($num_links, '<strong>Warning:</strong> There is currently 1 menu item in %title. It will be deleted (system-defined items will be reset).', '<strong>Warning:</strong> There are currently @count menu items in %title. They will be deleted (system-defined items will be reset).', array('%title' => $menu['title'])) .'</p>';
  }
  $caption .= '<p>'. t('This action cannot be undone.') .'</p>';
  return confirm_form($form, t('Are you sure you want to delete the custom menu %title?', array('%title' => $menu['title'])), 'admin/build/menu-customize/'. $menu['menu_name'], $caption, t('Delete'));
}

/**
 * Delete a custom menu and all items in it.
 */
function menu_delete_menu_confirm_submit($form, &$form_state) {
  $menu = $form['#menu'];
  $form_state['redirect'] = 'admin/build/menu';
  // System-defined menus may not be deleted - only menus defined by this module.
  if (in_array($menu['menu_name'], menu_list_system_menus())  || !db_result(db_query("SELECT COUNT(*) FROM {menu_custom} WHERE menu_name = '%s'", $menu['menu_name']))) {
    return;
  }
  // Reset all the menu links defined by the system via hook_menu.
  $result = db_query("SELECT * FROM {menu_links} ml INNER JOIN {menu_router} m ON ml.router_path = m.path WHERE ml.menu_name = '%s' AND ml.module = 'system' ORDER BY m.number_parts ASC", $menu['menu_name']);
  while ($item = db_fetch_array($result)) {
    menu_reset_item($item);
  }
  // Delete all links to the overview page for this menu.
  $result = db_query("SELECT mlid FROM {menu_links} ml WHERE ml.link_path = '%s'", 'admin/build/menu-customize/'. $menu['menu_name']);
  while ($m = db_fetch_array($result)) {
    menu_link_delete($m['mlid']);
  }
  // Delete all the links in the menu and the menu from the list of custom menus.
  db_query("DELETE FROM {menu_links} WHERE menu_name = '%s'", $menu['menu_name']);
  db_query("DELETE FROM {menu_custom} WHERE menu_name = '%s'", $menu['menu_name']);
  // Delete all the blocks for this menu.
  db_query("DELETE FROM {blocks} WHERE module = 'menu' AND delta = '%s'", $menu['menu_name']);
  db_query("DELETE FROM {blocks_roles} WHERE module = 'menu' AND delta = '%s'", $menu['menu_name']);
  menu_cache_clear_all();
  cache_clear_all();
  $t_args = array('%title' => $menu['title']);
  drupal_set_message(t('The custom menu %title has been deleted.', $t_args));
  watchdog('menu', 'Deleted custom menu %title and all its menu items.', $t_args, WATCHDOG_NOTICE);
}

/**
 * Validates the human and machine-readable names when adding or editing a menu.
 */
function menu_edit_menu_validate($form, &$form_state) {
  $item = $form_state['values'];
  if (preg_match('/[^a-z0-9-]/', $item['menu_name'])) {
    form_set_error('menu_name', t('The menu name may only consist of lowercase letters, numbers, and hyphens.'));
  }
  if (strlen($item['menu_name']) > MENU_MAX_MENU_NAME_LENGTH_UI) {
    form_set_error('menu_name', format_plural(MENU_MAX_MENU_NAME_LENGTH_UI, "The menu name can't be longer than 1 character.", "The menu name can't be longer than @count characters."));
  }
  if ($form['#insert']) {
    // We will add 'menu-' to the menu name to help avoid name-space conflicts.
    $item['menu_name'] = 'menu-'. $item['menu_name'];
    if (db_result(db_query("SELECT menu_name FROM {menu_custom} WHERE menu_name = '%s'", $item['menu_name'])) ||
      db_result(db_query_range("SELECT menu_name FROM {menu_links} WHERE menu_name = '%s'", $item['menu_name'], 0, 1))) {
      form_set_error('menu_name', t('The menu already exists.'));
    }
  }
}

If anyone could offer a little insight on how I need to apply this patch, I'd appreciate it!

Bug still exists in D6.16 - just got hit from it.

+++ modules/menu/menu.test	8 Dec 2009 09:57:28 -0000
@@ -140,8 +140,8 @@
+    $this->assertRaw(t('!name cannot be longer than %max characters but is currently %length characters long.', array('!name' => t('Menu name'), '%max' => MENU_MAX_MENU_NAME_LENGTH_UI, '%length' => drupal_strlen($menu_name))), t('Validation failed when menu name is too long.'));   ¶
+    ¶
     // Change the menu_name so it no longer exceeds the maximum length.

trailing whtiespace

Powered by Dreditor.

Patch in #39 must first be committed to D7, then the D6 port may be committed to D6. Thus the version number should be kept on 7.x for now.

Just to confirm - the patch for D6 in #40 works for latest version of D 6.17. When can we get this committed to core D6?

-backdrifting

Shouldn't we commit to D6 version first since D7 is still in production?

-backdrifting

#39: 292790-menu-validation-D7.patch queued for re-testing.

First we fix bugs in D7 than backporting. :)

Well test will fail here a reroll :)
+ removed trailing white space

subscribing

Subscribing.
Error encountered on a Drupal 6.16.

Something slightly confusing has happened here regarding the D7 fix.

Just to remind everyone, the patches in #39 and #52 have the same solution to menu.admin.inc but the code in menu.test for #52 was cleaned up to remove trailing whitespace.

I've just checked the souce code in D7 beta3 and it appears that the substance of the #39/#52 patches has been committed. The code in D7-beta3 menu.admin.inc is as expected from the patch, but it is odd that nothing has been announced on this thread about it being committed.

The code in menu.test is not exactly the same as either of the patches, because it has been reformatted with the array values on new lines, and the second parameter passed to $this->assertRaw has been removed.

So as I see it, the D7 code has been committed, and we can set this thread back to 6.x-dev and get the patch committed here too. BUT I want someone else to confirm what I've discovered above before I change the version.

It's been 2 years and 3 months since discovering the problem, but we WILL get this fix committed to D7 and D6, yes we will :-)

Jonathan

Looks like the D7 fix was part of #902644: Machine names are too hard to implement. Date types and menu names are not validated

Wow, what a thread #902644: Machine names are too hard to implement. Date types and menu names are not validated was! I've just read through it, 101 posts in 8 weeks.

Yes, this issue is now fixed in D7 so we can turn our attention to D6. The code change in the patch in #40 is what needs to be implemented, but that was created in 6.15 or 6.16. The file menu.admin.inc changed in 6.17 but has not changed since then, up to the latest 6.19 dev, so I've re-rolled that patch.

Please can some of you in the thread above test this patch, then we can hopefully finally get it committed in D6.

Jonathan

subscribing and will try patch.

[edit] and patch seems to be working nicely. thank you.

Yes, patch works fine for me, too. Thanx for the solution.

This problem pops up almost systematically with OG Menu
Perfect (after 2 years...)
Thanks

I tried to delete a menu item that I put at the same level as primary links et cetera. I wasn't able to delete that menu item without getting this message: "The menu name can't be longer than 27 characters". I found my solution to delete the menu item in the topic "How to delete a menu" http://drupal.org/node/245930. You have to click on the menu item and add the word "delete" at the end of the url.

The patch in #58 applies cleanly in D6.20 I am attaching the file again, however, to get it tested by the testing bot. I did not realise that if you add any -Dn suffix before the .patch then it will not be tested (even if that number matches the version of the issue).

Hmm... any ideas? the patch was against a file which has not changed since 2nd June 2010, as is still at this date in the latest 6.x dev of 16th Dec 2010. Is the format of the patch file wrong? The other patches above contain

RCS file: /cvs/drupal/drupal/modules/menu/menu.admin.inc,v
retrieving revision 1.82
diff -u -r1.82 menu.admin.inc

When is this getting somewhere near approved? I don't like applying patches to core.

The solution is decided on, and the coding is done and tested. But it needs a different kind of patch syntax to pass the automated tests. I don't the facility to create this, but someone else on this thread should be able to.

It is important enough to get fixed soon. So I gave it a try with my environment. May be just the Index statement was missing that I saw in other patches.

another attempt, now using git diff manually. I checked out dev so I apply the patch to that version, another possible mistake.

P.D. This fails as well. I think that the test only works for D7 development patches. The description here says that it is for HEAD, which is the main development version of Drupal 7.x at the moment. So it seems that D6 development is out of the picture for automatic testing and there is some explanation how to skip testing by letting the patch file end with -D6.patch.

I think this is the correct tag. The patch has been applied to the 7.x branch but is tested and reviewed and now need to be applied to the 6.x branch. Please leave it like this otherwise it never gets applied.

I think the patch test bot no longer works with any D6 patches: #961172: All D6 Core patches are failing

Thanks roball, yes that is it.
@null in #71, are you sure the status should be 'patch to be ported'. The patch has already been ported, there is no more coding to be done, it is correct for D6. It needs 'review' then when passed by test bot, it can go RTBC. I've not changed the status because I am quite willing to be wrong on this, just thought I'd ask.

That was my interpretation of the definition. Rereading it now, I must have been in a hurry. The patch is ready for 6.x so no porting is needed any more. I think you're right that it should be "RTBC"

#69: 292790_69_menu_name_validation.patch queued for re-testing.

subscribing

I have asked on #961172: All D6 Core patches are failing if they can help us get this patch to go green on the testbot. As we know, the code is fine just that automatic testing is not working.

subscribe

#961172: All D6 Core patches are failing might be fixed now. Or it will be shortly. Lets hope!

This is good news. Marking this 'reviewed and tested'.
Do we now have to find someone who can commit this?

Superb, now committed and pushed this one thanks.

Thank You! We've waited so long for this, but it has been a good experience for me to help it through.

Thank you to everyone who has contributed.

Jonathan

Whoow, just came back (after more than a year since #42, haha) to check this thread, what a hassle this has been for 4 lines of code :-D

Glad it got in though!