There are two ways to reproduce this bug
1. Go to user/1/shortcuts
, choose new set and hit the "change set" button without giving any input in text field. This will create new shortcut set without any label.
2. Go to add shortcut admin page admin/config/user-interface/shortcut/add-set
, hit "create new set" button without giving any input in text field. This will create new shortcut set without any label.
I guess the expected behavior is to report form error, attached patch will fix this issue by adding title and required attribute to form array.
Comments
Comment #1
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedPossible related issue is #647084: Missing shortcut edit and delete operations on shortcuts admin, i didn't had time to go through the discussion when creating this issue.
Comment #2
aspilicious CreditAttribution: aspilicious commentedNice catch and good patch.
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commentedLooks 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.Comment #5
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedAttached patch adds custom check to validation handler.
To prevent FormAPI throwing error like
'' field is required.
with custom check#title
and#required
properties is needless.Comment #6
aspilicious CreditAttribution: aspilicious commentedHmmm wouldn't it be nice if we had tests for this?
Comment #7
Cibes CreditAttribution: Cibes commentedPatch 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 ...
Comment #8
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedAttached patch add duplicate label check to the original patch.
Comment #9
Cibes CreditAttribution: Cibes commentedThanks 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 inadmin/config/user-interface/shortcut/add-set
- looks like it is not checked on that form at all.Comment #10
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedNice that you noticed this. I had same thought today morning, I even wrote a patch to fix this.
Comment #11
asrobRelates to sivaji's description I reproduced this bug and works for me.
Comment #12
David_Rothstein CreditAttribution: David_Rothstein commentedOops, I had really meant to review this earlier :)
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!
Comment #13
David_Rothstein CreditAttribution: David_Rothstein commentedRevised 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?
Comment #14
asrobIt is OK. I reproduced again.
Comment #15
Dries CreditAttribution: Dries commentedReviewed this patch, and ran the tests locally. Code looks good and everything works as expected. Committed to CVS HEAD. Thanks!