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

The message (was lost by drupal by formatting case):
---
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.