A #default_value = 0 for #type radios checks all radios

Maybe there is a string vs. int issue going on? 0 == '0' is true but 0 === '0' is false.

I think I have been caught out by this in the past.

Obviously the work around is to rename the key of your default value to something other than 0.

Please keep this issue focused. The change in _form_validate() is unrelated (and untested).

Re: #6 -- I think the change in _form_validate() is necessary for this change to work? It eliminates hundreds of test failures for the original change.

Rerolling after the test files moved. I didn't make any other changes.

This needs more tests, the current tests without the patch pass.

Thanks for fixing this annoying problem. However, my gut reaction is that anyone patching existing tests in form_test_checkboxes_radios needs to explain himself quite throughly and I do not see that anywhere in the issue. Adding new tests is always fine but why are you changing existing ones? We broke checkboxes enough times that it haunts me six years later so having like a hundred tests in place to keep them working makes me sleep better and changing those tests makes me twitchy.

Fixing tags per http://drupal.org/node/1517250

#10: drupal-1381140-10.patch queued for re-testing.

Rerolled #10 after test files moved. No other changes. My first attempt at rerolling a patch.

The cause of the failures appears to be that PDO always fetches values as strings. The data type check introduced by the patch causes the test failures when comparing an integer value to the string equivalent that was fetched.

I can think of a couple of ways to work around this, either converting all values to strings or all fetched values to the correct type, but I'm not sure what the recommended approach is. Maybe someone else has a better idea. I'd like to write a patch, but I could use some other opinions.

This is my first patch for core. It worked as expected but not done any changes with form tests.

I couldnt apply the last patch. here is a patch containing the modification from #21 and also a a hook_menu and form builder for the test. I didnt have time to write the test part.

Noticed a small coding standard violation. fixed it.

Fixed patch and added test part.

For some reason test OptionsWidgetsTest.php fails, however on local it passed tests. Any ideas?

#25: drupal8.form-radio-default-test-1381140-25.patch queued for re-testing.

Trying another patch, removed condition

$element['#return_value'] === $element['#default_value']

For your convenience, here is where the patch is failing: http://api.drupal.org/api/drupal/core%21modules%21system%21lib%21Drupal%... on line 98 according to the test bot

Here is what you need to do in order to fix this.

