Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
Since #1427838: password_confirm children do not pick up #states or #attributes was committed, states are now incorrectly applied to (at least) 'item' form elements.
Specifically the intention of that issue was to change this code:
// Elements of '#type' => 'item' are not actual form input elements, but we
// still want to be able to show/hide them. Since there's no actual HTML
// input element available, setting #attributes does not make sense, but a
// wrapper is available, so setting #wrapper_attributes makes it work.
$key = ($elements['#type'] == 'item') ? '#wrapper_attributes' : '#attributes';
$elements[$key]['data-drupal-states'] = Json::encode($elements['#states']);
To something that includes the password confirm element, and indeed early on in the work for that issue we had this code:
// Elements of '#type' => 'item' are not actual form input elements, but we
// still want to be able to show/hide them. Since there's no actual HTML
// input element available, setting #attributes does not make sense, but a
// wrapper is available, so setting #wrapper_attributes makes it work.
$key = ($elements['#type'] == 'item' || $elements['#type'] == 'password_confirm') ? '#wrapper_attributes' : '#attributes';
$elements[$key]['data-drupal-states'] = Json::encode($elements['#states']);
However, after some discussion it was decided to not target the elements specifically, but try to identify the types of elements that would likely use wrapper attributes instead of attributes.
This resulted in the following code being committed:
// Elements that are actual form input elements, use '#attributes'.
// In cases like 'item' that are not actual form input elements or
// those like 'password_confirm' that have child elements,
// use #wrapper_attributes.
$key = (($elements['#markup'] ?? FALSE) === '' && ($elements['#input'] ?? FALSE) === TRUE) ? '#wrapper_attributes' : '#attributes';
$elements[$key]['data-drupal-states'] = Json::encode($elements['#states']);
This code broke a unit test, which was then fixed in a follow up commit where the fix was to make the testing form 'item' an input element, with no #markup, this is fine for the unit test, but there are plenty of places in core alone that set '#markup' to be a non-empty string, and it's still valid to expect states to apply on those elements, as they did before.
Steps to reproduce
Have a form with some #states set on a regular, plain '#type' => 'item' form element, with #markup a non-empty value, observe that the attribute isn't correctly added to the wrapper on the page. And thus the states doesn't work.
Proposed resolution
Introduce a new FAPI property: #states_attributes_target_key to explicitly control where the attributes generated for a #states property are placed in the render array (for example, #attributes vs #wrapper_attributes).
This provides a more reliable mechanism for specifying the target attribute key for states data than the existing internal heuristics approach that is based on element type and properties.
Set that new property to have the correct values for the core-provided elements of item and password_confirm and document it so that future core and contrib module authors can use it correctly too.
Improve the test coverage so that we actually test JavaScript states of item elements in render arrays that contain markup and improve the unit tests of the FormHelper::processStates method.
It was also reported during this issue that there were some issues with elements of type: container, and so some additional FunctionalJavascript tests have been added to cover that.
Remaining tasks
- Get the code reviewed and approved
- Get the code committed
- Publish the Change Record
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | states-not-processed-correctly-on-items-issue-10378.patch | 1.64 KB | vnech |
Issue fork drupal-3490435
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
steven jones commentedAdding more words to the title of the issue.
Okay, so the tests failed as expected.
I wonder what the correct solution is here?
I'm going to push the change to go back to the sub-optimal check for the type of element fix, as that would actually work correctly, though not optimally.
Then maybe we need some further discussion about how to resolve this. Do we need a new property on form elements:
'#states_attributes_target_key'that gives the name of the attribute to push the states attributes on to?Core can set this on item and password_confirm elements to change the target, and anything that wants to do this in contrib can do this too.
Comment #4
cilefen commentedComment #5
steven jones commentedUnassigning myself and setting to needs work, since we need to decide what to do here, but we do have an improved test and sub-optimal fix in the MR
Comment #6
dabley commentedI have hit the same problem, although in my case the #type is 'container'. My code worked correctly when I applied patch #2700667-104: Notice: Undefined index: #type in Drupal\Core\Form\FormHelper::processStates() , but as of D10.3.8, that patch no longer applies, and my code no longer works (the #states I've specified are ignored). When I apply the update in MR !10378, then my code works correctly again (fields are hidden or visible as they are supposed to be). So, MR !10378 looks good to me, although I haven't checked to see if it causes anything else to break.
Comment #7
vnech commentedThe patch works for Drupal core v10.3.10
Comment #8
bkosborneThis bit me as well. I agree the logic introduced in the original code that introduced the regression is not correct.
Comment #9
bkosborneThis patch works for me to resolve the issue. I know it's not an ideal fix, but this fixes the regression and I think it should be committed to address that so people don't keep hunting around for what caused it.
I don't understand why someone would create a form type 'item' and set
#markupto an empty string and set#inputto TRUE. In what scenario does that make sense? I think most people using 'item' elements are in fact adding markup.Comment #10
catchWould be nice to get a form API maintainer review on this issue (shouldn't stop another framework manager committing it if they want to, but I'm not sure I want to without a second opinion on whether there's another option here).
Comment #11
quietone commentedSimplify title. No need to refer to the other issue in the commit message. The [regression] informs the reader there is an older issue involved.
Comment #12
godotislatePer #6, was there also a regression for
'#type' => 'container'that is fixed here? In which case, should there be a test for that as well?Comment #13
tim.plunkettThis should include test coverage for `container`
Comment #15
lendudePushed a test using a container and added support for this in FormHelper, not sure a growing list of exceptions to the rule is the best way to handle this, but ¯\_(ツ)_/¯
Comment #16
needs-review-queue-bot commentedThe 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 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.
Comment #17
steven jones commented@lendude thanks, but I think the comment in #6 was saying that the MR already fixed the container case, so we don't want to special case it, but instead revert it to what it was doing before this regression.
I've tweaked the code back to the special casing of only the password confirm and item elements, as per the previous code that was in core for some time (plus handling the password confirm case non-ideally).
Once we've gotten this regression sorted, and the test cases in to verify that, then we can discuss changing the behavior / implementation, which didn't really happen in #1427838: password_confirm children do not pick up #states or #attributes which is what was causing the whole problem in the first place.
@tim.plunkett I assume that adding the container test is enough here to get this regression committed and fixed?
I mean, technically, we're testing the else case of a conditional, so we could add a lot more test cases, but that seems redundant, we'd essentially need an infinite set of test cases to double check that Drupal does the 'right thing' when
A != B, for all values ofBthat don't equalA.For clarity we have this conditional:
And our tests send that code cases where:
$elements['#type']is:'date''item''password_confirm''container'Comment #18
larowlanLeft a comment on the MR
In the original issue @tim.plunkett expressed similar concerns about a fixed list of plugin IDs. I share that concern and have proposed an API addition to resolve that.
Comment #19
mlncn commentedThe current merge request fixes this and has a test. I agree with darthsteven that the bug needs to be fixed first and then the API addition can be worked on in a new issue with the tests already in place to prevent another regression.
Respectfully boosting to major as this is a significant regression and it can hit key workflows and – how do the marketers call it? – success journeys like checkout or donations, with the result that key information and functionality shows up when it should not— or does not show up at all.
(And i think these words are all in here already but i still needed StackExchange to surface this issue for me, so to repeat the common representation of the issue: drupal form #type item not working with #states API.)
Comment #20
mlncn commentedOh and unfortunately documentation for FAPI seems to have largely moved out of the codebase and the generated table of D7 days that remains the most useful reference and into the Drupal Wiki, in particular Conditional Form Fields and Form Render Elements which does link to in-code documentation sometimes including for Form States API. Maybe this would make sense there?
Comment #21
needs-review-queue-bot commentedThe 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.
Comment #22
steven jones commentedSomeone will need to help me with managing the MR because I'm guessing I need to make a new branch and rebase everything onto 11.x? Or something like that.
I think this can be reviewed again, and then there's some busy work to do to get this in a position to merge into the latest Drupal 11 branch etc.
Not sure if the tests are now sort of overkill, but hey ho, they'll correctly fail in previous versions without this fix either way.
Comment #23
godotislateSince it looks like the MR is yours, you can retarget it to 11.x instead of 11.1.x in the Gitlab UI. You will also need to rebase the branch against the main repo's 11.x locally and force push it up.
Alternatively you can make a new branch locally against 11.x, add your changes, then push that to the fork and create a new MR. Then close the previous one.
Comment #24
needs-review-queue-bot commentedThe 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.
Comment #25
steven jones commented@godotislate Thanks for the confirmation that rebasing was the thing to do here.
I've run:
And I think that's done the trick, I've updated the MR and let's see if the needs-review-queue-bot is a happier bot now.
Comment #26
godotislateThere was a failing test in the build, but I think it's unrelated and an intermittent failure. On the next update of the MR, though, we should make sure to re-run any failing test to make sure it gets to green.
One comment on the MR about where to document new the new form array property
#states_attributes_target_key.Because there's an API addition, we should probably add a change record, so tagging for that.
Last thing is that the unit test changes look good, but the unit test itself doesn't actually test the bug. For that it looks like we need updates to:
Drupal\FunctionalJavascriptTests\Core\Form\JavascriptStatesTestDrupal\form_test\Form\JavascriptStatesFormSeems like there's test coverage already for
password_confirm, but should verify it's sufficient. There is some coverage foritemas well, but I believe an example of anitemwith#markupis needed. Or perhaps #markup could be added to one of the existingitemelements. Doesn't look like there's acontainerelement in the form or a test for states functionality on container elements, should that should probably be added as well.Comment #27
steven jones commentedI've moved the documentation and added a test for an item with markup.
Tests look to be passing now.
I'd argue that adding more tests for 'container' when we're trying to revert a very specific regression that hasn't effected container isn't the right place to do it. We can spin out a separate issue for that work and we can crush the life out of another willing software developer over there.
I'll do the rebase, again, to move this over to the
mainbranch in a moment.Comment #28
steven jones commentedMarking as needs review again.
Comment #29
godotislateThanks for your work on this @steven_jones!
@dabley reported in #6 that this change here (well, an older version at least) fixed a similar issue with
container. A subsystem maintainer in #13 said that a test to show thatcontaineris fixed should be included.I'd say that we should confirm whether the latest changes do in fact fix
container, and if they do, then add a test case for it incore/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.phpandcore/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php. One case is probably enough (say,visible), and probably not necessary to test all the states variations.If the changes here don't in fact fix states for
container, then that can be kicked to a new issue.Separately, we could use a change record for the new
#states_attributes_target_keyproperty.Comment #30
steven jones commentedComment #31
steven jones commentedAdded some tests for container, but I think they'll pass either way, since this code doesn't change anything for containers as fas as I can tell. The tests can stay though, no reason not to add them now that they are written.
So...just need the change record now.
Comment #32
steven jones commentedHad a go at a draft change record: https://www.drupal.org/node/3572685
Comment #33
dwwThis was linked in Slack. I was a little confused by the summary posted there based on the title alone, since "Form states" makes me think of
FormStateInterfaceand$form_state. This is talking about#states. Trying to disambiguate the title. 🤓Also, tagging to be smashed. Will try to review in the "near" future, if I can. Bugs with
#stateshave always been an area of interest for me.Thanks!
-Derek
Comment #34
oily commentedReviewed the CR and tried to slightly clarify the difference between the existing 'internal heuristics' 'approach' and the new one. Other than that 'improvement' (he says optimistically) the CR LGTM. it is detailed and manages to explain quite an abstract ahd complex change to the Forms API.
Comment #35
oily commentedComment #36
steven jones commentedTagging as suggested in Slack.
Comment #37
needs-review-queue-bot commentedThe 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 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.
Comment #38
steven jones commentedI've done a rebase to see if that makes the bot happier. I can't see why the PHPStan rules are failing.
Comment #39
smustgrave commentedCan the proposed section be flushed out.
Comment #40
steven jones commentedComment #41
steven jones commentedComment #42
smustgrave commentedLeft a small comment on the MR.
Comment #43
smustgrave commented@oily please don't rebase unless it has a merge conflict as it's not needed (unless it's 100s back)
Comment #44
oily commentedUpdated the fork and pipeline all green. Test-only test:
https://git.drupalcode.org/issue/drupal-3490435/-/jobs/9380610
Not sure if the test-only output helps with #42? Does it suggest a need to re-enabe deleted wrapper-attributes fallback?
Comment #45
smustgrave commentedChecked the threads that have been resolved, took a shot at adding credit too for those that helped move the issue forward.
Leaving 1 thread open for committers though.
Comment #46
oily commentedRe: #43, That decision was influenced by #38. I was confused. I thought #38 meant a rebase had been done but PHPSTAN was failing afterwards. I actually clicked 'Update fork', I believe. Just wondered if that might clear any PHPSTAN problem. Looking at the rebase referenced in #38 seems the pipeline ran green afterwards. Now not exactly sure about the PHPSTAN issue.. I agree my rebase was unnecessary.
Comment #47
oily commentedRe: #45 There are test failures (see #44) which although test failures are expected in the test-only test, are test breakages? e.g. the failures in lines 94 and 98 of FormHelperTest.php? The test failures that are supposed to happen are in the output, too.
But I am suggesting that there appear to be additional failures which might need to be fixed? Are they possibly connected with the removal of the 'wrapper-attributes fallback'?
Comment #48
steven jones commented@oily no...so the test failures there are all expected.
Lets take them one by one:
Drupal\FunctionalJavascriptTests\Core\Form\JavascriptStatesTest::testJavascriptStates- is because there's a bug in the core implementation of states, and this is exactly exposing it.Drupal\Tests\Core\Form\FormHelperTest::testProcessStates@empty item- This is detecting the core bug being fixed.Drupal\Tests\Core\Form\FormHelperTest::testProcessStates@item with markup- again, the core bugDrupal\Tests\Core\Form\FormHelperTest::testProcessStates@item with markup and input- again, more core bug, because the heuristic is flawed.Drupal\Tests\Core\Form\FormHelperTest::testProcessStates@password_confirm- This one is slightly interesting, in that the test is a unit test, and thus doesn't merge in all the element defaults from the element definitions, i.e. the getInfo. And given that the heuristic relies on those, this unit test fails. This is correct, as its testing that unit of code, which with all the changes, doesn't have the heuristic so doesn't need all the other properties from thegetInfo.Basically: all as expected.
More broadly, about the heuristic approach, I don't think we should particularly revere it, I appreciate that it's now been in core for a long time and might even be considered a feature, not a bug, but equally states working properly was a nice feature that became a bug.
In the issue summary I outlined where it got introduced: https://www.drupal.org/project/drupal/issues/1427838#comment-15789918 which frankly to me looks like someone came up with a likely looking heuristic to use, which sort of works for some of the edge cases, and then it seemed to get in without much further thought or consideration, and in fact, when it went on the break the tests....the tests were changed rather than the heuristic questioned/tweaked.
Anyway, this is why I favour returning to basically what was there before: being explicit about which elements need special treatment, but extending it so that as form element authors, they can decide and opt-in.
Comment #49
oily commentedHi @steven jones, Thank you for the very full response. Will read with interest.
I came hear after I considered the test-only test in this issue and realised that I was wrong. That test tests without the code fix so my idea that the removal of the wrapper-attributes blah blah was breaking test assertions is rubbish.
Re: #48, I take no position on the heuristic vs basic approach. The tests seem to work as expected so RTBTC it is!
Comment #50
dwwI did a more thorough review of the MR:
#states_attributes_target_keyis not defined.core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.phpandcore/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.phpbeing expanded (very slightly) to cover this. That's all exactly as intended. I wrote most of that test originally, nice to see the basic plumbing is still easy to build on. 😊 I hope. 😂core/tests/Drupal/Tests/Core/Form/FormHelperTest.phpas a Unit test.core/modules/system/templates/form-element.html.twigis right and proper. The new docs incore/lib/Drupal/Core/Render/Element/FormElementBase.phpare all we need.I don't see anything to complain about or improve. +1 RTBC from me.
Thanks!
-Derek