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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

c960657’s picture

The $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.

webchick’s picture

Category: feature » bug
Priority: Normal » Critical

We need chx to take a look at this.

bleen’s picture

Just 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

Reg’s picture

subscribe

chx’s picture

I 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.

chx’s picture

Status: Needs review » Needs work

So yeah what this needs is a multistep example which checks that checkboxes w a 0 key can be ticked on/off. Also,

-    if (class_exists($class_name) && $value === 1) {
+    if (class_exists($class_name) && $value == 1) {

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'?

c960657’s picture

Status: Needs work » Needs review
FileSize
22.22 KB

I 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.

what this mean for 0 checkboxes, will the value be 0 or '0'... will the type change or will it be always '0'?

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'.

So yeah what this needs is a multistep example which checks that checkboxes w a 0 key can be ticked on/off.

Like this?

Status: Needs review » Needs work

The last submitted patch failed testing.

c960657’s picture

Status: Needs work » Needs review
FileSize
20.57 KB
yched’s picture

I'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 !

webchick’s picture

+++ modules/system/system.module	15 Dec 2009 17:36:00 -0000
@@ -396,19 +396,19 @@ function system_element_info() {
-    '#return_value' => 1,
+    '#return_value' => '1',

This 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?

webchick’s picture

Duh. 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'.

chx’s picture

Status: Needs review » Needs work

Consider 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).

c960657’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
20.86 KB

Added comments. Marking RTBC.

c960657’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
22.28 KB

Reroll. 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.

webchick’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -DrupalWTF

The last submitted patch failed testing.

Status: Needs work » Needs review

c960657 requested that failed test be re-tested.

Status: Needs review » Needs work

The last submitted patch, , failed testing.

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

Re-test of from comment #2385204 was requested by @user.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Tests pass - back to RTBC.

Status: Reviewed & tested by the community » Needs review

Re-test of checkboxes-zero-5.patch from comment was requested by webchick.

agentrickard’s picture

Status: Needs review » Reviewed & tested by the community

This was RTBC until a testbot glitch, AFAIK. Setting back.

Status: Reviewed & tested by the community » Needs review

Re-test of checkboxes-zero-5.patch from comment #15 was requested by sun.

sun’s picture

yched’s picture

Status: Needs review » Reviewed & tested by the community

Still green. Back to RTBC.

sun’s picture

Status: Reviewed & tested by the community » Needs review
+++ includes/form.inc	17 Dec 2009 18:54:41 -0000
@@ -1863,20 +1863,23 @@ function theme_fieldset($variables) {
 function theme_radio($variables) {
...
+  $output .= 'value="' . check_plain($element['#return_value']) . '" ';
+  // Cast operands to strings, because 0 == 'any string'.
+  if ((string)$element['#value'] == (string)$element['#return_value']) {
+    $output .= 'checked="checked" ';
+  }

@@ -2209,21 +2212,22 @@ function theme_text_format_wrapper($vari
 function theme_checkbox($variables) {
...
+  $checkbox .= 'value="' . check_plain($element['#return_value']) . '" ';
+  // Cast operands to strings, because 0 == 'any string'. An unchecked checkbox
+  // has #value of numeric 0.
+  if ($element['#value'] !== 0 && (string)$element['#value'] == (string)$element['#return_value']) {
     $checkbox .= 'checked="checked" ';
   }

I'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?

+++ modules/system/system.module	17 Dec 2009 18:54:43 -0000
@@ -396,19 +396,21 @@ function system_element_info() {
   $types['checkbox'] = array(
     '#input' => TRUE,
-    '#return_value' => 1,
+    // #return_value is a string, because that is what is received from the
+    // browser when the form is submitted.
+    '#return_value' => '1',

Is this change really/actually required? It seems like we're casting to string everywhere.

Powered by Dreditor.

c960657’s picture

FileSize
23.64 KB

This 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

Is this change really/actually required?

No, only for consistency and readability. The patch tries to clarify that the data type for radio and checkbox values is string.

c960657’s picture

FileSize
23.75 KB

Chasing HEAD.

Eric_A’s picture

Coming 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.

Eric_A’s picture

I think in this patch it should be:

     else {
       // Disabled form controls are not submitted by the browser. Ignore any
       // submitted value and always return default.
-      return $element['#default_value'];
+      return isset($element['#default_value']) && $element['#default_value'] !== 0 ? (string) $element['#default_value'] : 0;
     }
   }
 }
Eric_A’s picture

Hmm, 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.)

     else {
       // Disabled form controls are not submitted by the browser. Ignore any
       // submitted value and always return default.
-      return $element['#default_value'];
+      if (isset($element['#default_value'])) {
+         return $element['#default_value'] === 0 ? 0 : (string) $element['#default_value'];
+      }
     }
   }
}
effulgentsia’s picture

Status: Needs review » Needs work

Wow! 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:

+++ includes/form.inc	25 Jan 2010 18:02:28 -0000
@@ -2329,21 +2341,22 @@ function theme_text_format_wrapper($vari
+  // Cast operands to strings, because 0 == 'any string'. An unchecked checkbox
+  // has #value of numeric 0.

Remove this comment. That code is now handled by form_process_checked() and has no relevance to the theme function.

+++ includes/form.inc	25 Jan 2010 18:02:28 -0000
@@ -2398,20 +2411,21 @@ function form_process_checkboxes($elemen
+          '#name' => $element['#name'] . '[' . check_plain($key) . ']',

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.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
15.03 KB

On 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.

effulgentsia’s picture

FileSize
15.38 KB

With the following minor tweaks:

+++ includes/form.inc	19 Feb 2010 10:15:57 -0000
@@ -1964,6 +1964,28 @@ function theme_fieldset($variables) {
+  // For all other values, checked is determined by comparing #value and
+  // #return_value, without regard to type.
+  else {
+    $element['#checked'] = ($element['#value'] == $element['#return_value']);
+  }

Here we should cast to strings, even though it doesn't really matter, since we deal with 0 above.

+++ includes/form.inc	19 Feb 2010 10:15:57 -0000
@@ -2190,7 +2214,7 @@ function form_process_radios($element) {
-          '#return_value' => check_plain($key),
+          '#return_value' => (string)$key,

Add comment for why casting.

+++ includes/form.inc	19 Feb 2010 10:15:57 -0000
@@ -2398,8 +2421,8 @@ function form_process_checkboxes($elemen
-          '#return_value' => $key,
-          '#default_value' => isset($value[$key]) ? $key : NULL,
+          '#return_value' => (string)$key,
+          '#default_value' => isset($value[$key]) ? (string)$key : NULL,

Add comment for why casting.

This review is powered by Dreditor.

Eric_A’s picture


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?

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:

Is FAPI API (checkbox) #value (and #return_value) designed to basically hold an xhtml form input value string? Or can it be any PHP type, with the restriction that it must be serializable?

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?

effulgentsia’s picture

Title: Make #type => "checkboxes" support "0" as key » Identify and fix bugs when radio or checkbox element uses #return_value of string "0" or empty string.
Status: Needs review » Needs work
Issue tags: +Needs tests
+++ includes/form.inc	19 Feb 2010 11:12:43 -0000
@@ -1964,6 +1964,30 @@ function theme_fieldset($variables) {
+  // An empty value (NULL, FALSE, integer 0, and empty string) always means
+  // unchecked, except string '0', which we do not want to treat as empty in
+  // this context.
+  if (empty($element['#value']) && $element['#value'] !== '0') {
+    $element['#checked'] = FALSE;
+  }

Eric_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.

Eric_A’s picture

Note: 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.

 /**
+ * Initialize #checked on radio and checkbox elements.
+ */
+function form_pre_render_checked($element) {
+  // An empty value (NULL, FALSE, integer 0, and empty string) always means
+  // unchecked, except string '0', which we do not want to treat as empty in
+  // this context.
+  if (empty($element['#value']) && $element['#value'] !== '0') {
+    $element['#checked'] = FALSE;
+  }
+  // A value of TRUE always means checked.
+  elseif ($element['#value'] === TRUE) {
+    $element['#checked'] = TRUE;
+  }
+  // For all other values, checked is determined by comparing the string
+  // representations of #value and #return_value. We compare string
+  // representations, since that's what gets output to the HTML 'value'
+  // attribute.
+  else {
+    $element['#checked'] = ((string) $element['#value'] == (string) $element['#return_value']);
+  }
+  return $element;
+}
andypost’s picture

I 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?

fago’s picture

>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.

c960657’s picture

Doing so would allow us to keep the much simpler checks and thus keeps the code cleaner

I 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.

fago’s picture

Indeed - 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.

yched’s picture

re "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

Eric_A’s picture

Why 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.

fago’s picture

I 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.

YesCT’s picture

In 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.

effulgentsia’s picture

Issue tags: +D7 Form API challenge

This 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.

sun’s picture

marcingy’s picture

Priority: Critical » Major

Changing to major as per tag.

klonos’s picture

subscribing...

MustangGB’s picture

Tag update

klonos’s picture

(...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.

webchick’s picture

This 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.

  $form['checkboxes'] = array(
    '#type' => 'checkboxes',
    '#title' => t('Checkboxes'),
    '#options' =>  array(t('Banana'), t('Monkey'), t('Dishwasher')),
  );

This automatically defaults "Banana" to checked, even though I didn't specify a #default_value. This represents a regression from D6.

klonos’s picture

I'm taking it there then. Thanx for the info Angie ;)

bleen’s picture

@webchick: Most people use "foo" and "bar" and such as examples; I admire your creativity. Monkeys... :)

chx’s picture

Title: Identify and fix bugs when radio or checkbox element uses #return_value of string "0" or empty string. » Identify fix bugs with checkbox element

Now.

The identify part is missing. First, I have dissected the existing code. It is a mess. Let me list what I found.

  1. If there was a form submission, form_type_checkbox_value sets checkbox #value to the integer 0 and to #return_value if anything was posted. As we know, unchecked posts nothing.
  2. If there was no form submission, then #default_value gets copied over to #value.
  3. The #return_value is what gets into HTML as the attribute value, the call chain is theme_checkbox calling element_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.
  4. The default #return_value is 1 in system_elements.
  5. A comment in theme_checkbox corroborates with form_type_checkbox_value in stating that unchecked checkboxes are set to the integer 0.
  6. Now on to checkboxes. 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.
  7. 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.
  8. This bullet point and the next can be skipped it just explains the one after in painstaking detail. 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 because form_type_checkbox_values copies the values of #default_value into the keys of #value and then uses an isset 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, for checkboxes the things listed in #default_value is compared to the keys in #options without taking type into consideration and copied over to the checkbox #default_value if this check matches and the not-matching fact is matched by setting it to NULL.
  9. It must be noted that a "final catch" in _form_builder_handle_input_element changes NULL to the empty string.
  10. So we found that the #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.
  11. Most importantly, theme_checkbox uses == to compare #value to #return_value.

Here is how a singular checkbox is interpreted if there is no submission:

  #return_value
0 '0' ''
#default_value 0 Unsupported Unchecked due to specific check in theme_checkbox Unchecked (same)
'0' Unsupported Checked Unchecked
'' Unsupported Unchecked Checked
TRUE Unsupported Unchecked buggy. Unchecked buggy
FALSE Unsupported Checked, buggy Checked, buggy
NULL Unsupported Unchecked Checked buggy

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.

chx’s picture

Note 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.

chx’s picture

Status: Needs work » Needs review
FileSize
7.5 KB

This should work.

Status: Needs review » Needs work

The last submitted patch, no_more_broken_checkboxen.patch, failed testing.

chx’s picture

Seriously? 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.

chx’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Needs tests, -DrupalWTF, -D7 Form API challenge

The last submitted patch, no_more_broken_checkboxen.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +DrupalWTF, +D7 Form API challenge

#61: no_more_broken_checkboxen.patch queued for re-testing.

chx’s picture

Now 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.

chx’s picture

Issue tags: -Needs tests
chx’s picture

And 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.

chx’s picture

webchick 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.

sun’s picture

Very good start. Changes in this patch:

  1. Moved the logic for setting #checked into form_type_checkbox_value(), where it belongs. Without input processing, there is no input, so we don't care for an element's value outside Form API.
  2. Moved escaping of #return_value for HTML into theme_checkbox(), where it belongs.
  3. Removed the explicitly disabled test, because it works now.

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.

sun’s picture

Changed #default_value to FALSE; still works.

sun’s picture

Ignore #70, I was confused. Of course, it has to be NULL.

chx’s picture

These patches are wrong and an abuse of #value_callback. I encourage using #process instead, in fact I am working on it.

sun’s picture

- Changed #value to NULL for unchecked.

- Added form_process_checkbox() as #process callback to set #checked.

chx’s picture

We 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.

chx’s picture

Removed an unnecessary #default_value from system.module.

Status: Needs review » Needs work

The last submitted patch, no_more_broken_checkboxen.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
+++ includes/form.inc	2010-10-26 01:19:36 +0000
@@ -2790,6 +2797,16 @@ function form_process_radios($element) {
+function form_pre_render_checkbox($element) {
+  // As this ends up as the value of the HTML attribute 'value' it needs to be
+  // HTML escaped.
+  $element['#return_value'] = check_plain($element['#return_value']);
+  return $element;
+}

You don't need this. The theme function calls element_set_attributes(), which gets output using drupal_attributes() which check_plain()'s all attribute values.

+++ includes/form.inc	2010-10-26 01:19:36 +0000
@@ -2807,7 +2824,7 @@ function theme_checkbox($variables) {
+  if ($element['#checked']) {

If you change this to if (!empty($element['#checked']), then you can remove the hunk in common.test, I think.

+++ modules/simpletest/tests/common.test	2010-10-25 09:25:13 +0000
@@ -1533,6 +1533,7 @@ class DrupalRenderTestCase extends Drupa
     $element = array(
       '#type' => 'checkbox',
+      '#value' => TRUE,
       '#title' => $this->randomName(),
     );

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.

effulgentsia’s picture

Status: Needs review » Needs work

x-post

effulgentsia’s picture

We now can handle 0 #return_value so that #options (foo, bar, baz) will work.

If 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).

chx’s picture

Yes, 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.

chx’s picture

Status: Needs work » Needs review
FileSize
8.29 KB

Another 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.

Status: Needs review » Needs work

The last submitted patch, needs_checkboxes_test.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
11.37 KB

Added checkboxes tests for #options array('foo', 'bar', 'baz'). To give a summary of what this patch changes:

  1. The superb edge case of singular #type checkbox elements had all sorts of bugs for #default_value NULL, TRUE and FALSE and #return_value of '0' and ''. This is now fixed and the fix has a truckload of test cases.
  2. For #type checkboxes elements, previously, I believe, although I have not verified this, having #options array(15 => 'banana', '15monkeys' => 'bananas') and #default_value of array(15) resulted in both checkboxes being checked. (Once again, what sort of demented mind uses keys like that?)
  3. If #options had a key of 0 that did not work well. Now it does. But be superb careful! #value[0] is 0 for unchecked in this case and '0' for checked. YUCK.

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.

Status: Needs review » Needs work

The last submitted patch, no_more_broken_checkboxen.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
11.44 KB

That was mighty silly, effulgentsia tells me that I added a #process to checkbox when it already had one *blush*

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
12.72 KB

I 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.

yched’s picture

When 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]

chx’s picture

Status: Reviewed & tested by the community » Needs work

It has been decided that we roll those fixes in.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
17.51 KB

OK. I just grepped for "_0", and started deleting code. Let's see what bot thinks.

yched’s picture

I'll wait for the bot, but the changes in options.module look good.

effulgentsia’s picture

For anyone interested in testing this with Webform: #955158: Add the default body field to new D7 Webform installations.

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. Thanks.

yched’s picture

Testbot 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 !

Status: Fixed » Closed (fixed)
Issue tags: -DrupalWTF, -D7 Form API challenge

Automatically closed -- issue fixed for 2 weeks with no activity.