Currently the #type => "checkboxes" element does not support an option in the #options array with the key 0. This introduces the need for a quite complicated workaround in modules/field/modules/options/options.module.
The reason for this restriction is due to confusion between 0 (numeric) and "0" — the former indicates that the underlying checkbox is not checked). AFAICT this restriction can be avoided if we are careful about casting to string when needed.
This patch adds a few casts here and there and removed the $zero_placeholder stuff from the options module. The patch includes parts of the patch for #319483: FAPI checkboxes and radios need strengthening for XSS.
Comment | File | Size | Author |
---|---|---|---|
#89 | fapi-checkbox-zero.88.patch | 17.51 KB | effulgentsia |
#86 | fapi-checkbox-zero.86.patch | 12.72 KB | effulgentsia |
#85 | no_more_broken_checkboxen.patch | 11.44 KB | chx |
#83 | no_more_broken_checkboxen.patch | 11.37 KB | chx |
#81 | needs_checkboxes_test.patch | 8.29 KB | chx |
Comments
Comment #1
c960657 CreditAttribution: c960657 commentedThe $zero_placeholder stuff was introduced in #635202: Remove #process pattern from option widgets as a workaround for a regression introduced in #179932: Required radios/checkboxes are not validated. I have not studied these issues deeply. My initial angle on this was pure FAPI, not related to Fields.
The patch hasn't been tested intensively. For now I am interested in opinions on whether this will work at all, or whether I missed the real reason why 0 was not allowed.
Comment #2
webchickWe need chx to take a look at this.
Comment #3
bleen CreditAttribution: bleen commentedJust throwing it out there that I have run into this as an issue on several occasions.... +1 for figuring out a way to make this patch work
Comment #4
Reg CreditAttribution: Reg commentedsubscribe
Comment #5
chx CreditAttribution: chx commentedI can't believe my eyes that someone wrote this and actually made tests pass. I will check what sorts of tests we have and whether things still are OK. They should for the patch looks good. Thanks for not ushering it in without me. Stay tuned.
Comment #6
chx CreditAttribution: chx commentedSo yeah what this needs is a multistep example which checks that checkboxes w a 0 key can be ticked on/off. Also,
this either requires a comment or something in the issue or something... now it's '1', right? Why? Edit: what this mean for 0 checkboxes, will the value be 0 or '0'... will the type change or will it be always '0'?
Comment #7
c960657 CreditAttribution: c960657 commentedI changed the quoted code to just
if (class_exists($class_name) && $value) {
— in this case it's not important, whether the value is 1, '1' or TRUE, so I guess just treating it as a boolean is the most PHP'esque way of doing it.The latter. I added
(string)
casts form_type_checkbox_value() to ensure that reported checkbox values are always strings. I don't see that we need to distinguish between e.g. 1 and '1'.Like this?
Comment #9
c960657 CreditAttribution: c960657 commentedReroll (due to #639466: hook_options_list() + XSS filtering).
Comment #10
yched CreditAttribution: yched commentedI'll let chx set to RTBC when he thinks it's ready.
Just chiming in that options.module changes are fine.
Thanks for this, c960657 !
Comment #11
webchickThis is the one hunk I do not understand and makes me a bit nervous. Does this mean we can never have numeric return values on checkboxes anymore..? Why do we only make this change in seemingly this one place and not everywhere in core?
I'm on crack. Are you, too?
Comment #12
webchickDuh. I get it. Because this is the place where checkbox elements themselves are defined. Still though, some explanation of this might be good, perhaps an inline comment too. If i saw that out of context, I would probably think it was silly and change it to TRUE instead of '1'.
Comment #13
chx CreditAttribution: chx commentedConsider this "RTBC once comments are added". I would like to see a comment on
if ((string)$element['#value'] == (string)$element['#return_value'])
please (because every string is == 0 i know but it's not trivial).Comment #14
c960657 CreditAttribution: c960657 commentedAdded comments. Marking RTBC.
Comment #15
c960657 CreditAttribution: c960657 commentedReroll. Had to make some manual changes and updates to form.test and form_test.module due to a conflict with #302240: button broken due to fix various problems when using form storage . Would you like to give it another look? Only those two files are changes.
Comment #16
webchickComment #21
yched CreditAttribution: yched commentedTests pass - back to RTBC.
Comment #24
agentrickardThis was RTBC until a testbot glitch, AFAIK. Setting back.
Comment #26
sunsubscribing
Comment #27
yched CreditAttribution: yched commentedStill green. Back to RTBC.
Comment #28
sunI'm a bit concerned about how much critical and partially also security-related logic we are putting into theme functions here.
Just a thought: Can we move this into #value_callback functions? Perhaps, by adding a custom #checked or #selected or #enabled property, which becomes either TRUE or FALSE?
Is this change really/actually required? It seems like we're casting to string everywhere.
Powered by Dreditor.
Comment #29
c960657 CreditAttribution: c960657 commentedThis patch introduces a new function, form_process_checked(), that adds a
#checked
key to all checkbox and radio elements. What do you think of this? The key is read-only and only used by theme_checkbox and theme_radio, so modules should still use #return_value and #value/#default_value to specify whether an element is checked or not. I'm don't know whether this is confusing.The function treats radio and checkbox elements the same way, i.e. a radio with a #value of the number 0 is considered unchecked. Is that a problem, or is the consistency an improvement? See also a somewhat related issue awaiting review: #650666: Inconsistent behaviour of unchecked radio and checkbox
No, only for consistency and readability. The patch tries to clarify that the data type for radio and checkbox values is string.
Comment #30
c960657 CreditAttribution: c960657 commentedChasing HEAD.
Comment #31
Eric_A CreditAttribution: Eric_A commentedComing from #642820: PHP notice when submitting form with disabled checkbox. I love the ideas in this patch! Still:
For disabled checkboxes I don't think you can cast the checkbox #default_value to string and return it without first making sure $element['#default_value'] !== 0.
I going to look at the patch thoroughly, but I wanted to mention this now.
Comment #32
Eric_A CreditAttribution: Eric_A commentedI think in this patch it should be:
Comment #33
Eric_A CreditAttribution: Eric_A commentedHmm, we should return NULL in the above block if $element['default_value'] is undefined, even if it leads to wrong element values. But that is another issue. (The $input === FALSE case should return integer 0 for the case where an unchecked checkbox has $element['#access'] === FALSE and for disabled unchecked checkboxes, but currently it returns nothing. EDIT: applies to the case where $element['default_value]' is undefined.)
Comment #34
effulgentsia CreditAttribution: effulgentsia commentedWow! What an absolutely awesome patch.
#32/#33: Not as part of this issue: let's leave that discussion to #642820: PHP notice when submitting form with disabled checkbox.
As far as I can see, this is ready to fly except for two very minor points:
Remove this comment. That code is now handled by form_process_checked() and has no relevance to the theme function.
form_process_radios() achieves this by setting #parents. Is there a reason why checkboxes need to be different? If there's a reason to set #name here rather than #parents, does the same need to be done in form_process_radios()?
This review is powered by Dreditor.
Comment #35
effulgentsia CreditAttribution: effulgentsia commentedOn 2nd thought, I'm questioning #30. I just don't understand the need for so much casting to string, especially in form_type_checkbox_value(). In HEAD, you can set the #return_value of checkbox 'foo' to integer 5, and if it's submitted checked, $form_state['values']['foo'] will be integer 5. Why do we want this issue to change that behavior to forcing $form_state['values']['foo'] to a string?
Here's a patch that only casts to string within form_process_checkboxes() and form_process_radios(). Is there something I'm missing? It also includes #34 and a change from form_process_checked() to form_pre_render_checked(), since there's no reason someone can't include checkboxes for rendering on a page that's not a form.
Comment #36
effulgentsia CreditAttribution: effulgentsia commentedWith the following minor tweaks:
Here we should cast to strings, even though it doesn't really matter, since we deal with 0 above.
Add comment for why casting.
Add comment for why casting.
This review is powered by Dreditor.
Comment #37
Eric_A CreditAttribution: Eric_A commentedIn HEAD, you can set the #return_value of checkbox 'foo' to integer 5, and if it's submitted checked, $form_state['values']['foo'] will be integer 5. Why do we want this issue to change that behavior to forcing $form_state['values']['foo'] to a string?
Yeah. In theory I agree that string values are only really needed in html context.
Discussions are mostly about integer and string types in $input and $form_state['foo']['values']/ $form['foo']['#value'], but:
If FAPI #value and #return_value is about xhtml $_POST/$_GET, then let it always be strings, wether the value is coming from the browser or from the API. But basically only cast values to string when you are in xhtml input/output context.
For convenience it would be nice to be able to just shove a custom type object instance into #return_value and depending on user input either get that instance passed back or something signaling an unchecked state. Just put a __toString method in your custom class and the theme layer (theme_checkbox) will know what to use for the xhtml form input value atrribute.
Does anyone do this in contrib?
Comment #38
effulgentsia CreditAttribution: effulgentsia commentedEric_A correctly points out in #642820-26: PHP notice when submitting form with disabled checkbox that empty string should also be a valid #return_value, leaving only FALSE and integer 0 as having special meaning, which I believe is the original intent within FAPI.
Besides the code in this patch, there's other places that need fixing (for example, everywhere array_filter() is used in relation to checkboxes). And all this points to the need for much more thorough tests.
Comment #39
Eric_A CreditAttribution: Eric_A commentedNote: edited a little more. See emphasised and strike throughs
A quick comment.
It's a great step to pass the theme layer a #checked property, rather than have it figure it out itself!
It seems we need to be able to process the underlying logic in the form element value callback (edit: that, or probably better: the process callback) and if necessary in the render element pre_render callback. A helper function seems in order.
Forms API would use the form value callback to determine a valid #value from $input. edit: again, a process callback could assist
Render API would use #pre_render to do some work *if* the #checked key does not exist. So if you are not using Forms API and just want to use Render API to render a checkbox, you set either the
#value#default_value property or the #checked property.Edit: an important idea here is to have both API's working with the same element values.
Edit: Main point is that forms are not always rendered and that a submit handler too should know for sure wether the box is checked or not. So the checked/unchecked rules cannot live in the pre_render function.
A forms API #process function and a Render API #pre_render function could both call the same helper to determine checked status.
Comment #40
andypostI think that all mess is because there's no conclusion about return values - what kind of type (int, string) the return value should be.
We already have docs at http://api.drupal.org/api/drupal/developer--topics--forms_api_reference....
#type = checkbox
if defined #return_value else 1 (int)
#type = checkboxes
if defined #return_value else 1 (int) for each keyed entry of return array
#type = radio
if defined #return_value else 1 (int) (but now in docs undefined)
#type = radios - wtf in example (non keyed array in #options and nothing about #default_value)
undefined?! suppose shoud be array with keys and each key value should be 1
#type = select
nothing in docs! suppose keyed array with values from #options, also what's about #default_value?
Comment #41
fago>Besides the code in this patch, there's other places that need fixing (for example, everywhere array_filter() is used in relation to checkboxes). And all this points to the need for much more thorough tests.
Why is it important to have return values of an empty string or "0" ? Even if it's allowed in the XHTML spec that doesn't mean we have to support that. Is there any value in that? Why not simply extend the "numeric 0" is treated as unchecked to everything that evaluates to FALSE is treated as unchecked?
Doing so would allow us to keep the much simpler checks and thus keeps the code cleaner. Also from a developer using FAPI point of view it makes sense to me that when int 0 isn't supported, '' and '0' aren't either.
Comment #42
c960657 CreditAttribution: c960657 commentedI disagree. The original purpose of this issue is to remove the ugly workaround in the Field module that was necessary due to "0" being treated as unchecked. The first patches actually removed more code than they added.
Comment #43
fagoIndeed - as long as we have to support option values evaluating to 0 yes. But when FAPI doesn't support it, field API shouldn't either. It doesn't necessarily need to do so I think.
Anyway I'm fine with fixing that in FAPI, but then as effulgentsia wrote we need a good test coverage for that so we can be sure we don't miss anything.
Comment #44
yched CreditAttribution: yched commentedre "But when FAPI doesn't support it, field API shouldn't either"
CCK D6 supports it, so if Fields D7 doesn't, some data migration will be tricky.
IIRC, 0 as a valid value in checkboxes / radios worked in FAPI D6, CCK didn't have the workarounds we had to put in D7
Comment #45
Eric_A CreditAttribution: Eric_A commentedWhy is it important to have return values of an empty string or "0" ?
Suppose I build a math quizz. A new question of the form a + b = ? is generated by the system every time. Radios are generated with the correct answer and a few wrong answers. Well, I just want to be able to use that "0" and do my calculations without any special casing.
Comment #46
fagoI see, makes sense.
in _form_validate() there is:
> $value = $elements['#type'] == 'checkboxes' ? array_keys(array_filter($elements['#value'])) : $elements['#value'];
But if '0' is allowed, that needs to be fixed.
Comment #47
YesCT CreditAttribution: YesCT commentedIn preparation for the new "major" issue priority, I'm tagging this (not actually critical) issue as priority-major.
See: http://drupal.org/node/669048#comment-3019824 (#669048: We should have four priorities not three (introduce a new 'major' priority)) for more information about the coming major issue priority.
Comment #48
effulgentsia CreditAttribution: effulgentsia commentedThis is still on my radar, though I'm not sure when I'll be able to get to it, and certainly don't mind if others want to push it along. I really believe the best way to proceed on it is to first figure out all the possible expectations that require tests to be added, add those tests, then merge in hunks from earlier patches that seem relevant. When thinking through tests, consider that developers are used to setting the #default_value of a checkbox to TRUE or FALSE. How should that work when #return_value isn't "1", and especially, if it's empty string or string "0", and how does that get translated into #value when there is user input and when there isn't?
Anyway, for now, just adding another tag to keep this issue in front of the right people.
Comment #49
sunCross-linking #650666: Inconsistent behaviour of unchecked radio and checkbox
Comment #50
marcingy CreditAttribution: marcingy commentedChanging to major as per tag.
Comment #51
klonossubscribing...
Comment #52
MustangGB CreditAttribution: MustangGB commentedTag update
Comment #53
klonos(...coming from #259292: Required radios/checkboxes are not validated (D6))
I understand this was caused when #179932: Required radios/checkboxes are not validated landed. I need to ask a simple question and then I'll hold my peace (as to not bother people with backporting to d6 questions and such). Does this issue apply to d6 too? ...I mean after patch in post #23 from #259292: Required radios/checkboxes are not validated (D6) is applied.
Comment #54
webchickThis sounds like maybe the proper place to post this bug I accidentally found in #140783: A select list without #default_value always passes form validation.
This automatically defaults "Banana" to checked, even though I didn't specify a #default_value. This represents a regression from D6.
Comment #55
klonosI'm taking it there then. Thanx for the info Angie ;)
Comment #56
bleen CreditAttribution: bleen commented@webchick: Most people use "foo" and "bar" and such as examples; I admire your creativity. Monkeys... :)
Comment #57
chx CreditAttribution: chx commentedNow.
The identify part is missing. First, I have dissected the existing code. It is a mess. Let me list what I found.
form_type_checkbox_value
sets checkbox#value
to the integer0
and to#return_value
if anything was posted. As we know, unchecked posts nothing.#default_value
gets copied over to#value
.#return_value
is what gets into HTML as the attributevalue
, the call chain istheme_checkbox
callingelement_set_attributes
. This means that#return_value
has security implications. The simplest solution is to use a preprocess function to check_plain it -- that way the check_plain'ing (and the consequent string casting!) does not get into trouble with the rest of the code. Per the previous point it does not matter what is posted so it is okay to solve the problem separately.system_elements
.theme_checkbox
corroborates withform_type_checkbox_value
in stating that unchecked checkboxes are set to the integer0
.form_type_checkbox_values
states the "Programmatic form submissions use NULL to indicate that a checkbox should be unchecked [...] since a checkbox can never legitimately be NULL" where of course it comes to question what can not be NULL? This probably means that the values of the$_POST
array can never be legitimately NULL which is hardly a surprise as they are all strings. This has nothing to do with the rest of code but as the comment was unclear it needs to be cleaned up.form_process_checkboxes
uses the key of the#options
array for a#return_value
so most of the time#return_value
is a string or an integer. We won't waste our time musing on other types.form_type_checkbox_values
and this function together does a very subtle and very tricky==
style check between the values listed in#default_value
and the keys in#options
becauseform_type_checkbox_values
copies the values of#default_value
into the keys of#value
and then uses anisset
for every key in#options
on#value
but that seems to convert freely between ints and strings. If the check does not match then the#default_value
becomes NULL, if it matches then it becomes the value originally listed in#options
. Summing up, forcheckboxes
the things listed in#default_value
is compared to the keys in#options
without taking type into consideration and copied over to thecheckbox
#default_value
if this check matches and the not-matching fact is matched by setting it to NULL._form_builder_handle_input_element
changes NULL to the empty string.#value
of an unchecked, derived-from-checkboxes singular checkbox will be the empty string if it's not submitted but it will 0 if it's submitted and it's still not checked. This contradicts the double-checked fact that unchecked checkbox has a#value
of 0. We can not fix this in D7, however.theme_checkbox
uses==
to compare#value
to#return_value
.Here is how a singular checkbox is interpreted if there is no submission:
theme_checkbox
Now, the same can be said if it's a checkboxes element and the #default_value is listed and #return_value is derived from the key of options. I will write tests and see what the above patches fix and try to come up with a minimal effect patch if necessary.
While #return_value is always an int or string the same can not be said for #default_value -- I am strongly inclined to say that the intention for FALSE and NULL is to be unchecked and the intention for TRUE is to be checked always. These almost always succeed already.
I would not mix radios in this mess, thanks.
Comment #58
chx CreditAttribution: chx commentedNote that there is one more problem -- the "final catch" in
_form_builder_handle_input_element
makes #value the empty string if #default_value is NULL and that means #default_value NULL matches #return_value ''. This IMO remains broken. A fix would require an exemption for checkbox there. Instead, I have exempted this case from my tests. So I present only 44 form tests instead of 45. If you run it w/o the form.inc fix and remove the text exemption then you get the five fails above.The above patches are wrong because of string casts. The submitted value is disregarded,
#return_value
is used, there is no casting.In one fell swoop, this solves #319483: FAPI checkboxes and radios need strengthening for XSS without harming the form API process.
Comment #59
chx CreditAttribution: chx commentedThis should work.
Comment #61
chx CreditAttribution: chx commentedSeriously? One exception in DrupalRenderTestCase? That thing renders a checkbox without going through form API :( i squarely refuse to support such things. #type radio in there already had a value, I have added a #value to the checkbox to make it pass.
Fixed the checkboxes_value comment.
Comment #62
chx CreditAttribution: chx commentedComment #64
chx CreditAttribution: chx commented#61: no_more_broken_checkboxen.patch queued for re-testing.
Comment #65
chx CreditAttribution: chx commentedNow that we have the tests to deal with it, the logic can be vastly simplified. Observe that the problems are in the line 0, TRUE, FALSE and NULL -- we do not need to care about the column then, these values have a specific meaning, namely TRUE is checked, the other three are unchecked. That results in the following little patch which is even simpler than the original if was.
Comment #66
chx CreditAttribution: chx commentedComment #67
chx CreditAttribution: chx commentedAnd finally, although the tests exempt the NULL problem, let's add that too to the special problem line. While in_array could be used too, I really like the spelled-out simplicity of this condition.
Comment #68
chx CreditAttribution: chx commentedwebchick asked whether 0 #return_value could be supported by changing the unchecked-on-submission value to NULL. Certainly possible. First we need to write a ton of tests to submit the forms and check what happens. Second, if #return_value can be 0 then we need to check the logic for this case. I think that #default_value 0 and '0' need to match #return_value 0 and '0' both... right now 0 does not match '0'. This is a decision to be made.
Comment #69
sunVery good start. Changes in this patch:
I'm going to try to change #default_value to NULL now. I basically agree with webchick that it's odd to have 0 as default. That said, form_builder() applies FALSE to all elements, so perhaps that should stay the default.
Comment #70
sunChanged #default_value to FALSE; still works.
Comment #71
sunIgnore #70, I was confused. Of course, it has to be NULL.
Comment #72
chx CreditAttribution: chx commentedThese patches are wrong and an abuse of #value_callback. I encourage using #process instead, in fact I am working on it.
Comment #73
sun- Changed #value to NULL for unchecked.
- Added form_process_checkbox() as #process callback to set #checked.
Comment #74
chx CreditAttribution: chx commentedWe now can handle 0 #return_value so that #options (foo, bar, baz) will work. The problem with NULL is handled too by changing the NULL to FALSE in form_checkbox_value which is just fine because the two are interchangeable -- neither can be a submitted value and both signal unchecked. Consequently, now we have 54 tests that pass.
The XSS handling belongs to pre render so that everyone overriding theme_checkbox is already safe.
So this is a straight continuation of my patch, I did not use any of sun's changes.
Edit: the test logic is also very straightforward now.
Comment #75
chx CreditAttribution: chx commentedRemoved an unnecessary #default_value from system.module.
Comment #77
effulgentsia CreditAttribution: effulgentsia commentedYou don't need this. The theme function calls element_set_attributes(), which gets output using drupal_attributes() which check_plain()'s all attribute values.
If you change this to if (!empty($element['#checked']), then you can remove the hunk in common.test, I think.
Please remove this (see above). There are some use-cases for rendering checkboxes (and other element types) without FAPI. For example, a mini-form that submits to a 3rd party site (e.g., sign up for newsletter).
Powered by Dreditor.
Comment #78
effulgentsia CreditAttribution: effulgentsia commentedx-post
Comment #79
effulgentsia CreditAttribution: effulgentsia commentedIf we can make #return_value=0 work, great. If not, I think an acceptable alternative would be to make form_process_checkboxes() cast the array index to a string when setting #return_value. In fact, that would be consistent with what form_process_radios() already does in HEAD, and should still let us have #options (foo, bar, baz).
Comment #80
chx CreditAttribution: chx commentedYes, that's what I figured that we string-cast 0 (but only 0, I think for minimal impact). Patch forthcoming. And now that the checked logic is inside FAPI and not the render layer, yes it's simple to revert the render test.
Comment #81
chx CreditAttribution: chx commentedAnother patch before turning in. We need some checkboxes element test too. effulgentsia, i checked what you say and i would believe that form_process_radios does a check plain on key to become the HTML value and then theme_radio calls drupal_attributes which check plains it again. That sounds a bug.
Comment #83
chx CreditAttribution: chx commentedAdded checkboxes tests for #options array('foo', 'bar', 'baz'). To give a summary of what this patch changes:
In short, nothing should break (hey, bot can you hear that? no breaks!) but now we can handle 0-based options. And we have tests. Like. A real ungodly lot of tests.
Comment #85
chx CreditAttribution: chx commentedThat was mighty silly, effulgentsia tells me that I added a #process to checkbox when it already had one *blush*
Comment #86
effulgentsia CreditAttribution: effulgentsia commentedI spent hours reviewing this and discussing with chx, and trying to write what I hope are semi-decent comments, so that no one else has to spend that much time making sense of this.
This patch contains only comment changes and whitespace cleanup, and trivial improvements to the tests (like changing assertEquals() to assertIdentical()). Therefore, RTBC.
Yay for the hundred or so new assertions related to checkboxes! The lack of these has been a major obstacle to refactoring other legacy cruft. In addition to all the new tests now passing, the Webform module has a bunch of tests that are failing, but that now pass with this patch (after this patch, the Webform has only a couple remaining test failures, unrelated to checkboxes, and trivial to fix).
Some of this issue's history has also tried to remove the workarounds (converting '0' to '_0' and back) that Field module employs to deal with what is now being fixed. That is left out of this patch, and can be done as a follow-up.
Comment #87
yched CreditAttribution: yched commentedWhen this gets in, we'll need to revisit / remove the workarounds that were added to options.module to support 0 with checkboxes.
[edit: er, sorry, eff already mentioned that at the end of his post above]
Comment #88
chx CreditAttribution: chx commentedIt has been decided that we roll those fixes in.
Comment #89
effulgentsia CreditAttribution: effulgentsia commentedOK. I just grepped for "_0", and started deleting code. Let's see what bot thinks.
Comment #90
yched CreditAttribution: yched commentedI'll wait for the bot, but the changes in options.module look good.
Comment #91
effulgentsia CreditAttribution: effulgentsia commentedFor anyone interested in testing this with Webform: #955158: Add the default body field to new D7 Webform installations.
Comment #92
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #93
yched CreditAttribution: yched commentedTestbot didn't run on #89 ?
Well, it seems some other patches were successfully tested since then, so I guess we're OK.
Thanks for the sprint on this one, folks !