Problem

  • Specifying a '#default_value' => 0 in a set of radio button #options using string keys checks all options.

Example

$form['myradios'] = array(
  '#type' => 'radios',
  '#options' => array(
    0 => '- None -',
    'one' => 'one',
    'two' => 'two',
    'three' => 'three',
  ),
  '#default_value' => 0,
);

Result:

<input id="edit-myradios-0" name="radios" value="0" checked="checked" class="form-radio" type="radio">
<input id="edit-myradios-one" name="radios" value="one" checked="checked" class="form-radio" type="radio">
<input id="edit-myradios-two" name="radios" value="two" checked="checked" class="form-radio" type="radio">
<input id="edit-myradios-three" name="radios" value="three" checked="checked" class="form-radio" type="radio">

Example of steps to reproduce in core:

  • Edit any node view
  • Add 'Published' filter
  • Tick 'Expose ...'
  • Select '- Any -' under 'Published status'
  • Save filter
  • Go to filter form again. 'No' is selected instead of '- Any -'.
CommentFileSizeAuthor
#147 1381140-147.patch8.84 KBLendude
#147 interdiff-1381140-145-147.txt1.24 KBLendude
#145 1381140-145.patch8.72 KBLendude
#145 interdiff-1381140-142-145.txt507 bytesLendude
#142 1381140-142.patch8.88 KBLendude
#142 interdiff-1381140-136-142.txt3.61 KBLendude
#141 1381140-139.patch822 bytesbapi_22
#136 1381140-136.patch7.2 KBLendude
#136 interdiff-1381140-129-136.txt4.15 KBLendude
#129 1381140-129.patch6.52 KBmkalkbrenner
#129 127-129-interdiff.txt1.36 KBmkalkbrenner
#127 1381140-127.patch6.06 KBLendude
#124 1381140-124.patch12.97 KBPavan B S
#123 1381140-123.patch6.88 KBtameeshb
#121 1381140-121.patch6.88 KBtameeshb
#118 interdiff-1381140-116-118.txt478 byteshimanshu-dixit
#118 1381140-118.patch6.93 KBhimanshu-dixit
#116 1381140-116.patch6.94 KBtameeshb
#116 interdiff-114-116.txt814 bytestameeshb
#114 interdiff-1381140-112-114.txt853 byteshimanshu-dixit
#114 1381140-114.patch6.98 KBhimanshu-dixit
#112 interdiff-1381140-109-112.txt841 byteshimanshu-dixit
#112 1381140-112.patch7.82 KBhimanshu-dixit
#109 interdiff-1381140-103-109.txt929 byteshimanshu-dixit
#109 1381140-109.patch7.82 KBhimanshu-dixit
#105 interdiff-1381140-103-105.txt577 byteshimanshu-dixit
#105 1381140-105.patch7.82 KBhimanshu-dixit
#103 interdiff-1381140-100-103.txt504 byteshimanshu-dixit
#103 1381140-103.patch7.78 KBhimanshu-dixit
#100 interdiff-1381140-96-100.txt775 byteshimanshu-dixit
#100 1381140-100.patch7.78 KBhimanshu-dixit
#96 1381140-94.patch7.26 KBtameeshb
#96 interdiff-93-94.txt833 bytestameeshb
#94 interdiff-87-93.txt2.18 KBtameeshb
#93 1381140-93.patch7.25 KBtameeshb
#87 fix-radio-default-zero-value-1381140-87.patch8.42 KBdan2k3k4
#81 Screenshot-3.png154.82 KBlomasr
#80 Screenshot-1.png162.92 KBlomasr
#77 fix-radio-default-zero-value-1381140-77.patch8.42 KBblazey
#73 fix-radio-default-zero-value-1381140-73.patch6.05 KBblazey
#73 interdiff.txt856 bytesblazey
#71 fix-radio-default-zero-value-1381140-71.patch7.59 KBLendude
#71 interdiff-1381140-69-71.txt1.88 KBLendude
#69 interdiff-1381140-66-69.txt3.13 KBLendude
#69 fix-radio-default-zero-value-1381140-69.patch7.63 KBLendude
#66 fix-radio-default-zero-value-1381140-66.patch6.52 KBLendude
#66 interdiff-1381140-63-66.txt1.25 KBLendude
#63 fix-radio-default-zero-value-1381140-63.patch6.52 KBLendude
#63 interdiff-1381140-60-63.txt2.39 KBLendude
#60 fix-radio-default-zero-value-1381140-60.patch6.5 KBLendude
#60 fix-radio-default-zero-value-1381140-60-TEST_ONLY.patch3.59 KBLendude
#60 interdiff-1381140-59-60.txt2.09 KBLendude
#59 fix-radio-default-zero-value-1381140-59.patch5.98 KBIRuslan
#54 fix-radio-default-zero-value-1381140-54.patch2.82 KBIRuslan
#52 fix-radio-default-zero-value-1381140-52.patch2.82 KBIRuslan
#50 fix-radio-default-zero-value-1381140-50.patch2.1 KBIRuslan
#48 fix-radio-default-zero-value-1381140-48.patch1.24 KBIRuslan
#45 drupal8.form-radio-default-test-1381140-45.patch15.56 KBceardach
FAILED: [[SimpleTest]]: [MySQL] 60,224 pass(es), 2 fail(s), and 1 exception(s). View
#45 interdiff.txt16.36 KBceardach
#41 drupal8.form-radio-default-test-1381140-41.patch4.3 KBbabruix
PASSED: [[SimpleTest]]: [MySQL] 53,209 pass(es). View
#39 drupal8.form-radio-default-test-1381140-39.patch3.79 KBbabruix
FAILED: [[SimpleTest]]: [MySQL] 53,148 pass(es), 1 fail(s), and 0 exception(s). View
#36 drupal8.form-radio-default-test-1381140-36.patch3.7 KBbabruix
FAILED: [[SimpleTest]]: [MySQL] 53,162 pass(es), 23 fail(s), and 0 exception(s). View
#31 drupal8.form-radio-default-test-1381140-31.patch2.68 KBbabruix
FAILED: [[SimpleTest]]: [MySQL] 52,593 pass(es), 1 fail(s), and 0 exception(s). View
#25 drupal8.form-radio-default-test-1381140-25.patch3.51 KBbabruix
FAILED: [[SimpleTest]]: [MySQL] 52,646 pass(es), 1 fail(s), and 0 exception(s). View
#24 drupal8-form-radios-default-1381140-24.patch2.26 KBesoteric1
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/tests/modules/form_test/form_test.module. View
#23 drupal8-form-radios-default-1381140-23.patch2.26 KBesoteric1
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/tests/modules/form_test/form_test.module. View
#21 drupal8-form-radios-default-1381140-21.patch766 bytestanmayk
FAILED: [[SimpleTest]]: [MySQL] 48,201 pass(es), 3 fail(s), and 0 exception(s). View
#17 drupal-1381140-17.patch3.36 KBdcam
FAILED: [[SimpleTest]]: [MySQL] 37,217 pass(es), 3 fail(s), and 0 exception(s). View
#10 drupal-1381140-10.patch3.26 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1381140-10.patch. Unable to apply patch. See the log in the details link for more information. View
#4 drupal8.form-radio-default.4.patch3.22 KBsun
FAILED: [[SimpleTest]]: [MySQL] 34,384 pass(es), 3 fail(s), and 0 exception(es). View
drupal8.form-radio-default.0.patch2.24 KBsun
FAILED: [[SimpleTest]]: [MySQL] 34,214 pass(es), 168 fail(s), and 107 exception(es). View
drupal8.form-radio-default.0.test-only.patch1.56 KBsun
FAILED: [[SimpleTest]]: [MySQL] 34,369 pass(es), 1 fail(s), and 0 exception(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Status: Needs review » Needs work

The last submitted patch, drupal8.form-radio-default.0.patch, failed testing.

xjm’s picture

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

JeremyFrench’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
3.22 KB
FAILED: [[SimpleTest]]: [MySQL] 34,384 pass(es), 3 fail(s), and 0 exception(es). View

After analyzing this further and fixing it, I'm relatively confident that I'm partially duplicating another issue in the queue, but who cares.

Status: Needs review » Needs work

The last submitted patch, drupal8.form-radio-default.4.patch, failed testing.

Damien Tournoud’s picture

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

xjm’s picture

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.

sun’s picture

Status: Needs work » Closed (duplicate)

Merged this one-line fix into #811542: Regression: Required radios throw illegal choice error when none selected, as it fixes the overall #value and #default_value handling of #type 'radios'.

sun’s picture

Status: Closed (duplicate) » Needs work
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1381140-10.patch. Unable to apply patch. See the log in the details link for more information. View

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

Status: Needs review » Needs work

The last submitted patch, drupal-1381140-10.patch, failed testing.

tim.plunkett’s picture

Issue tags: +Needs tests

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

chx’s picture

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.

cweagans’s picture

dcam’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -needs backport to D7

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

Status: Needs review » Needs work
Issue tags: +Needs tests, +needs backport to D7

The last submitted patch, drupal-1381140-10.patch, failed testing.

dcam’s picture

Status: Needs work » Needs review
FileSize
3.36 KB
FAILED: [[SimpleTest]]: [MySQL] 37,217 pass(es), 3 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, drupal-1381140-17.patch, failed testing.

sun’s picture

Priority: Major » Normal

Modules can easily work around this bug, so downgrading to normal.

dcam’s picture

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.

tanmayk’s picture

Status: Needs work » Needs review
FileSize
766 bytes
FAILED: [[SimpleTest]]: [MySQL] 48,201 pass(es), 3 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, drupal8-form-radios-default-1381140-21.patch, failed testing.

esoteric1’s picture

FileSize
2.26 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/tests/modules/form_test/form_test.module. View

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.

esoteric1’s picture

FileSize
2.26 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/tests/modules/form_test/form_test.module. View

Noticed a small coding standard violation. fixed it.

babruix’s picture

Status: Needs work » Needs review
FileSize
3.51 KB
FAILED: [[SimpleTest]]: [MySQL] 52,646 pass(es), 1 fail(s), and 0 exception(s). View

Fixed patch and added test part.

Status: Needs review » Needs work

The last submitted patch, drupal8.form-radio-default-test-1381140-25.patch, failed testing.

babruix’s picture

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

babruix’s picture

Status: Needs work » Needs review
babruix’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +needs backport to D7

The last submitted patch, drupal8.form-radio-default-test-1381140-25.patch, failed testing.

babruix’s picture

Status: Needs work » Needs review
FileSize
2.68 KB
FAILED: [[SimpleTest]]: [MySQL] 52,593 pass(es), 1 fail(s), and 0 exception(s). View

Trying another patch, removed condition

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

Status: Needs review » Needs work

The last submitted patch, drupal8.form-radio-default-test-1381140-31.patch, failed testing.

barneytech’s picture

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

chx’s picture

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) {
webmozart’s picture

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;
}
babruix’s picture