Consider everything that is valid as an array index. Consider what it will become during POST. Consider what is needed to match the two together. Your inspiration for a solid test is core/modules/system/lib/Drupal/system/Tests/Form/CheckboxTest.php :

    // Ensure that the checked state is determined and rendered correctly for
    // tricky combinations of default and return values.
    foreach (array(FALSE, NULL, TRUE, 0, '0', '', 1, '1', 'foobar', '1foobar') as $default_value) {
      // Only values that can be used for array indices are supported for
      // #return_value, with the exception of integer 0, which is not supported.
      // @see form_process_checkbox().
      foreach (array('0', '', 1, '1', 'foobar', '1foobar') as $return_value) {

We were hit by this problem in the Symfony2 Form component too. Our solution lies in the classes ChoiceList and SimpleChoiceList.

Basically, you have to convert any choice value that is entering your choice field logic (this applies for radio fields, checkboxes and drop downs) from either the application (through #default_value) or the browser (through a form submission) to a valid array key and then use strict comparison to compare it with the array keys in your #options array. We use the following function for choice value conversion:

protected function fixIndex($index)
{
    if (is_bool($index) || (string) (int) $index === (string) $index) {
        return (int) $index;
    }

    return (string) $index;
}

Looks problem was in WebTestBase->assertFieldChecked() method,
it checked

!empty($elements[0]['checked'])

and should check

!empty($elements[0]['@attributes']['checked'])

Ok, added fix in form.inc.
Similar issue described in Default check values in tableselect aren't set properly for a 0-based array so probably will combine these two patches into one.

Thanks for moving this forward @babruix! Found some tweaks while reviewing the patch in #41.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -2797,6 +2797,7 @@ protected function assertNoFieldById($id, $value = '', $message = '', $group = '
+//    $element_to_check = isset($elements[0]['@attributes']) ? $elements[0]['@attributes'] : $elements[0];

This change doesn't seem right - I think WebTestBase.php should be left as is.

+++ b/core/modules/system/tests/modules/form_test/form_test.moduleundefined
@@ -1632,6 +1638,24 @@ function form_test_checkboxes_radios($form, &$form_state, $customize = FALSE) {
+/**
+ * Form constructor for testing whether settings a '#default_value' => 0 in ¶
+ * a set of radio button #options using string keys checks all options.
+ * @ingroup forms
+ */

This will need a more complete docblock per https://drupal.org/node/1354#forms.

A similair issue to the one here (and caused by the same line of code), is that currently if you have an #options array containing the key 0, it will always be checked='checked' if the #default_value is a string.

$form['bugged_radios'] = array(
    '#type' => 'radios',
    '#title' => 'radios',
    '#options' => array(
      'bla' => 'bla',
      '1' => 'One',
      '0' => 'Null',
    ),
    '#default_value' => 'bla',
  );

will result in a set of buttons with both 'bla' and 0 checked.

Using === will solve this, but will lead to other issues because of PHP automatically casting numeric keys to (int) and all submitted values being strings.

$form['bugged_radios'] = array(
    '#type' => 'radios',
    '#title' => 'radios',
    '#options' => array(
      'bla' => 'bla',
      '1' => 'One',
      '0' => 'Null',
    ),
    '#default_value' => '0',
  );

So this will lead to a rendered set of buttons with none selected when using ===.
(An example of this behaviour can be seen in the default node view included in views in the D8 core, with the Published filter handler.)

My current D7 solution is to cast both #value and #return_value to strings in the comparisson

if (isset($element['#return_value']) && $element['#value'] !== FALSE && (string) $element['#value'] == (string) $element['#return_value']) {
    $element['#attributes']['checked'] = 'checked';
  }

Does the trick for me (so far).

Len

Updated patch so it applies cleanly to HEAD

Unlike @sun in #19, I think it's a pretty important thing to be fixed because even Drupal core affected by this problem (see #43, for D7 Views is affected correspondingly).
The problem still is not fixed since D7, and most probably from D6 versions.

Even more, in addition to case in the issue description, next case also will lead to error (both elements will have checked attribute, and browser will highligth the last one):

$types = array('text' => 'Text', 0 => 'Zero');
$form['radios'] = array(
  '#type' => 'radios',
  '#title' => 'Some radios element',
  '#default_value' => 'text',
  '#options' => $types,
);

From my point of view solution in all previous patches fixes only one usage of that problem, meanwhile all usages of 'radio' element affected (it's at least 'radios' and table select and all contributed modules which use 'radio' directly).
Most important for me that during a development of contributed modules it's not evident to handle such case. As a result, new silent errors with potential data loss will be introduced into projects.
That's why I propose to fix it directly in the rendering of the 'radio' element.

Patch in the attachment.

Actually the failed patch revealed another defect of Views module.
As we could see in DisplayPluginBase->buildOptionsForm():
$options = array(FALSE => $this->t('None'), 'custom_url' => $this->t('Custom URL'));

But it's incorrect to use FALSE as an array key, it's always will be converted to int 0.
That's why it didn't match to an empty default value.
It does not introduce bug itself, but logically it's incorrect to have key 0, and except it to be default in case of FALSE in #default_value.
I've rechecked similar places within Views module(i.e. Drupal\views\Plugin\views\style\Table->buildOptionsForm()) and here we see:
'#options' => array('' => $this->t('None'))
And it's working well.

Looks good to me

Great to see somebody taking another look at this! Nice work @IRuslan.

+++ b/core/modules/views_ui/src/Tests/DisplayTest.php
@@ -168,7 +168,7 @@ public function testLinkDisplay() {
+    $this->assertFieldChecked('edit-link-display-');

Are we ok with ending up with an id looking like that (I'm not really)? Not sure why this change has influence on the id being generated, but that looks ugly to me.

This still needs tests, for starters see #34 and other cases spread throughout this issue.

@Lendude, we changed the option key for radios element, it leads to changed HTML id (it's based on radios element name + option key).

Yep, tests are required for this changes, I'll try to add them a bit later.

Added tests.

Looking good.

Added another test scenario that fails without the patch. And added test-only patch to illustrate the problem.

Two nitpicks.

1. +++ b/core/lib/Drupal/Core/Render/Element/Radio.php
   @@ -61,8 +61,19 @@ public static function preRenderRadio($element) {
   +      // To avoid auto-casting during '==' we convert int 0 to '0' for both operands.
   +      // It will prevent wrong true-checking for both cases: 0 == 'string' and 'string' == 0.
   +      if ($element['#value'] === 0) {
   

   over 80 chars

2. +++ b/core/modules/views_ui/src/Tests/DisplayTest.php
   @@ -168,7 +168,8 @@ public function testLinkDisplay() {
   +    // We are looking to be selected a None option with '' key, that's why there is no key in the ID.
   

   same here

Fixed nitpicks and in the case of 2. turned it into an actual English sentence.

Two small nitpicks.

1. +++ b/core/lib/Drupal/Core/Render/Element/Radio.php
   @@ -61,8 +61,20 @@ public static function preRenderRadio($element) {
   +      if ($element['#value'] === 0) {
   +        $element['#value'] = '0';
   +      }
   +      if ($element['#return_value'] === 0) {
   +        $element['#return_value'] = '0';
   +      }
   +
   +      if ($element['#value'] == $element['#return_value']) {
   +        $element['#attributes']['checked'] = 'checked';
   +      }
   

   Here we have an extra new line when the second if is closed, but we haven't it in the first nor in the third one.

2. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestRadiosCheckedForm.php
   @@ -0,0 +1,67 @@
   +      '#default_value' => 0
   

   missing comma

fixed nitpicks

Tagging for VDC because it blocks #2459289: Boolean default values are not saved from working properly.

Ran into some more fun scenarios in #2459289: Boolean default values are not saved (boolean default values) that were still failing with the version of the patch in #66.

Attempt at a fix and more tests.

Ok, so FALSE is a reserved 'empty' value, lets just cast TRUE to 1 then.

Patch from #71 works for me. Attaching unit test covering Drupal\Core\Render\Element\Radio::preRenderRadio.

Apparently it no longer blocks #2459289: Boolean default values are not saved because that issue was fixed?

@xjm well I just wrote the tests for #2459289: Boolean default values are not saved to circumvent this issue. This issue still stops you from properly seeing which value you have selected in a boolean filter. And this issue caused the TEST ONLY patch to pass where it should have failed (see #28 there).

The Views UI is the only place I know in core that is actually effected by this issue, hence the VDC tag.

Readded missing form class.

This issue is still affecting Views. To reproduce:

1. Edit any node view
2. Add 'Published' filter
3. Tick 'Expose ...'
4. Select '- Any -' under 'Published status'
5. Save filter
6. Go to filter form again. 'No' is selected instead of '- Any -'.

@blazey thanks for the extra test coverage and added your steps to the issue summary.

I think this looks great, but I wrote a bit too much of it to RTBC this.

Reproduced the issue on my Local . Please see screen shot . '-Any-' & 'No' both are checked.

Applied the patch #77 . Now '-Any-' is only checked. But getting error 'You must select a value unless this is an non-required exposed filter. ' . Please see the screenshot.

Thanks for screenshots. It's not a bug, it's a feature ;). 'Any' cannot be chosen as a default value for required filter. Deselect 'Required' and the error will go away.

You are right .I didn't notice it. #77 works

Hi,

We have applied the patch comment#77. We didn't see it will work. We also facing an same error.

Thanks @blazey, I've read through the code first, and it makes sense.

Tested with simplytest with the steps to reproduce and seems to work fine.

You can't select '- Any -' for a required value but it works fine if you deselect the required checkbox for the value of the exposed filter.

+++ b/core/lib/Drupal/Core/Render/Element/Radio.php
@@ -54,8 +54,26 @@ public static function preRenderRadio($element) {
+    if (isset($element['#return_value']) &&  $element['#value'] !== FALSE) {

There is a double space in this line.

Leaving RTBC under the assumption that can be fixed on commit.

Ah good catch @tstoeckler, I've updated the patch for you @blazey :)

Looks good, thanks!

Fixing tags

Thanks for getting this to RTBC! Couple of things though:

1. +++ b/core/lib/Drupal/Core/Render/Element/Radio.php
   @@ -54,8 +54,26 @@ public static function preRenderRadio($element) {
   +      if ($element['#value'] === 0) {
   +        $element['#value'] = '0';
   +      }
   +      elseif ($element['#value'] === TRUE) {
   +        $element['#value'] = '1';
   +      }
   +      if ($element['#return_value'] === 0) {
   +        $element['#return_value'] = '0';
   +      }
   +      elseif ($element['#return_value'] === TRUE) {
   +        $element['#return_value'] = '1';
   +      }
   

   This is the same logic repeated twice, we could add a protected static helper method and do something like

    $element['#value'] = static::castValue($element['#value'];
   $element['#return_value'] = static::castValue($element['#return_value'];
   

   Similar to #1381140-35: A #default_value = 0 for #type radios checks all radios.

2. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestRadiosCheckedForm.php
   @@ -0,0 +1,87 @@
   +
   +/**
   + * @file
   + * Contains \Drupal\form_test\Form\FormTestRadiosCheckedForm.
   + */
   +
   

   This isn't necessary any more, can just remove the @file (and it'll fail coding standards automated checks).

3. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -1702,7 +1702,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   -        $options = array(FALSE => $this->t('None'), 'custom_url' => $this->t('Custom URL'));
   +        $options = array('' => $this->t('None'), 'custom_url' => $this->t('Custom URL'));
   

   Why does this have to change? Could it break contrib/custom code doing similar?

   If it's not necessary here, we could make the change in a follow-up.

Changes from #92

This will need to add the castValue() method, it doesn't exist yet I was just suggesting one.

Here is the new patch,

Oops forgot to change $index to $value .

This should pass tests. But i wonder if i have documented the helper function right?

It converts both (int) 0 and FALSE to '0' so it should be @param (mixed) $value I think.

Similarly the function description should probably be something like "Ensures $element['#default_value'] and $element['#value'] are consistently typed."?

Here is the new patch.

Okay, I guess we can use filter_var to check if $value is bool .

This should definitely work. Also changed the documentation to @return mixed since we are returning mixed values from castValue(). Here is the new patch though i am still not sure about the description of @param and @return . Any suggestions ?

Ok, removed the test from DisplayTest.php . I think it's better to create a follow-up for the change in #50 . @catch, Can you review this ? This time i am sure this would pass the tests :)

Couple of nitpicks:

1. +++ b/core/lib/Drupal/Core/Render/Element/Radio.php
   @@ -37,6 +37,24 @@ public function getInfo() {
   +  * Ensures $element['#default_value'] and $element['#value'] are consistently
   +  * typed.
   

   This is not what the function does. 'Casts passed boolean and integer values to strings'

2. +++ b/core/lib/Drupal/Core/Render/Element/Radio.php
   @@ -37,6 +37,24 @@ public function getInfo() {
   +  *   A value which would be casted.
   

   'A value to cast.'

3. +++ b/core/lib/Drupal/Core/Render/Element/Radio.php
   @@ -37,6 +37,24 @@ public function getInfo() {
   +  *   The casted value of the parameter.
   

   casted => cast

4. +++ b/core/lib/Drupal/Core/Render/Element/Radio.php
   @@ -37,6 +37,24 @@ public function getInfo() {
   +  protected static function castValue($value)
   +  {
   

   { needs to be on the same line as the function declaration

Made changes from #115

Almost there I think!

+++ b/core/lib/Drupal/Core/Render/Element/Radio.php
@@ -38,17 +38,15 @@
+  * Casts passed boolean and integer values to strings typed.

strings typed? 'to strings' or 'to string type' I'd say.

Fixed #117 in this patch.

All feedback has been addressed, back to RTBC

array() -> []

Rerolled the patch, please review.

@Pavan B S you included all sorts of things from a different patch...looks like the states javascript test....

Rerolled the reroll to not include unrelated stuff

I solved a related issue slightly different for us in #2712131: D8: Views: UI: -Any- option for non-required boolean yes/no filter won't stick.
One of the issues should be declared as duplicate and include the other use-case, too.

I added the use-case from #2712131: D8: Views: UI: -Any- option for non-required boolean yes/no filter won't stick to the tests to see if the patch here solves that as well.

The patch in #129 applies cleanly to 8.4.x and fixes the issue of selecting "-Any-" then saving the form to find it set to "No" when editing again. The "-Any-" setting stays selected when editing the filter again after applying #129. This allows the exposed filter can be used to view all content, published content, or unpublished content.

This looks RTBC after manual tests.

Confirming #129 fixed my issue on 8.3.2 as well. RTBC I would think, let's please get this onto the 8.3 series as well as 8.4 as it is a serious issue.

1. +++ b/core/lib/Drupal/Core/Render/Element/Radio.php
   @@ -37,6 +37,22 @@ public function getInfo() {
   + /**
   +  * Casts passed boolean and integer values to string type.
   +  *
   +  * @param mixed $value
   +  *   A value to cast.
   +  *
   +  * @return mixed
   +  *   The cast value of the parameter.
   +  */
   +  protected static function castValue($value) {
   +    if (is_bool($value) ||  (int) (bool) $value === (int) $value) {
   +      return (string) (int) $value;
   +    }
   +    return $value;
   +  }
   

   So this doesn't actually do what it says - it casts bools and ints that can be interpreted as bools - ie. 0 or 1 to string afaics.

2. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestRadiosCheckedForm.php
   @@ -0,0 +1,92 @@
   +        0 => 'Zero',
   ...
   +        1 => 'One',
   ...
   +        0 => 'Zero',
   ...
   +        1 => 'One',
   ...
   +        1 => 'True',
   +        0 => 'False',
   ...
   +        1 => 'True',
   +        0 => 'False',
   ...
   +        1 => 'True',
   +        0 => 'False',
   

   It would be great to see other integers tested and all integers as strings tested.

3. diff --git a/core/lib/Drupal/Core/Render/Element/Radio.php b/core/lib/Drupal/Core/Render/Element/Radio.php
   index d7f2607..c52d44f 100644
   --- a/core/lib/Drupal/Core/Render/Element/Radio.php
   +++ b/core/lib/Drupal/Core/Render/Element/Radio.php
   @@ -71,13 +71,13 @@ public static function preRenderRadio($element) {
        Element::setAttributes($element, ['id', 'name', '#return_value' => 'value']);
    
        if (isset($element['#return_value']) && $element['#value'] !== FALSE) {
   -      // To avoid auto-casting during '==' we convert int 0 to '0' for both
   -      // operands. It will prevent wrong true-checking for both cases: 0 ==
   -      // 'string' and 'string' == 0.
   -      $element['#value'] = static::castValue($element['#value']);
   -      $element['#return_value'] = static::castValue($element['#return_value']);
   +//      // To avoid auto-casting during '==' we convert int 0 to '0' for both
   +//      // operands. It will prevent wrong true-checking for both cases: 0 ==
   +//      // 'string' and 'string' == 0.
   +//      $element['#value'] = static::castValue($element['#value']);
   +//      $element['#return_value'] = static::castValue($element['#return_value']);
    
   -      if ($element['#value'] == $element['#return_value']) {
   +      if ((string) $element['#value'] == (string) $element['#return_value']) {
            $element['#attributes']['checked'] = 'checked';
          }
        }
   

   Also if I simply just cast both to a string and do the comparison the tests pass... so either the test is not doing what we think it is or a simpler fix is possible.

Inline comments for the renamed castValue() would also be ducky because it certainly raised my eyebrows. For example, as I noted to the committers, the (int) cast is to ensure it's not 'octopus' and the (string) cast is to ensure it's '0' or '1'. I guess? But let's explicitly document and explain the "why" of each bit of that line.

Maybe it's about NULL rather than 'octopus', which is less exciting. (But then what about 'octopus'?) In any case it clearly needs documentation. :)

#132.1 removed method, see #132.3
#132.2 added some ints as strings and other ints then 0 and 1
#132.3 Lets make it simple then, unless somebody can come up with a test that breaks it (I couldn't). Also nice to see that I had the same thought back in #43 (4 years ago....), but I never rolled it into a patch, silly me.

I believe this is blocking #2864610: Convert ExposedFormUITest in views_ui module to BrowserTestBase =)

Since the Options keys are coveted to string after process form elements, So we have to cast the $element['#value'] to string before compare with $element['#return_value']. That will solve the problem.

public static function preRenderRadio($element) {
    $element['#attributes']['type'] = 'radio';
    Element::setAttributes($element, ['id', 'name', '#return_value' => 'value']);

    if (isset($element['#return_value']) && $element['#value'] !== FALSE && (string)$element['#value'] == $element['#return_value']) {
      $element['#attributes']['checked'] = 'checked';
    }
    static::setAttributes($element, ['form-radio']);

    return $element;
  }

@bapi_22 please try to continue work on existing patches and not just try to start from scratch and remove all the test coverage we have already added. As you can see in #136, that alone won't be sufficient, there are special conditions to consider.

We need to account for people setting the default to an array, which is not valid for a radio button, but people do it anyway, see #2900911: \Drupal\workflows\Form\WorkflowTransitionAddForm::form sets a default value for radios as an empty array.
Also we need to account for people setting keys to FALSE. (as done in \Drupal\views\Plugin\views\display\DisplayPluginBase, will open a follow up for that too)

My solution would be to just say that empty() == empty(), that should clear up most weird configurations.

Added test coverage for these two scenarios.

@Lendude Thanx for working on that! Btw why not just compare values instead of casting them to strings?
I mean $element['#value'] === $element['#return_value'])

+++ b/core/modules/system/src/Tests/Form/ElementTest.php
@@ -89,9 +89,49 @@ public function testOptions() {
-  public function testWrapperIds() {
+  public function ptestWrapperIds() {

is that intentional?

@andypost whoops little debugging code left in

Btw why not just compare values instead of casting them to strings?

We need the casting to make sure string to int comparisons go right, since array keys will always be ints and submitted values will always be strings

Looks great, I think better to extend comment about "submitted values will always be strings"

@andypost so something like this maybe?

Bug fix so eligible for 8.4.x

IMO perfect!

That failure looks unrelated.

I'd love to see the first core patch I ever worked on finally get committed!

reroll needed after #2867154: Form: Convert system functional tests to phpunit landed

This converts \Drupal\Tests\system\Functional\Form\ElementTest::testRadiosChecked to a proper BrowserTest, that should clear up the fails

Thanx a for reroll

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!