WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 23 months ago

#32760 closed defect (bug) (fixed)

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

Reported by: designsimply Owned by: westonruter
Milestone: 4.3 Priority: normal
Severity: major Version: 4.3
Component: Customize Keywords: has-patch commit
Focuses: javascript Cc:

Description

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

Change History (31)

#1 @obenland
2 years ago

  • Milestone changed from Awaiting Review to 4.3
  • Severity changed from normal to major
  • Version set to trunk

#2 @celloexpressions
2 years ago

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.

#3 @westonruter
2 years ago

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.

#4 @westonruter
2 years ago

  • Focuses javascript added
  • Owner set to westonruter
  • Status changed from new to accepted

#5 @westonruter
2 years ago

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.

#6 @westonruter
2 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

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.

#7 follow-up: @designsimply
2 years ago

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)

#8 in reply to: ↑ 7 @westonruter
2 years ago

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?

#9 follow-up: @celloexpressions
2 years ago

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

#10 in reply to: ↑ 9 ; follow-up: @westonruter
2 years ago

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.

#11 in reply to: ↑ 10 ; follow-up: @tywayne
2 years ago

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+/, '') )

#12 in reply to: ↑ 11 @westonruter
2 years ago

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?

#13 @tywayne
2 years ago

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.

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


2 years ago

#15 follow-up: @celloexpressions
23 months ago

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.

#16 in reply to: ↑ 15 @westonruter
23 months ago

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.

#17 @westonruter
23 months ago

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…

#18 @westonruter
23 months ago

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.

This ticket was mentioned in Slack in #core by westonruter. View the logs.


23 months ago

This ticket was mentioned in Slack in #core-customize by ocean90. View the logs.


23 months ago

#21 @westonruter
23 months ago

  • Keywords needs-unit-tests added; needs-testing removed

#22 @westonruter
23 months ago

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.

#23 @westonruter
23 months ago

  • Keywords commit added; needs-unit-tests removed

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


23 months ago

#25 @westonruter
23 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

Note: See TracTickets for help on using tickets.