Status: Needs work » Needs review
FileSize
3.7 KB
FAILED: [[SimpleTest]]: [MySQL] 53,162 pass(es), 23 fail(s), and 0 exception(s). View

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

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

and should check

!empty($elements[0]['@attributes']['checked'])
chx’s picture

Issue tags: +Needs manual testing

Status: Needs review » Needs work
Issue tags: -Needs manual testing

The last submitted patch, drupal8.form-radio-default-test-1381140-36.patch, failed testing.

babruix’s picture

Status: Needs work » Needs review
FileSize
3.79 KB
FAILED: [[SimpleTest]]: [MySQL] 53,148 pass(es), 1 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, drupal8.form-radio-default-test-1381140-39.patch, failed testing.

babruix’s picture

Status: Needs work » Needs review
FileSize
4.3 KB
PASSED: [[SimpleTest]]: [MySQL] 53,209 pass(es). View

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.

Cottser’s picture

Assigned: sun » Unassigned
Status: Needs review » Needs work

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.

Lendude’s picture

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

heddn’s picture

Issue tags: +Needs reroll
ceardach’s picture

Issue tags: -Needs reroll
FileSize
16.36 KB
15.56 KB
FAILED: [[SimpleTest]]: [MySQL] 60,224 pass(es), 2 fail(s), and 1 exception(s). View

