Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
It seems that #states with radios doesn't work quite right.
It works when the page is loaded, but doesn't change when the radio is changed.
So in the example below, if 'one' is selected, the checkbox is unchecked on page load. If 'two' is selected, the checkbox is checked on page load. Works OK that way. But when you change the radios, it doesn't change the checkbox.
$form['dummy_radios'] = array(
'#type' => 'radios',
'#options' => array('one' => 'one', 'two' => 'two'),
);
$form['dummy_checkbox'] = array(
'#type' => 'checkbox',
'#title' => 'dummy checkbox',
'#description' => 'checked if two checked above',
'#states' => array(
'checked' => array(
'input[value=two]' => array('checked' => TRUE),
),
),
);
Comment | File | Size | Author |
---|---|---|---|
#27 | states_bug_demo.tgz | 2.65 KB | rfay |
#18 | drupal.states-radios.18.patch | 1.03 KB | sun |
#17 | drupal.states-radios.17.patch | 4.05 KB | rfay |
#16 | drupal.states-radios.16.patch | 2.4 KB | rfay |
#13 | drupal.states-radios.13.patch | 8.91 KB | sun |
Comments
Comment #1
sunConfirming.
Comment #2
sunWorks for me.
Comment #3
sunNow also explaining that condition. :)
Comment #4
sunComment #5
rfayDoesn't work for me. Firefox 3.5, Linux.
Attached is a demonstration module. Use the "Bug Demo: #states radios element not working" menu entry.
What it *does* do: If you reload the page, the correct action is taken for the selected item. But not if you change it on the page.
If you think I should have different syntax in the example, I'm sure you'll let me know :-)
Comment #6
rfaykkafer said in his drupalcon presentation that radios were not supported at this point. @sun, have you had a chance to take a look at the demonstration module in #5 to see if you could make it go?
Comment #7
sunThanks for that demo module. Attached patch works for me.
Comment #8
sunoh, but you need to use the following in your code instead:
Comment #9
rfayWorked for me!
The attached patch is just sun's #7 again, with a demonstration added to form_test.module. I also changed the hook_menu() entries to remove the #type => MENU_CALLBACK, as I believe the various items should be easily accessible for manual testing.
All a part of my campaign to make sure that issues are understandable to the reader and manually testable, whether or not we can test them with the current PIFR system.
To test this patch, enable form_test.module (
drush enable form_test
) and access the menu named "#states Radio button demonstration" (/form-test/radios-demonstration). Then click the radio buttons and watch the textfield element change.Thanks for your effort on this, sun.
Comment #10
sunHm. Although I fully understand your intention, I'm not sure whether it is a good idea to add demonstration code resp. manual testing code in a uncontrolled and unplanned fashion like this. I would recommend to remove this addition for now and start a separate issue to discuss and shape up a proper, agreed-on way to do this. For example,
- you made various menu items visible, which I'd agree with, but perhaps doesn't make sense for all.
- the code duplicates what is in Examples module already.
- the added form constructor is specific to #states with radios. Doesn't make sense for me, as there could be a single one that allows to manually test all kind of #states for various elements.
...and probably more.
Duplicate newline here.
We can drop this description.
Initial newline should be removed.
We should remove t() here.
Invalid multi-line array syntax.
94 critical left. Go review some!
Comment #11
rfayThanks for the good thinking, @sun.
I've been talking up this idea with various people and got good response from the likes of webchick and dries, but without a clear plan or approach, so this is a good beginning discussion.
I would agree with you... except that what we currently do with all test code is to add it in an uncontrolled and unplanned fashion. If we had an overriding plan for test code, we should follow that plan, but it's not our current way.
I took a look and in this case I think all should be visible. The general approach of making a menu entry visible can't do any damage that I know of. But I'm certainly open to conversation.
Actually the purpose of this type of code and the purpose of Examples are different, especially in the development process. The purpose here is to demonstrate, where the purpose in Examples is to communicate (and demonstrate, I suppose). So the idea here is just to put code in that can be used in the context of this issue.
I would think that as we add related tests/demonstrations, we would add to this form constructor. My intent was to use the same approach we currently use with tests.
And of course all the rest of your comments are style, and you're the master of style.
If I knew a better place to discuss this I'd put the discussion there, but for now I'll just try to convince you :-)
The intent: Deliberately make issues manually testable, whether machine-testable or not. We waste mindspace if we don't do this.
The style: Just folllowing what we currently do for tests.
What do you say?
Comment #12
sunwell, we can let the core maintainers decide. However, we still need to fix the issues I mentioned.
Comment #13
sunFixed those issues. Let's fix this, please.
Comment #14
rfayYay, I agree RTBC. Thanks, sun.
Comment #15
webchickHm. This patch is a bit of a monster. I don't really agree with the whitespace changes and have no idea where it's documented that making this stuff less readable is a good idea. I agree in principle about removing MENU_CALLBACK from the test module, but am of mixed feelings about it being done here and not in other test modules, because it creates inconsistency.
Could we pull that stuff out and just fix this bug?
Comment #16
rfayRerolled without the demonstration test. Pushing to RTBC as it already was.
This one will slightly conflict with #767738: Rewrite drupal_process_states() documentation, deal with action of 'required' for #states, so if it goes first that one will need a reroll.
Comment #17
rfayPut the demonstration test back in, removed the MENU_CALLBACK changes. Per IRC communication/confusion/bafflement with webchick :-)
Still RTBC.
Comment #18
sunwebchick had the same reaction as me, and so I think we should defer the demonstration code stuff to a separate issue, as mentioned before.
Let's just fix the bug here. Documentation for #states in common.inc is completely revamped in another issue already.
Comment #19
rfay@sun, webchick wanted the demonstration stuff, and asked me to put it back into #17. I agree that if we pull the changes to the docblock it will make life easier - but we'll have to add the stuff about radios over there.
Comment #20
sunwebchick can and should overrule me, for sure. But my gut feeling still is that we should coordinate the addition of "demonstration code for manual testing" a bit more. If I'm not mistaken, not all testing modules can be enabled safely, i.e. without potentially doing harmful stuff to your site. And when I somehow manage to enable a testing module for manual testing, then I'd probably expect some kind of sane structure...
For example, I recently started to work on a "proper" markup_test module in my sandbox, which tries to introduce and build a sane technical structure for manually testing + reviewing issues for the "markup" component, but with the goal to do it in a generic and extensible way in the end. Ultimately, when there's some more code and experience with that module, it will be worth to have its own d.o project, as you will likely be able to use the same testing code to ensure proper styling + behaviors in contributed and custom themes you build.
That markup_test module, as well as the Examples module, as well as the suggested demonstration code here, may be partially but not fully duplicating efforts, but I'm not sure yet. What rather matters for me is that we don't add arbitrary code to Drupal core with no clear vision, structure, and behavior -- at least not at this stage of the release cycle.
Comment #21
rfayCreated #785362: Create more extensive guidelines for "mock module" creation and style
I definitely agree that we can duplicate code with the demonstration technique... and that we have been doing that regularly since we began the whole simpletest thing. I don't think it's necessarily bad.
Comment #22
rfayI have no objection to any of the patches above. My intent was to do a first proof-of-concept of a manual test that could accompany a patch. #17 does that.
#18 has just the fix, nothing else, and will not collide with the documentation being prepared in #767738: Rewrite drupal_process_states() documentation, deal with action of 'required' for #states, although that one will have to be updated with the caveat about using 'value'.
#17 has the fix with some documentation and with the demonstration manual test.
#16 has the fix with some documentation and without the manual test.
#13 has the fix with some documentation and with the manual test, and with setting type=MENU_CALLBACK on some of the items.
It's not worth getting stuck on this. @webchick, please choose one and go for it. We'll work this out in the future. When this is fixed, a demonstration will go into Examples modules, so this issue doesn't probably strictly need the demonstration.
Comment #23
sun@rfay: While we're waiting for a commit here, I'd happily welcome your review on #783438: #states doesn't work for #type item :)
Comment #25
webchickThanks. Committed #18 to HEAD!
Comment #26
tstoecklerCan someone confirm this actually works on current HEAD? I just updated my HEAD and I cannot get it to work.
I would provide sample code, but to see if I wasn't making any stupid mistakes I copied the exact code from the documentation and tried again with no success. Also using 'checked' => TRUE works for me (except that it doesn't make much sense for radios).
Thanks.
Comment #27
rfay@tstoeckler, works for me. Attached is the module I just used for testing it.
Use the menu entry: Bug Demo: #states radios element not working (bug_demo/states_radios)
Comment #28
tstoecklerThat's how it's called, and in fact it does not work for me. Todays HEAD.
Comment #29
rfay@tstoeckler, we all want it to work correctly, and believe me we don't doubt that you may have a bug. Are you saying that the test module I gave doesn't work for you? If so, please provide details that may help to sort this out. Or is there another construct that doesn't work? If so, please adjust the test module and give us a demonstration.
Comment #30
tstoecklerDamn...
My jQuery version was corrupted. Don't ask me how that happened. Got a fresh copy of jQuery and the demo module works perfectly now, as well my own code.
Sorry for the noise.
Comment #32
jason.fisher CreditAttribution: jason.fisher commentedI know this is closed now, but this could be extended to also properly support "checked" for radio buttons, as per the original request.
Would that be a patch on this existing issue, or a new issue?
That addition allows the second example here to work exactly as the first (which the already committed patch addresses):
Comment #33
vipul.jadvani CreditAttribution: vipul.jadvani commented9: drupal.states_radio_buttons_767268_08.patch queued for re-testing.