Customizer Menus: cannot save a menu with the same name as an existing menu

[designsimply]

Steps to reproduce:

1. Go to Customize > Menus on a WordPress blog running at least r32806 (preferably the latest though)
2. If there are not already existing menus, create and save at least one menu.
3. Create a 2nd menu and name it exactly the same as one of the existing menus.
4. Add some items to the 2nd menu that are different than the first so you can distinguish them after save.
5. Save changes.
6. Go back to the main menus panel and re-open the menu you just tried to create.

Result: the new menu fails silently and doesn't get saved if it has the same name as an existing menu.

Screencast: ​40s no audio
^{Seen at ​http://whatisinfinity.com/wp-admin/customize.php?url=http%3A%2F%2Fwhatisinfinity.com%2Fwp-admin%2F with WordPress 4.3-alpha-32909 + 32678.2.diff using Firefox 38.0.5 on Mac OSX 10.10.3.}

It should probably warn you against creating a menu with the same name and not let you continue until the name is unique.

Found in user testing. See ​https://make.wordpress.org/design/2015/05/27/customizer-menus-user-testing/#comment-24599

[celloexpressions]

This was previously handled by sending back an error from the ajax call, where this is checked, before creating the menu UI. Guess we need to do that all in JS now.

[westonruter]

We could handle this via a validate method on the nav_menu setting, whereby if a user tries to update a nav_menu setting to have the same name as something else, it could automatically get (2) appended to the name, similarly to how post slugs get made unique.

[westonruter]

For reference, currently in Core, the admin page will just block the creation of a new menu that has the same name as an existing menu: https://core.trac.wordpress.org/browser/tags/4.2.2/src/wp-includes/nav-menu.php#L227

This works OK for the admin page because the menu has to be created up-front before adding menu items. In the Customizer, however, the menu gets “staged” and items are added to it before everything gets created/persisted in the database at Save & Publish. The Customizer doesn't have a good pattern in place for doing server-side validation that blocks a save from happening, so as noted above, I think we need to gracefully handle conflicts by amending the name with a number upon saving, just as the post slug duplicates are handled.

[westonruter]

​https://github.com/xwp/wordpress-develop/pull/94

[westonruter]

OK, 32760.diff​ fixes the issue:

• When saving a nav menu, the name is checked for conflicts with existing menus and a number is appended. The conflict-resolved name is used in the UI when the Customizer finishes saving.
• Empty names are prevented by forcing the value "(unnamed)" to be used instead.
• Fixed an issue where setting save() errors weren't resulting in their errors short-circuiting the logic flow.

@designsimply: Please test.

[designsimply]

Tested 32760.diff, LGTM. ​53s
^{(tested using 32760.diff + WP 4.3-alpha-32280-src using Firefox 38.0.5 on Mac OSX 10.10.3)}

Aside: instead of using "(unnamed)" as a default if no name is entered, what about blocking people from continuing unless they enter a name? Similar to what happens when you tried to leave the link text field blank: ​14s
^{(not tied to it, thinking about consistency though)}

[westonruter]

Replying to designsimply:

Aside: instead of using "(unnamed)" as a default if no name is entered, what about blocking people from continuing unless they enter a name? Similar to what happens when you tried to leave the link text field blank: ​14s
^{(not tied to it, thinking about consistency though)}

Good idea. So how about if the keyboard focus is placed on the name input if they try to submit without entering one?

[celloexpressions]

Yeah, and possibly also add a red outline like we have if you try to add a link without a URL.

[westonruter]

Additional changes: ​https://github.com/xwp/wordpress-develop/compare/46d3a5e...a2a24f0?w=1

[westonruter]

Replying to celloexpressions:

Yeah, and possibly also add a red outline like we have if you try to add a link without a URL.

& @designsimply: OK, please check out 32760.2.diff​. This adds the red outline and adds focus to a menu name input that is empty when you try to create a menu. I've also changed the logic for when the unnamed name is supplied so that you can now actually delete a nav menu name and it won't all of a sudden inject "(unnamed)" into the input. Now the "(unnamed)" name comes when the setting is saved on the server, in the same way that name conflicts are resolved.

[tywayne]

Replying to celloexpressions:

Yeah, and possibly also add a red outline like we have if you try to add a link without a URL.

There is a similar check in nav-menu.js, but it additionally checks to be sure the name isn't just blank spaces. Maybe consider adding that check here as well? Something like if ( !name || !name.replace(/\s+/, '') )

[westonruter]

Additional change: ​https://github.com/xwp/wordpress-develop/commit/30081684a66a8226cd8de5b7838a254f0ef02603

[westonruter]

Replying to tywayne:

Replying to celloexpressions:

Yeah, and possibly also add a red outline like we have if you try to add a link without a URL.

There is a similar check in nav-menu.js, but it additionally checks to be sure the name isn't just blank spaces. Maybe consider adding that check here as well? Something like if ( !name || !name.replace(/\s+/, '') )

Great point! I've addressed this now in 32760.3.diff​.

Any other concerns before this gets committed?

[tywayne]

I may not have explained my suggestion very well - I was thinking that if we're marking an empty value as invalid, we should also mark it invalid if the value only contains spaces (which would actually produce the same results in the UI as an empty value).

The updated patch does work in that it trims spaces and replaces with "(unnamed)", but did not produce the behavior I was expecting.

I realize that isn't exactly related to this specific ticket though and would be happy to just create a new ticket/patch to suggest it, as it could also be applied to the check for value in Custom Link text.

[celloexpressions]

Testing the patch, looks like I can create an unnamed menu with just spaces and successfully add items and set it to a location then save, but when refreshing the Customizer it's gone.

I think we should just skip the unnamed stuff and reject empty or spaces-only menus before they're even created, as @tywayne suggested. That would be the simplest approach and should avoid confusion for the user.

[westonruter]

Replying to celloexpressions:

Testing the patch, looks like I can create an unnamed menu with just spaces and successfully add items and set it to a location then save, but when refreshing the Customizer it's gone.

I think we should just skip the unnamed stuff and reject empty or spaces-only menus before they're even created, as @tywayne suggested. That would be the simplest approach and should avoid confusion for the user.

The menu goes away? It should be retained, but just with a conflict-resolved name and with empty names being replaced with "(unnamed)". There's also the possibility that a user could clear out the name input field after they first add the menu, so that needs to be taken into account as well.

[westonruter]

I'm able to reproduce the error where the menu doesn't save. I can see the Ajax response includes an empty_term_name error. Looking into it…

[westonruter]

Additional change: ​https://github.com/xwp/wordpress-develop/commit/8b3615aab81ff4115d233ac48d90a63f7d58f5b5

[westonruter]

There we go. Fixed in 32760.4.diff​. This now adds an up-front check of the trimmed name is an empty string, and if so, it supplies the "unnamed" name. The wp_insert_term() function returns the empty_term_name error when the trimmed name is empty, so this prevents that from happening.

If anyone can do a quick validation of this, I'll then commit.

[westonruter]

Additional change: ​https://github.com/xwp/wordpress-develop/commit/1d8e20bb58a278a6568e48dcd57cdba5d4698235

[westonruter]

Additional changes: ​https://github.com/xwp/wordpress-develop/compare/d0ab22ac973906b60c9bd760a83e26bde449c17b...e06bf93b08aac64bb342761db47a751fbd55696f

[westonruter]

In 32760.6.diff​:

• Move logic for checking for empty name to eliminate double checking.
• Send back null as saved_value when deleted.
• Add unit tests for saved_value and conflict-resolved menu name.

[westonruter]

In 33071:

Customizer: Fix saving menus with empty names or names that are already used.

Adds validation for initially-supplied nav menu name, blocking empty names from being supplied. If later an empty name is supplied and the nav menu is saved, the name "(unnamed)" will be supplied instead and supplied back to the client. If a name is supplied for the menu which is currently used by another menu, then the name conflict is resolved by adding a numerical counter similar to how post_name conflicts are resolved. Includes unit tests.

Fixes #32760.