Updated patch so it applies cleanly to HEAD

heddn’s picture

Status: Needs work » Needs review

The last submitted patch, drupal8.form-radio-default-test-1381140-45.patch, failed testing.

IRuslan’s picture

Issue summary: View changes
Priority: Normal » Major
Status: Needs work » Needs review
FileSize
1.24 KB

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.

Status: Needs review » Needs work

The last submitted patch, 48: fix-radio-default-zero-value-1381140-48.patch, failed testing.

IRuslan’s picture

Status: Needs work » Needs review
FileSize
2.1 KB

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.

Status: Needs review » Needs work

The last submitted patch, 50: fix-radio-default-zero-value-1381140-50.patch, failed testing.

IRuslan’s picture

Status: Needs work » Needs review
FileSize
2.82 KB

Status: Needs review » Needs work

The last submitted patch, 52: fix-radio-default-zero-value-1381140-52.patch, failed testing.

IRuslan’s picture

Status: Needs work » Needs review
FileSize
2.82 KB
IRuslan’s picture

NickWilde’s picture

Looks good to me

Lendude’s picture

Status: Needs review » Needs work

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.

IRuslan’s picture

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

IRuslan’s picture

Status: Needs work » Needs review
FileSize
5.98 KB

Added tests.

Lendude’s picture

Looking good.

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

The last submitted patch, 60: fix-radio-default-zero-value-1381140-60-TEST_ONLY.patch, failed testing.

swentel’s picture

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

Lendude’s picture

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

cilefen’s picture

Title: A #default_value = 0 for #type radios checks all radios (Worst PHP Loose Data Typing Bug Ever) » A #default_value = 0 for #type radios checks all radios
gnuget’s picture

Status: Needs review » Needs work

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

Lendude’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Lendude’s picture

Issue tags: +VDC

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

Lendude’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 69: fix-radio-default-zero-value-1381140-69.patch, failed testing.

Lendude’s picture

Status: Needs work » Needs review
FileSize
1.88 KB
7.59 KB

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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

