Prevent creating shortcut set with empty or duplicate label

Nice catch and good patch.

Looks like the test failure is because on the user account page, the textfield is being made always required, even when the corresponding radio button is not selected. To work correctly, it seems like it's unfortunately going to require a custom validation handler (which would check which radio button was chosen before throwing the error).

Why are you adding '#title' => t('Label') to the form? Doesn't seem related to this issue...? This might be required for screen-readers (I'm not 100% sure it is in this particular case, due to the order of the HTML), but for non-screenreaders the "New set" already serves as the joint label. So, if we need to add this at all, I think it should be with #title_display set to 'invisible' (see http://api.drupal.org/api/function/theme_form_element/7). It should also probably say 'Set name' rather than 'Label' to be consistent with the terminology used elsewhere.

Hmmm wouldn't it be nice if we had tests for this?

Patch does the job nicely.

But it is possible to create shortcut sets with the name of an already existing set. This makes working with those sets rather difficult. Can somebody also add a routine to check for existing names (or should I file a new issue - but this seems "almost related")?

Creating a new shortcut set in user/1/shortcuts is not so obvious I think. I was actually wondering what this textbox is supposed to do. Maybe the textbox could be put directly after the radiobutton and instead have "New set" as a default value? Again this is probably the wrong place to mention this ...

Thanks for including the duplicates in the issue.

Patch works fine if you try to create a duplicate under user/1/shortcuts but you can still create duplicates as an admin in admin/config/user-interface/shortcut/add-set - looks like it is not checked on that form at all.

Relates to sivaji's description I reproduced this bug and works for me.

Oops, I had really meant to review this earlier :)

1. This seems to mostly work well; however, it introduces a bug where editing an existing shortcut set does not allow you to submit the form without also renaming the set (it checks the name against itself).
2. There are some minor code style issues that need to be fixed (spelling errors, code comments not starting with a capital letter, missing @see blocks for the new validate functions)...
3. The shortcut_set_name_is_exists() function has a grammatical error in the function name (should get rid of the "is") and as an API function, belongs in the shortcut.module file rather than in the shortcut.admin.inc file.
4. We might consider making the wording of the error messages consistent with other places in Drupal that check for uniqueness (e.g., filter module, menu module), at least if it's the case that those are already consistent with each other?
5. As aspilicious suggests, it would be nice to have tests for this (although I don't think it's a critical thing to write a test for).

But the overall approach of the patch definitely looks solid to me. I'll see if I can reroll this a bit later with those fixes so we can get this in. Thanks!

Revised patch with the above issues fixed, plus some tests.

I also changed the internals of the API function to use the db_query_range() method for determining if a database record exists, since that is the recommended method used throughout core (although I'm noticing that in this case the database column does not have an index...)

Look OK?

It is OK. I reproduced again.

Reviewed this patch, and ran the tests locally. Code looks good and everything works as expected. Committed to CVS HEAD. Thanks!