Use case
--------
Create a select form element of with required set to true, When the form element is rendered, it has option
"- select -" set as default.
This is set so as to avoid form getting submitted with the first select option as default.
below is sample code and screen grab to demonstrate the error
function test_form($form, &$form_state) {
$form = array();
$form['default_select_element'] = array(
'#type' => 'select',
'#title' => t('test select'),
'#required' => TRUE,
'#options' => array('red', 'blue' => 'blue', 'green' => 'green'),
'#default_value' => NULL,
);
$form['select_radios'] = array(
'#type' => 'radios',
'#title' => t('On & Off radios'),
'#required' => TRUE,
'#options' => array('on' => 'on', 'off' => 'off'),
'#default_value' => NULL,
);
$form['optional_select_element'] = array(
'#type' => 'select',
'#title' => t('Select element dependent on states'),
'#options' => array('Yes' => 'yes', 'No' => 'No'),
'#states' => array(
'visible' => array(
':input[name="select_radios"]' => array('value' => 'on'),
),
'required' => array(
':input[name="select_radios"]' => array('value' => 'on'),
),
),
'#default_value' => NULL,
);
return $form;
}
Having '-select-' option for form elements controlled by '#states' is very important, as it stops stops from form getting submitted with the first top option selected by default.
Patch
Review
| Comment | File | Size | Author |
|---|---|---|---|
| #23 | select-handling.png | 8.31 KB | David_Rothstein |
| #19 | form-1426646-13-before.png | 27.03 KB | andymartha |
| #19 | form-1426646-13-after.png | 27.42 KB | andymartha |
| #13 | form-1426646-13.patch | 848 bytes | Ravi.J |
| #8 | form-1426646-8.patch | 914 bytes | xjm |
Comments
Comment #1
Ravi.J commentedPatch attached
Screen grab of form elements now work correctly with patch applied.
Comment #2
Ravi.J commentedComment #3
marcingy commentedThis needs to be fixed in d8 first otherwise looks good. A test would be nice but I don't think we can have one as it is ajax.
Comment #4
marcingy commentedComment #5
Ravi.J commentedI will roll out a D8 patch in the evening today.
As the patch is very small and straight forward, should be simple to go into D7 as well.
Comment #6
xjmYeah, there's no way to add a test this without #237566: Automated JavaScript unit testing framework.
Comment #7
xjmThis is normal based on http://drupal.org/node/45111.
Comment #8
xjmRerolled with some cleanup and a better comment.
We'll want to manually test this with the #states test module:
http://drupal.org/sandbox/rfay/1269964
Comment #10
nod_tagging
Comment #11
stborchertDuring the core sprint on DrupalCon Munich I've installed a new site with a fresh checkout of Drupal 8 and installed _#states exercise_.
Test result: select elements controlled by statsAPI do not lose the "-select-" option anymore -> RTBC
Comment #12
catchThis comment looks a bit odd to me:
Is that last fragment even needed?
Comment #13
Ravi.J commentedRerolled patch with cleaned up comments.
Will be great to see this committed.
Comment #14
Ravi.J commentedRerolled patch with cleaned up comments.
Will be great to see this committed.
Comment #15
Ravi.J commentedComment #16
marcingy commentedYou can't rtbc your own patches
Comment #17
Ravi.J commentedI have had this patch for D7 here since February. Path was already RTBC'd before and the trivial nature of the patch was hoping it would be committed.
Comment #18
yesct commented#13: form-1426646-13.patch queued for re-testing.
Comment #19
andymartha commentedI can confirm that after installing Drupal 8.x-dev from March 6th, 2013, the drupal form code put in the original example yielded one select box with a default blank option, but the one that used '#states' did not have that option.
After applying the form-1426646-13.patch from #13 by Ravi.J, the form example module yielded both consistant default blank options. See screenshots.
Is everyone ok with the code comments and formatting?
Comment #20
yesct commented(nothing in the following to hold this up. Just a note that dreditor makes it super easy to embed images in issue comments, so dreditor is highly recommended for that and lots of other reasons. Also, taking screenshots of drop downs.. it is sometimes nice to get the expanded drop down, using Jing on a mac, that is possible by using the key combination: command-shift-1. See: http://drupal.org/node/1859584)
coding style looks good.
Comment #21
webchickThis looks like a nice improvement, thanks. Not really possible to provide tests for this, I don't think, since it's JS-related.
Committed and pushed to 8.x. Moving down to 7.x.
Comment #22
attiks commentedThere should be tests in testswarm, but possibly not covering all cases, see also the FAT project
Comment #23
David_Rothstein commentedI don't understand this patch. Why would we want to make Drupal use terminology for required fields even when the field isn't required?
For example, with the code example shown at the top of this issue but modified slightly to remove the 'visible' part I now get this:
That's inconsistent; there are two fields both showing "- Select -" even though only one of them actually requires you to select something. Normally, Drupal would use "- None -" for the second one.
I am not sure there is any bug to fix here in the first place, actually - maybe just a documentation improvement is needed? If you want the behavior described at the top of this issue, you should be able to use #empty_option and/or #empty_value; that's what they're there for. I don't think Drupal core should try to guess the intention of someone who is using #states.
Comment #24
Ravi.J commented@David_Rothstein it is indeed a bug in core . The issue is that both the elements are required. One of them is set to required with form #states, while other is set to required using #required attribute.
At the moment their behaviours are not consistant.
@webchick , so did my patch get committed ? Wow, delighted, first core patch that got accepted even though its very small.
Comment #25
Ravi.J commentedWould be great if someone can review and RTBC the path so that the fix can be committed to D7 too
Comment #26
MichaelLund commentedNeed to change to so that "Select element depending to states" drop-down, is either - Select - or - None - depending on whether the field is required or not.
comment #1 it is required and says - Select as it should
comment #23 its not required and should say - None - instead of select
Comment #39
quietone commentedThis was commited to Drupal 8 in March 2013. It was reopened in #23 but the testing there did not use the code from the IS so the results did not illustrate the fix. I tested this on Drupal 9.3.x, standard install and the patch is working as intended.
I also tested on Drupal 7 and this does not to be ported. Therefor changing version.
If this is incorrect reopen the issue, by setting the status to 'Active', and add a comment explaining what still needs to be done.
Thanks!