Closed (fixed)
Project:
Webform
Version:
7.x-3.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
9 May 2012 at 17:30 UTC
Updated:
26 May 2012 at 05:30 UTC
Jump to comment: Most recent file
Comments
Comment #1
quicksketchSounds good to me!
Comment #2
liam morlandPatch attached.
Comment #3
quicksketchI'm hesitant about this patch. While it makes sense to "set a default" instead of throwing a validation error for obvious oversights, I don't think that the "Listbox" option being automatically checked for users is necessarily an obvious configuration change that a user would expect. I was expecting a validation error instead, telling the user that option groups are only supported if the listbox option is enabled. A colleague of mine said it should perhaps change it automatically for you but display a warning message (while redirecting back to the component listing). I'm not sure whether that's more appropriate than a validation error or not, but I would like to avoid silently modifying the user's settings without giving them indication.
Comment #4
liam morlandI like the second suggestion better. A validation error forces the user to find the Listbox control and check it. If they are not familiar with the control they may be confused. Attached is a patch which includes setting a message to warn the user that a listbox will be used.
Comment #5
quicksketchThanks Liam. I cleaned up the patch approach quite a bit but kept the general idea. Instead of modifying webform.component.inc, I wanted to keep the code in select.inc so I tacked on the necessary code to our element validate handlers. I tried to use a separate handler to keep them all separate, but I couldn't ensure that _webform_edit_validate_options() handler would run first (thus converting options for us). So instead I just implemented it directly in the existing validation handlers. This has the upside of preventing us from needing to convert the options again manually, since they're already an array by the time we want to check them.
I tested and committed this patch to both branches. Let me know if any further changes look necessary to you.
Comment #6
quicksketchDoh, found a bug when not using options element. Looks like we do still have to convert the list to options when using the plain text area. Patch applies on top the last one.