blazey’s picture

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

Status: Needs review » Needs work

The last submitted patch, 73: fix-radio-default-zero-value-1381140-73.patch, failed testing.

xjm’s picture

Issue tags: -VDC

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

Lendude’s picture

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

blazey’s picture

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’s picture

Status: Needs work » Needs review
Issue tags: +Dublin2016
Lendude’s picture

Issue summary: View changes

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

lomasr’s picture

FileSize
162.92 KB

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

lomasr’s picture

FileSize
154.82 KB

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.

blazey’s picture

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.

lomasr’s picture

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

suresh_ewall’s picture

Hi,

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

dan2k3k4’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -ChennaiSprint2017 +ChennaiSprint2017 SprintWeekend2017

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.

tstoeckler’s picture

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

dan2k3k4’s picture

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

tstoeckler’s picture

Looks good, thanks!

jp.stacey’s picture

cilefen’s picture

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

Status: Reviewed & tested by the community » Needs work

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.

tameeshb’s picture

Status: Needs work » Needs review
FileSize
7.25 KB

Changes from #92

tameeshb’s picture

FileSize
2.18 KB

Status: Needs review » Needs work

The last submitted patch, 93: 1381140-93.patch, failed testing.

tameeshb’s picture

tameeshb’s picture

Status: Needs work » Needs review
catch’s picture

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

Status: Needs review » Needs work

The last submitted patch, 96: 1381140-94.patch, failed testing.

himanshu-dixit’s picture

Status: Needs work » Needs review
FileSize
7.78 KB
775 bytes

Here is the new patch,

Status: Needs review » Needs work

The last submitted patch, 100: 1381140-100.patch, failed testing.

himanshu-dixit’s picture

Oops forgot to change $index to $value .

himanshu-dixit’s picture

Status: Needs work » Needs review
FileSize
7.78 KB
504 bytes

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

catch’s picture

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

himanshu-dixit’s picture

Status: Needs review » Needs work

The last submitted patch, 105: 1381140-105.patch, failed testing.

himanshu-dixit’s picture

Status: Needs work » Needs review

The last submitted patch, 103: 1381140-103.patch, failed testing.

himanshu-dixit’s picture

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

The last submitted patch, 105: 1381140-105.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 109: 1381140-109.patch, failed testing.

himanshu-dixit’s picture

Status: Needs work » Needs review
FileSize
7.82 KB
841 bytes

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 ?

Status: Needs review » Needs work

The last submitted patch, 112: 1381140-112.patch, failed testing.

himanshu-dixit’s picture

Status: Needs work » Needs review
FileSize
6.98 KB
853 bytes

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 :)

Lendude’s picture

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

tameeshb’s picture

Made changes from #115

Lendude’s picture

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.

himanshu-dixit’s picture

Fixed #117 in this patch.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

All feedback has been addressed, back to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 118: 1381140-118.patch, failed testing.

tameeshb’s picture

Status: Needs work » Needs review
FileSize
6.88 KB

array() -> []

Status: Needs review » Needs work

The last submitted patch, 121: 1381140-121.patch, failed testing.

tameeshb’s picture

Status: Needs work » Needs review
FileSize
6.88 KB
Pavan B S’s picture

Rerolled the patch, please review.

Lendude’s picture

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

Status: Needs review » Needs work

The last submitted patch, 124: 1381140-124.patch, failed testing.

Lendude’s picture

Status: Needs work » Needs review
FileSize
6.06 KB

Rerolled the reroll to not include unrelated stuff

mkalkbrenner’s picture

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.

mkalkbrenner’s picture

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.

sk33lz’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Baltimore2017

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.

HongPong’s picture

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

xjm’s picture

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.

xjm’s picture

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

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Lendude’s picture

Status: Needs work » Needs review
FileSize
4.15 KB
7.2 KB

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

Manuel Garcia’s picture

Status: Needs review » Needs work

The last submitted patch, 136: 1381140-136.patch, failed testing. View results

bapi_22’s picture

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’s picture

bapi_22’s picture

Lendude’s picture

Status: Needs work » Needs review
Issue tags: +blocker, +phpunit initiative
FileSize
3.61 KB
8.88 KB

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

The last submitted patch, 141: 1381140-139.patch, failed testing. View results

andypost’s picture

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

Lendude’s picture

@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

andypost’s picture

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

Lendude’s picture

Version: 8.5.x-dev » 8.4.x-dev
FileSize
1.24 KB
8.84 KB

@andypost so something like this maybe?

Bug fix so eligible for 8.4.x

andypost’s picture

Status: Needs review » Reviewed & tested by the community

IMO perfect!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 147: 1381140-147.patch, failed testing. View results

dcam’s picture

Status: Needs work » Reviewed & tested by the community

That failure looks unrelated.

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