The States API won't pick up the value of a select field with #multiple = TRUE option.

Steps to reproduce:
- Create two fields in a form with the following code:

  $form['dependee'] = array(
    '#type' => 'select',
    '#options' => array(
      'a' => 'Option A',
      'b' => 'Option B',
      'c' => 'Option C',
    ),
    '#multiple' => TRUE,
  );

  $form['dependent'] = array(
    '#type' => 'textfield',
    '#states' => array(
      'visible' => array(
        'select[name="dependee[]"]' => array('value' => array('a')),
      ),
    ),
  );

The dependent field will stay hidden regardless of the value of the dependee. This happens because the value of a multiple select field is an array, and States tries to compare it with the reference with a === operator, which returns always false.

The proposed solution is to add a handler for arrays in states.Dependent.comparisons. This works with ANDed values:

'select[name="dependee[]"]' => array('value' => array('a', 'b')),

and with ORs as well (following the syntax proposed in #735528: FAPI #states: Fix conditionals to allow OR and XOR constructions):

'select[name="dependee[]"]' => array(array('value' => array('a')), array('value' => array('c')))

Before

After

Files: 

Comments

peterpoe’s picture

FileSize
704 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to drop checkout database. View

Reference and values array should have the same length.

Status: Needs review » Needs work

The last submitted patch, states-multiple-select-1149078-1.patch, failed testing.

arcaneadam’s picture

Status: Needs work » Needs review
FileSize
974 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch states-multi-select-fix-1149078-3.patch. Unable to apply patch. See the log in the details link for more information. View

I've tweaked the patch above to make for nicer code along with adding comments. I think some discussion needs to be had about how to handle multi-selects though, because in the first patch it only returns true if the two arrays match exactly. Where as my patch will return true as long as the values given in #states are selected in the select box, even if there are additional values.

So for example if my form structure looks like this

 $form['test'] = array(
    '#type' => 'select',
    '#title' => t('Toggle'),
    '#multiple' => TRUE,
    '#options' => array('true' => 'True', 'false' => 'False'),
  );
  
  $form['works'] = array(
    '#title' => t('It works'),
    '#type' => 'textfield',
    '#states' => array(
      'visible' => array(
        ':input[name="test[]"]' => array('value' => array('true')),
      ),
    ),
  );

and in my select box i have "True' and 'False' both picked then my patch evaluates to true. I wonder if there doesn't need to be some sort of extra key in the state api to tell it to do exact or inclusive checking, though I think there is already another issue for that somewhere.

How it is right now I think this patch at least fixes a current problem and limitation with the states api.

arcaneadam’s picture

Also as a side note for anyone running into this issue and not wanting to patch core. You can sort of hack around this in a custom js using something similar to the following:

(function($) {
  Drupal.behaviors.optAdd = {
    attach: function() {
      var opt_add = {};
      opt_add.Dependent = {};
      opt_add.Dependent.comparisons = {
        'Array': function(reference, value) {
          //Make sure that value is an array, other wise we end up always evaling to true
          if(!( typeof (value) == 'object' && ( value instanceof Array))) {
            return false;
          }
          //We iterate through each of the values provided in the reference
          //and check that they all exist in the value array.
          //If even one doesn't then we return false. Otherwise return true.
          var arrayComplies = true;
          $.each(reference, function(key, val) {
            if($.inArray(val, value) < 0) {
              arrayComplies = false;
            }
          });
          return arrayComplies;
        },
      };
      var states = Drupal.states;
      $.extend(true, states, opt_add);
      Drupal.states = states;
    }
  }
})(jQuery);
pacufist’s picture

I faced this problem on Drupal 7.14.

After apply patch #3 all started work just perfect.

wuinfo’s picture

Status: Needs review » Needs work
wuinfo’s picture

Status: Needs work » Needs review

Patch applied if putting the patch under core folder. I will put the status to review and let bot test it first.

wuinfo’s picture

Issue tags: -FAPI #states

Status: Needs review » Needs work
Issue tags: +FAPI #states

The last submitted patch, states-multi-select-fix-1149078-3.patch, failed testing.

wuinfo’s picture

Status: Needs review » Needs work
FileSize
1.08 KB
PASSED: [[SimpleTest]]: [MySQL] 48,524 pass(es). View

I can reproduce the original issue in the latest 8.x with following setup:

New Drupal8.x setup with latest code with command: git pull
Built a small module with custom form and code in #3 comment

Attached patch applies to 8.x with same code as #3.

In Drupal 7.16, I can reproduce the same issue and the code in #3 is also working.

wuinfo’s picture

Status: Needs work » Needs review

change the status

Cottser’s picture

Thanks @wuinfo! We could move this even further by cleaning up the code comments a bit as per http://drupal.org/node/1354. At a glance, I can see the comments need a space after the //, need to be a complete sentence (end in a period), and should be wrapped at 80 characters (some wrap early).

wuinfo’s picture

Assigned: Unassigned » wuinfo

Code can be a little bit better, I will work on it.

wuinfo’s picture

Assigned: wuinfo » Unassigned
Status: Needs work » Needs review
FileSize
1.17 KB
964 bytes
PASSED: [[SimpleTest]]: [MySQL] 48,727 pass(es). View

@Cottser comment and code are both changed. Review needed.

wuinfo’s picture

+++ b/core/misc/states.jsundefined
@@ -75,6 +75,20 @@ states.Dependent.comparisons = {
+    $.each(reference, function(key, val) {
+      if ($.inArray(val, value) < 0) {
+        return false;
+      }
+    });

return false here just exit the each loop. So there is a bug in the code.

wuinfo’s picture

FileSize
1013 bytes
PASSED: [[SimpleTest]]: [MySQL] 48,810 pass(es). View
952 bytes

Here is a new patch which basically use #10's code and #14's comment.

wuinfo’s picture

FileSize
1.01 KB
PASSED: [[SimpleTest]]: [MySQL] 48,778 pass(es). View

Quit the .each loop as early as possible by "return false;", according to http://api.jquery.com/each/

Jelle_S’s picture

+++ b/core/misc/states.jsundefined
@@ -75,6 +75,22 @@ states.Dependent.comparisons = {
+      if ($.inArray(val, value) < 0) {
+        arrayComplies = false;
+        return false;

Maybe add a comment that this return false is to break the loop and not to actually return false. When I first read this patch I was thinking we could replace the return arrayComplies; with a return true; since we returned false in the loop anyways. (But then it hit me it isn't the actual return, but it's just to break the foreach).

wuinfo’s picture

FileSize
373 bytes
1.04 KB
PASSED: [[SimpleTest]]: [MySQL] 49,356 pass(es). View

Comment was added according #18

jlapp’s picture

I was able to reproduce this issue on Drupal 7.19 and manually applying the patch from #19 resolved it for me. It would be great to see this committed to 8.x and backported to 7.x.

nod_’s picture

Status: Needs review » Needs work
Issue tags: +ie8

use jQuery $.isArray() detecting arrays is something we don't want to deal with.
don't use $.each, make a filtered for loop
don't need $.inArray(); we can use .indexOf since we don't need to care about IE8 (patch for D7 will be different, that's fine).

Pre-emptively tagging for https://drupal.org/project/ie8

Antoine Vigneau’s picture

Hi,

I wake up this thread because I'm experimenting this issue on 7.21.

I want to have a #states selector matching all select element of a multi-value field. My field is an Entity Reference and the widget is a Multiple Selects List. But at the end, I normally could select all these elements using: #field-dependee-values .form-type-select select. I expect that the dependent reacts on all select elements of the dependee, in fact it reacts to nothing (always show).
The only things which works is #field-dependee-values .form-type-select select:first but it works only for the first select element.

Here is my #states declaration in a form_alter (it's in a node edit form):

  $form['field_dependent']['#states'] = array(
    'visible' => array(
      '#field-dependee-values .form-type-select select:first' => array( /* Here it selects only the first  tag, I would like to select all */
        array('value' => '1'),
        array('value' => '2'),
      ),
    ),
  );

The dependent must react to multiple values of the dependee, hence the multiple 'value' array.

I tried other kind of selector with the name too but the name attribute has this structure: name_field_dependee[und][0][target_id], so I tried select[name="name_field_dependee[][][]"], it doen't work. I applied the patch #19 too but the comparison was never detected as an Array.

peterpoe’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
997 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch states-multi-select-fix-1149078-22.patch. Unable to apply patch. See the log in the details link for more information. View

Applied _nod's review (use for instead of $.each and indexOf instead of $.inArray).

hass’s picture

RdeBoer’s picture

Would love to see this patch committed. This is quite a common use-case and has cause me a few head-aches and poor UX issues.

Oostie’s picture

Status: Needs review » Reviewed & tested by the community

Works for me, can this patch be committed to core?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 23: states-multi-select-fix-1149078-22.patch, failed testing.

Tobias Xy’s picture

Issue tags: +needs backport to D7
mgifford’s picture

Status: Needs work » Needs review
FileSize
990 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,600 pass(es). View

reroll....

vaza18’s picture

These patches were not working for me with multiselect selector when default value was not defined.

It became working only after adding this code (set oldValue = false instead of null) in defaultTrigger function:

...
defaultTrigger: function (event, valueFn) {
    var oldValue = valueFn.call(this.element);
    if (!oldValue) {
      oldValue = false;
    }
...
muschpusch’s picture

Is it possible to use / test this functionality without patching core for D7? I extended / overwrote the 'Dependent.comparisons' function like mentioned in #4 but it doesn't seem to work.

mgifford’s picture

@vaza18 can you roll a new patch?

ufku’s picture

For multiple selects, the correct approach is to initiate the state value as undefined to make sure strict equality fails when null is returned as a value.

Need to change:

          // Initialize the value of this state.
          this.values[selector][state.name] = null;

to:

          // Initialize the value of this state.
          this.values[selector][state.name] = undefined;

I can't provide a patch now sorry.

mgifford’s picture

FileSize
1.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,253 pass(es). View

Ok, I added that @ufku - we just need folks to test it now.

nod_’s picture

Status: Needs review » Needs work
Issue tags: +JavaScript, +Novice

The following line should be replaced with Array.isArray(value):


+     if (!(typeof(value) === 'object' && (value instanceof Array))) {

and we could use some fancy Array function for the other piece of code, like [].every() or something.

himanshupathak3’s picture

Assigned: Unassigned » himanshupathak3
Issue tags: +SrijanSprintNight
himanshupathak3’s picture

FileSize
1.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,704 pass(es). View
460 bytes

Added first requirement in code. Added patch and interdiff for it.
@nod_ the second requirement for function every is not supported by this way [12, 5, 8, 130, 44].every(elem => elem >= 10);, is there some other way we can use that ?

himanshupathak3’s picture

Assigned: himanshupathak3 » Unassigned
Status: Needs work » Needs review
DrCord’s picture

I applied patch #37, it did not fix multiple selects for me :( I did apply it to drupal 7.38 though...)

Kuntyi’s picture

Hi all,

I tried to fix this issue. I have added solution to add logic operator, you need to add array with operator to the end of the value array, look syntax:

$form['dependent'] = array(
'#type' => 'textfield',
'#states' => array(
'visible' => array(
'select[name="dependee[]"]' => array('value' => array('a', 'c', array('xor'))),
),
),
);

You can use next operators: and (default), or and xor.
Can someone test it?

Thanks.

Status: Needs review » Needs work

The last submitted patch, 40: 0001-Fixed-multiple-select-fields.patch, failed testing.

GoZ’s picture

I think we should focus on the #37 patch way to fix this issue.

@Kuntyi, your patch add a more complex and powerful solution, so may be we could talk about it in another issue. This current issue will fix the multiple select field issue, and your issue will add logic operator feature.

jrb’s picture

Status: Needs work » Needs review
FileSize
1.33 KB

In #37, I think the Array.isArray(value) comparison added per the suggestion in #35 should actually be !Array.isArray(value) (i.e. *not* an array). After making this change, it works for me.

I've attached an updated patch.

Status: Needs review » Needs work

The last submitted patch, 43: states_api_doesn_t_work-1149078-43.patch, failed testing.

jrb’s picture

Status: Needs work » Needs review
FileSize
1.33 KB

Rerolled #43 for latest core.

mgifford’s picture

Does this need the <h3>Why this should be an RC target</h3> info & RC phase evaluation table? Also there's the "rc target triage" tag.

Just trying to help this along.

legolasbo’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice +Contributed project blocker
Related issues: +#2635664: Make the hide/show of the other field work.

Removing the novice tag because I don't think there are any outstanding novice tasks remaining.

I've manually reviewed the patch. The only thing I can think of to improve this patch would be to return FALSE by default, Iterate over the values and return TRUE if they all exist. Besides that nitpick, the patch works like a charm and committing this patch would unblock #2635664: Make the hide/show of the other field work.. I therefor think that a change in logic should not hold back the resolution of this issue.

Landing this patch unblocks #2635664: Make the hide/show of the other field work., which is why I added the Contributed project blocker tag.

legolasbo’s picture

FileSize
31.31 KB
33.72 KB

To really take this home I've hidden some irrelevant files and made some before/after screenshots.

legolasbo’s picture

Issue summary: View changes
alexpott’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs work

This really makes me wish we had frontend testing.

Also this fails our eslint validation with the following errors.

core/misc/states.js
   95:34  error    Inconsistently quoted property `Array` found                quote-props
   95:34  error    Properties shouldn't be quoted as all quotes are redundant  quote-props
   95:34  error    Inconsistently quoted property `Number` found               quote-props
  163:47  error    Unexpected use of undefined                                 no-undefined
alexpott’s picture

Version: 7.x-dev » 8.0.x-dev

Didn't mean to set to 7.x

legolasbo’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
0 bytes
1.27 KB

Thanks for pointing to the eslint documentation, I updated my phpstorm config to also include eslint for future patches.

Attached patch fixes the eslint errors. Even though the changes should not change any behaviour I manually tested it again to make sure.

Back to RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs manual testing
+++ b/core/misc/states.js
@@ -145,9 +159,6 @@
-          // Initialize the value of this state.
-          this.values[selector][state.name] = null;
-

Given we don't have frontend testing I want to see some evidence of visual regression testing because of this. This was added in #735528: FAPI #states: Fix conditionals to allow OR and XOR constructions and it was changed from being self.values[selector][state.pristine] = undefined; so I think we might have broken the OR or XOR logic...

legolasbo’s picture

Status: Needs work » Needs review
FileSize
854 bytes
495 bytes
945 bytes
+++ b/core/misc/states.js
@@ -146,7 +160,7 @@
           // Initialize the value of this state.
-          this.values[selector][state.name] = null;
+          this.values[selector][state.name] = undefined;

After reading trough #735528: FAPI #states: Fix conditionals to allow OR and XOR constructions I came to the conclusion that this change in #45 was probably unneeded and not relevant for the functionality. This means that the eslint error was introduced without any reason and the offending line should not be removed, but reverted to it's original state.

I put my theory to the (manual) test and concluded that the change to the line in question can indeed be reverted without breaking the functionality added in #45. Attached you'll find a new patch which reverts the line in question back to the way it is in HEAD.

neoaptt’s picture

There is an issue if you pass integer values to be compared against each other.
if you map the integer values to strings then you get the correct results.

Array: function (reference, value) {
            // Make sure value is an array.
            if (!Array.isArray(value)) {
                return false;
            }
            // Convert all comparisons to strings for indexOf to work with integers comparing to strings
            reference = reference.map(String);
            value = value.map(String);
            // We iterate through each value provided in the reference. If all of them
            // exist in value array, we return true. Otherwise return false.
            for (var key in reference) {
                if (reference.hasOwnProperty(key) && value.indexOf(reference[key]) === -1) {
                    return false;
                }
            }
            return true;
        },

Because the default value of multiselects is null, the reevaluate passes right over it. Thinking that null is the same value as null.

I tried to come up with a solution but the best I can do is pass a default value in. That isn't a fix be any means.

EDIT: Fixed, just skip the null checks.

update: function (selector, state, value) {
            // Only act when the 'new' value is actually new.
            if (value !== this.values[selector][state.name] || $(selector).prop('multiple')) {
                this.values[selector][state.name] = value;
                this.reevaluate();
            }
        },
legolasbo’s picture

neoappt, could you roll a patch of your solution?

mtift’s picture

Status: Needs review » Needs work

Hmmm, none of the above solutions work for me. Although, I was having a problem just with checkbox example, such as the one mentioned on https://api.drupal.org/api/drupal/core%21includes%21common.inc/function/...

$form['toggle_me'] = array(
  '#type' => 'checkbox',
  '#title' => t('Tick this box to type'),
);
$form['settings'] = array(
  '#type' => 'textfield',
  '#states' => array(
    // Only show this field when the 'toggle_me' checkbox is enabled.
    'visible' => array(
      ':input[name="toggle_me"]' => array('checked' => TRUE),
    ),
  ),
);

So maybe this was the wrong place to post. Sorry.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

Issue tags: +Needs tests

We now have a javascript test base, so adding the 'needs tests' tag.

sukanya.ramakrishnan’s picture

Has a fix been applied to core for this issue?

Thanks,
Sukanya

deranga’s picture

Version: 8.1.x-dev » 7.x-dev

Applied patch from comment #52 to states.js on Drupal 7.50 and this works as expected now for multi-valued select field.

Will this patch ever be rolled into an update of Drupal 7?

deranga’s picture

Version: 7.x-dev » 8.1.x-dev

Apologies accidentally changed version number for issue.

legolasbo’s picture

@deranga,

I believe the fix will be backported to D7 after it get's committed to 8.1.x-dev

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

sukanya.ramakrishnan’s picture

Can this issue be taken up again? i tried applying the patch, but it doesnt work for me. I think i am going wrong with the syntax for the states property. Can someone please put down the correct syntax for a multi select field so that i can try again. Basically i need one field to be visible based on multiple values chosen in a select field (or condition)

Thanks,
Sukanya

GoZ’s picture

Re-roll patch with updates from neoaptt in #55.
I also join module to help test (rename file to .tar.gz instead of .tar_.gz).

Manual tests look fine with #55 updates.

Still missing javascript test base as suggested by jhedstrom in #59.

GoZ’s picture

Status: Needs work » Needs review
GoZ’s picture

Assigned: Unassigned » GoZ
Status: Needs review » Needs work

I'm working on tests.

There is some updates to make to #54.

+++ b/core/misc/states.js
@@ -201,7 +218,7 @@
+      if (value !== this.values[selector][state.name] || $(selector).prop('multiple')) {

Multiple property should not be used to defined null default value. Default null value is an issue for #multiple property but also #size property.
Both are specific to select list but we should find another solution more generic.

I suggest to use $this->reevaluate() at the end of states.Dependent.prototype.initializeDependee, which force analyze states conditions and apply events.

GoZ’s picture

Assigned: GoZ » Unassigned
Status: Needs work » Needs review
FileSize
1.84 KB
848 bytes

Finally, i'm not sure this issue is the place to add tests. Another issue in progress is covering #states tests #2702233: Add Javascript tests for Form API's #states: require, visible, invisible so we should add #multiple tests there.

Following patch solve default null issue for both #multiple and #size attribute when i check in my browser, but mink seems to not be OK with that. Very strange, may be we should investigate in specific issue.

sukanya.ramakrishnan’s picture

Was finally able to get the patch working. Manual testing is fine for me.

Thanks
Sukanya

sukanya.ramakrishnan’s picture

Issue summary: View changes
droplet’s picture

  1. +++ b/core/misc/states.js
    @@ -103,6 +103,23 @@
    +      reference = reference.map(String);
    

    The next function returns when the condition is met. We need not loop over each item.

  2. +++ b/core/misc/states.js
    @@ -103,6 +103,23 @@
    +      for (var key in reference) {
    

    Considered Array.some

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

leisurman’s picture

Ive tried patch 52 and 69, but its still not working

Pavan B S’s picture

+++ b/core/misc/states.js
@@ -103,6 +103,23 @@
+      // Convert all comparisons to strings for indexOf to work with integers comparing to strings

Applied the #69 patch manually and fixed the coding standard error "Line is exceeding 80 characters".

Status: Needs review » Needs work

The last submitted patch, 75: states_api_doesn_t_work-1149078-75.patch, failed testing.

sukanya.ramakrishnan’s picture

@leisurman, I found that i had to clear the browser cache in addition to drush cr to get this working.. weird, but worth giving a try!

Thanks,
Sukanya

leisurman’s picture

@sukanya.ramakrishnan Which patch did you use #75. I have not tried that one.

leisurman’s picture

I cleared browser and drupal cache. Patch #75 did not work for me. I get this warning. warning php Warning: Invalid argument supplied for foreach() in conditional_fields_states_handler_select_multiple() (line 29 of /var/www/html/d818/modules/conditional_fields/conditional_fields.states.

I am using hook form alter, But I also tried the Conditional fields module, this is its issue:
https://www.drupal.org/node/2857718
This module works in Drupal 7 when the dependee field is a multiple select

sukanya.ramakrishnan’s picture

@leisurman, I used the one in 69. I did not try any other module. Are u sure your syntax is correct? i had issues with my syntax and after figuring out the correct syntax, i updated the description on this issue to show the correct one (The description was missing something before, i cant recollect). Please verify your syntax against the example in the description of this issue.

jhedstrom’s picture

Dinesh18’s picture

I tried to apply patch mentioned in #75 and it is getting applied properly with 1 white space-errors.
Do we need reroll ?

Dinesh18’s picture

FileSize
7.78 KB

here is the gitbash screenshot

droplet’s picture

Issue tags: -ie8, -Needs manual testing

It's better to fix #72 & #75 at the same time.

Jo Fitzgerald’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.66 KB

Re-rolled.

@droplet Can you clarify what you are requesting in #72, please?

Status: Needs review » Needs work

The last submitted patch, 85: 1149078-85.patch, failed testing. View results