The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- Status changed to Needs review
over 1 year ago 12:35pm 23 March 2023 - 🇮🇳India gauravvvv Delhi, India
I have attached patch for 10.1.x. none of the above patch is applying to 10.1.x. So not attaching interdiff. please review
- 🇺🇦Ukraine ysamoylenko
Confirming that https://www.drupal.org/project/drupal/issues/1149078#comment-14271390 🐛 States API doesn't work with multiple select fields Needs review works correctly with empty values on 9.5.3.
- Status changed to Needs work
over 1 year ago 7:10pm 7 April 2023 - 🇫🇷France nod_ Lille
I don't have a good idea about what should be the expected behavior, and I don't have the time to get into it this weekend. One the other hand, one small review:
-
+++ b/core/misc/states.js @@ -159,6 +159,24 @@ + Object.entries(reference).forEach(([key, referenceValue]) => {
key is never used,
Object.values()
would be more appropriate than entries. -
+++ b/core/misc/states.js @@ -159,6 +159,24 @@ + if (value.indexOf(referenceValue) === -1) { + return false; + } + }); + return true;
This could be simplified by using
includes()
Also not sure what that loop is doing, returing true or false inside the foreach does not do anything. I see #142 was using a for loop, in that case returning true/false would make sense. Not with forEach().
I would definitely expect some tests cases here just to make sure we agree on the behavior.
-
- Status changed to Needs review
over 1 year ago 7:21pm 9 April 2023 - Status changed to Needs work
over 1 year ago 10:13pm 9 April 2023 The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- Status changed to Needs review
over 1 year ago 2:29am 10 April 2023 - 🇮🇳India gauravvvv Delhi, India
Addressed #149, attached interdiff for same. please review
- 🇮🇳India gauravvvv Delhi, India
Wrong patch uploaded in #153. Updated the patch in #154.
- 🇮🇳India saurabh-2k17
- First commit to issue fork.
- @vladimiraus opened merge request.
- 🇳🇴Norway neslee canil pinto India
@VladimirAus Object.values will give referenceValue undefined, inside if we use Object.entries it will work fine with key, value pair
- last update
over 1 year ago 29,283 pass - last update
over 1 year ago 29,366 pass - last update
over 1 year ago 29,374 pass - 🇫🇷France arnaud-brugnon
Improve patch based on #160 comments
By the way, it works fine for me.
- Status changed to Needs work
over 1 year ago 8:41pm 6 May 2023 - First commit to issue fork.
- last update
over 1 year ago 29,420 pass - Status changed to Needs review
over 1 year ago 9:33am 8 June 2023 - 🇷🇴Romania vasike Ramnicu Valcea
Updated the MR - trying to add the tests from #132
So:
- Add tests
- Change compare - The arrays values should match - it should work also for empty arrays.
- Fix a test ... i think the OR check there was not right if value2 OR value3 doesn't mean if should be displayed when both value2 and value3 selected.
For OR case we should something if (value2 and value3) OR (value7 and value32)Note: i only used tests to check the "solution" ... i didn't test manually.
- Status changed to Needs work
over 1 year ago 5:46pm 14 June 2023 - 🇺🇸United States dww
Thanks, great progress!
About out the door for a few AFK meetings, but I'll try to look more closely when I'm back. Super quick skim finds some nits with the tests (wrong comments, etc). Also, the other #states tests in these files are now starting to do:
- Setup initial state + validate
- Trigger the change
- Check all the changes
- Revert the trigger change
- Re-verify the initial state is back
The test in the MR is currently stopping at step 3 (like many, but not all of the other protected helpers)...
- last update
over 1 year ago Custom Commands Failed - 🇺🇸United States dww
Ugh, that's super annoying. 😢 I worked on this locally. To my great surprise, 2 of the "initial state" assertions are failing after I remove the values in the multi-select trigger:
$this->assertTrue($item_visible_no_value->isVisible());
$this->assertFalse($textfield_visible_value2_and_value3->isVisible());
If I comment those out, the test passes. If either are still present, the test fails. However, when I inspect the HTML output of the failing page, I see it looks like it's working as expected:
The multi-select indeed has no values, and the only thing visible in the section of tested elements is what
$item_visible_no_value
points to. The rest of the test is working fine. Manually testing the multi-select trigger from this HTML result page and all the JS seems to be working as expected, too.I'm not sure this is actually a bug in the states code -- it could just be a bug in the test. But I'm out of time for today to keep digging.
Sorry,
-Derek - last update
over 1 year ago 29,428 pass, 1 fail - last update
over 1 year ago 29,429 pass - last update
over 1 year ago 29,429 pass - 🇺🇸United States dww
Grr, phpcs didn't like how I commented out the failing assertions. 😢
I fixed that and pushed a commit with the failing assertions included. That run should fail.
Pushed another where I properly commented out.
core/scripts/dev/commit-code-check.sh
is now passing locally. 😅Also rebased to the latest 10.1.x branch. We probably need to change the MR to point to 11.x for now. However, I just verified that the MR plain diff applies cleanly to the 11.x branch, too (as expected), so I'm not going to mess with trying to get the MR target branch changed (which I don't have permissions to do).
- 🇷🇴Romania vasike Ramnicu Valcea
i confirm that the js is working properly ... but it seems the
$trigger->setValue([]);
doesn't work properly.So what should we do ... find another way to "deselect" the multiple selection?
What about the chasing this "odd case" in a separate issue.
And try to commit the "current solution"? - 🇺🇸United States dww
Yeah, I hate holding up a fix in the quest for "perfect" tests. Definitely open to moving the last bits to a follow-up...
However, in more closely inspecting the HTML output of the tested page, the initial state's markup for the
item
looks like this:<div data-drupal-states="{"visible":{"select[name=\u0022multiple_select_trigger[]\u0022]":{"value":[]}}}" id="edit-item-visible-when-multiple-select-trigger-has-no-value" class="js-form-item form-item js-form-type-item form-item-item-visible-when-multiple-select-trigger-has-no-value js-form-item-item-visible-when-multiple-select-trigger-has-no-value"> <label for="edit-item-visible-when-multiple-select-trigger-has-no-value">Item visible when multiple select trigger has no value</label> </div>
After the test fails, the final output page has this for the element:
<div data-drupal-states="{"visible":{"select[name=\u0022multiple_select_trigger[]\u0022]":{"value":[]}}}" id="edit-item-visible-when-multiple-select-trigger-has-no-value" class="js-form-item form-item js-form-type-item form-item-item-visible-when-multiple-select-trigger-has-no-value js-form-item-item-visible-when-multiple-select-trigger-has-no-value" style=""> <label for="edit-item-visible-when-multiple-select-trigger-has-no-value">Item visible when multiple select trigger has no value</label> </div>
The only difference is that the one that's failing the test assertion includes this:
style=""
. That shouldn't matter, but maybe it does?I'll also note that while inspecting the HTML on the failed test output, I'm getting 5 of these:
Incorrect use of <label for=FORM_ELEMENT> The label's for attribute doesn't match any element id. This might prevent the browser from correctly autofilling the form and accessibility tools from working correctly. To fix this issue, make sure the label's for attribute references the correct id of a form field.
All 5 of the affected nodes are the
item
elements. That seems completely unrelated to this issue or bug, but wanted to note it here. Probably we need a follow-up to investigate this part, too. - Open on Drupal.org →Environment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Waiting for branch to pass - Open on Drupal.org →Environment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Waiting for branch to pass - last update
over 1 year ago 29,430 pass - Status changed to Needs review
over 1 year ago 3:53pm 16 June 2023 - 🇺🇸United States dww
Thanks for the review and fixes. Almost there! I opened 📌 Get all assertions working in JavascriptStatesTest::doMultipleSelectTriggerTests() Active as the follow-up to sort out the final test weirdness, and pushed a commit to add the @see comment pointing to it as part of the @todo.
I believe all feedback is now addressed, so this is ready for (hopefully final?) review. Removing the "Needs tests" tag, since that's now done.
Thanks again!
-Derekp.s. Before commit, this issue definitely needs a review of issue credits. There are a bunch of 1-off patches in here that probably shouldn't get credits, and a couple of folks who commented a lot but haven't uploaded anything who might need to be credited. If I have a chance later today, I'll work on this, but if I don't get to it, someone needs to before this is truly ready.
- last update
over 1 year ago 30,341 pass - 🇺🇸United States DamienMcKenna NH, USA
The latest merge request in patch format for 9.5.x.
- Status changed to Needs work
over 1 year ago 8:03pm 1 July 2023 The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- Status changed to Needs review
over 1 year ago 8:04pm 1 July 2023 Slightly confused here, I am looking for a solution for the scenario 4 in this comment
https://www.drupal.org/project/drupal/issues/1149078#comment-14590562 🐛 States API doesn't work with multiple select fields Needs review'select[name="dependee[]"]'=> [['value' => ['a']], ['value' => ['b']]],
It will only show the field if I select EXACTLY A or EXACTLY B (if I select A and B it won't show the field)I need the field to be displayed if I select A or B . I applied the latest patch and still see the issue. Has this issue been addressed in the latest patch/ am i missing something or is it going to be addressed separately?
Thanks,
Sukanya- 🇷🇴Romania vasike Ramnicu Valcea
@sukanya.ramakrishnan maybe at first sight
For
'select[name="dependee[]"]'=> [['value' => ['a']], ['value' => ['b']]],
We expect to work also for botha
andb
selected
But it's not true ... because it's about the value(s) of the element.And actually we have:
[a,b] !== [a]
and[a,b] !== [b]
Which means that maybe you need something different like
'select[name="dependee[]"]'=> [['value' => ['a']], ['value' => ['b'], ['value' => ['a', 'b']]],
To cover all the Multiple element value(s) scenarios.P.S. remember, we're talking about MULTIPLE and we're using SET of Values ... not Parts of Values
i hope i got it right :).
- Status changed to Needs work
about 1 year ago 7:26pm 31 August 2023 - 🇺🇸United States smustgrave
Still needs submaintainer review but @dww could the MR be updated for 11.x please?
- 🇮🇳India mukhtarm
Any idea on how to fix this issue, I applied the MR (https://git.drupalcode.org/project/drupal/-/merge_requests/3805/diffs) and still doesn't resolve the issue.
for eg:
$form['payment_methods'] = [ '#type' => 'select', '#title' => $this->t('Select the payment methods'), '#options' => [ 'cvs' => $this->t('convenience stores'), 'credit' => $this->t('credit card'), 'payeasy' => $this->t('Pay-easy'), ], '#multiple' => TRUE, '#required' => TRUE, '#attributes' => [ 'id' => 'payment_methods', 'name' => 'payment_methods', ], ]; $form['cvs'] = array( '#type' => 'details', '#open' => TRUE, '#title' => t('Convenience Store Options'), '#description' => $this->t('<b>Convenience Store informations are taken only if the cvs payment method selected.</b>'), '#attributes' => [ 'id' => 'cvs_fieldset' ], '#states' => array( 'visible' => array( ':input[name="configuration[link_type_plus][payment_methods][]"]' => ['value' => 'cvs'], ), ) );
Why i given the name so?: when inspected the element the name for the element displayed so. Also when giving
name="payment_methods][]"
also not working. - 🇷🇴Romania vasike Ramnicu Valcea
@MukhtarM i think you're trying "wrong selector"
':input[name=..."
when here it's aboutselect
'select[name=..."
could you, please, check?
thanks - Merge request !5188Issue #1149078 by vasike, wuinfo - Bill Wu, dww, Gauravvvv, Marios... → (Closed) created by vasike
- Status changed to Needs review
about 1 year ago 1:54pm 30 October 2023 - 🇷🇴Romania vasike Ramnicu Valcea
new MR for 11.x available ... all updated and "Green", so i think we're back to Needs Review
- 🇮🇳India mukhtarm
Thanks @vasike, The issue is fixed for me after applied the latest MR(https://git.drupalcode.org/project/drupal/-/compare/308e696f76fc33d6a5d8...).
Exactly was in the section:
Array(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. return Object.entries(reference).every(([key, referenceValue]) => value.includes(referenceValue), ); },
Correct solution for me is:
'#states' => array( 'visible' => array( 'select[name="configuration[link_type_plus][payment_methods][]"]' => [['value' => ['cvs']], ['value' => ['payeasy']]], ), )
if that helps anybody in future. This will show and hide the fieldset when either
'cvs'
or'payeasy'
is selected - 🇳🇱Netherlands ronaldtebrake
Can confirm for us https://git.drupalcode.org/project/drupal/-/merge_requests/3805 also did the trick (for D10.x)
I've attached a static patch of the current state of the MR for CI/CD purposes. - 🇫🇷France ericbellot
For me, on Drupal 10.1.6, the patch # 184 does not work completely. I had to go back to version # 183 of states.js.
- last update
12 months ago 29,721 pass, 1 fail - Assigned to nod_
- Issue was unassigned.
- Status changed to RTBC
8 months ago 4:09am 20 March 2024 - 🇫🇷France nod_ Lille
Works for me, it's simple enough and does the job advertised. Thanks for the tests too!
- Status changed to Needs work
8 months ago 5:01pm 24 March 2024 The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- Status changed to RTBC
8 months ago 5:26pm 24 March 2024 - 🇷🇴Romania vasike Ramnicu Valcea
it seems we need to hide the patch from #185
back to previous Status ... RTBC - Status changed to Needs work
8 months ago 3:56pm 25 March 2024 - Status changed to RTBC
8 months ago 3:45pm 26 March 2024 - Status changed to Fixed
8 months ago 3:58pm 26 March 2024 Automatically closed - issue fixed for 2 weeks with no activity.
Sorry, but the last version of Drupal 10.3.1 break all my "select" based conditional fields.
My conditional fields works well on Drupal 10.2.6Here an extract from my form where the field_child is visible when the taxonomy term (tid:1234) is selected
$form['field_child']['#states'] = [ 'visible' => [ ['select[name="field_parent[]"]' => '1234', ], ];
I answer to myself. finally i've solved my issue by fixing 2 points:
The first point was a mistake from my side : the "value" used in the definition must be a "string". In my code, i used an entity reference id defined as an integer.
The second point was linked to my select field defined as a "multiple" one. To address this point it's necessery to alter the "compare" function of core/misc/states.js. I add this js library into my module
/** * @file * Extends core/misc/states.js. */ (function($) { //correction core : can't add a #states on a select with multiple = true function _compareCustom(a, b) { if (a !== "undefined" && typeof b === 'object' && b instanceof Array) { if (b.includes(a)) { return true; } } if (a === b) { return typeof a === 'undefined' ? a : true; } return typeof a === 'undefined' || typeof b === 'undefined'; } //hack : surcharge function compare from core/states.js to use _compareCustom instead of _compare2 Drupal.states.Dependent.prototype.compare = function compare(reference, selector, state) { var value = this.values[selector][state.name]; if (reference.constructor.name in Drupal.states.Dependent.comparisons) { return Drupal.states.Dependent.comparisons[reference.constructor.name](reference, value); } return _compareCustom(reference, value); }; })(jQuery);
This solution works with all drupal version from 8.x to 10.3.1.