Download & Extend
Issues

Prevent creating shortcut set with empty or duplicate label

Posted by sivaji on May 16, 2010 at 5:15pm
Project:Drupal core
Version:7.x-dev
Component:shortcut.module
Category:bug report
Priority:normal
Assigned:Unassigned
Status:closed (fixed)
Issue tags:Novice, Quick fix

Issue Summary

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.

AttachmentSizeStatusTest resultOperations
shortcut_set_user_1.png21 KBIgnored: Check issue status.NoneNone
shortcut_set_admin.png23.01 KBIgnored: Check issue status.NoneNone
shortcut_empty_label.patch1.09 KBIdleFAILED: [[SimpleTest]]: [MySQL] 20,339 pass(es), 1 fail(s), and 0 exception(es).View details
Login or register to post comments

Comments

#1

Posted by sivaji on May 16, 2010 at 5:24pm
Status:active» needs review

Possible 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.

#2

Posted by aspilicious on May 16, 2010 at 6:17pm

Nice catch and good patch.

#3

Posted by System Message on May 16, 2010 at 6:30pm
Status:needs review» needs work

The last submitted patch, shortcut_empty_label.patch, failed testing.

#4

Posted by David_Rothstein on May 16, 2010 at 11:51pm

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.

#5

Posted by sivaji on May 18, 2010 at 12:48pm
Status:needs work» needs review

Attached patch adds custom check to validation handler.

Why are you adding '#title' => t('Label') to the form?

To prevent FormAPI throwing error like '' field is required. with custom check #title and #required properties is needless.

AttachmentSizeStatusTest resultOperations
800366_shortcut_set_name_5.patch1.2 KBIdlePASSED: [[SimpleTest]]: [MySQL] 20,361 pass(es).View details

#6

Posted by aspilicious on May 18, 2010 at 2:45pm

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

#7

Posted by Cibes on May 19, 2010 at 10:24am

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 ...

#8

Posted by sivaji on May 20, 2010 at 7:46am
Title:Prevent creating shortcut set with empty label» Prevent creating shortcut set with empty or duplicate label

Attached patch add duplicate label check to the original patch.

AttachmentSizeStatusTest resultOperations
800366_shortcut_set_name_8.patch1.51 KBIdlePASSED: [[SimpleTest]]: [MySQL] 20,421 pass(es).View details

#9

Posted by Cibes on May 21, 2010 at 10:11am
Status:needs review» needs work

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.

#10

Posted by sivaji on May 21, 2010 at 5:57pm
Status:needs work» needs review

Nice that you noticed this. I had same thought today morning, I even wrote a patch to fix this.

AttachmentSizeStatusTest resultOperations
800366_shortcut_set_name_10.patch3.22 KBIdlePASSED: [[SimpleTest]]: [MySQL] 20,429 pass(es).View details

#11

Posted by asrob on June 29, 2010 at 8:39pm
Status:needs review» reviewed & tested by the community

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

#12

Posted by David_Rothstein on June 30, 2010 at 3:06am
Status:reviewed & tested by the community» needs work

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!

#13

Posted by David_Rothstein on June 30, 2010 at 4:48am
Status:needs work» needs review

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?

AttachmentSizeStatusTest resultOperations
800366_shortcut_set_name_13.patch7.13 KBIdleFAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 800366_shortcut_set_name_13.patch.View details

#14

Posted by asrob on June 30, 2010 at 5:56am
Status:needs review» reviewed & tested by the community

It is OK. I reproduced again.

#15

Posted by Dries on June 30, 2010 at 3:13pm
Status:reviewed & tested by the community» fixed

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

#16

Posted by System Message on July 14, 2010 at 3:20pm
Status:fixed» closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

nobody click here