Because of PHP's type juggling, "0" == "funky monkey". theme_radio says $element['#value'] == $element['#return_value'] which means that a #value of 0 and #return_value of "ice cream" would result in a "checked" radio button.

For a real-world use case see #1314418: Setting default to -Any- resets to No when Exposed filters enabled.

Attached is a patch that simply changes == to ===. I've also added tests, based on those added in #654796: Identify fix bugs with checkbox element.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bugfix
Issue priority Major
Unfrozen changes Unfrozen because it fixes a longstanding bug
Prioritized changes Yes, bug fixes
Disruption No
CommentFileSizeAuthor
#39 minor-nits-1333910-39.patch10.4 KBmartin107
#39 interdiff-1333910-37-39.txt2.72 KBmartin107
#37 radio_element_does_php-1333910-31-reroll.patch10.64 KBjoelpittet
#31 interdiff-29-31.txt942 bytesmartin107
#31 radio_element_does_php-1333910-31.patch10.73 KBmartin107
#29 radio_element_does_php-1333910-29.patch10.72 KBjoelpittet
#29 interdiff.txt3.55 KBjoelpittet
#24 theme_radio_does_php-1333910-24.patch12.63 KBjoelpittet
#24 theme_radio_does_php-1333910-24-tests-only.patch8.13 KBjoelpittet
#14 1333910-13-radio_button_type_juggling.patch9.49 KBmikeker
#14 1333910-13-interdiff.txt614 bytesmikeker
#12 1333910-12-radio_button_type_juggling.patch9.3 KBmikeker
#12 1333910-interdiff.txt1.28 KBmikeker
#11 1333910-11-radio_button_tests.patch8.01 KBmikeker
#4 1333910-4.patch4.46 KBmikeker
#2 1333910.patch3.48 KBmikeker

Comments

mikeker’s picture

mikeker commented

Crap... Git just barfed on me. I'll upload the promised patch as soon as I can.

mikeker’s picture

mikeker commented
Status: Active » Needs review
StatusFileSize
new 1333910.patch3.48 KB

Attached patch fixes the comparison issue and adds test

Status: Needs review » Needs work

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

mikeker’s picture

mikeker commented
Status: Needs work » Needs review
StatusFileSize
new 1333910-4.patch4.46 KB

Clearly it was too late last night for me to be submitting patches... After this morning's coffee is seems obvious that, as with #654796: Identify fix bugs with checkbox element, casting to string is the way to go.

As before, TRUE, 1, and '1' are equal, as are FALSE, 0, and '0'. My only concern is that (string)'' == (string)FALSE... Suppose you had a form similar to:

$form['published'] = array(
  '#type' => 'radios',
  '#options' => array('' => "Don't care", FALSE => "not published", TRUE => "published"),
  '#default_value' => '',
);

That currently results in both "Don't care" and "not published" being marked as checked though browsers only respect the last checked attribute in a radios group. The attached patch would only mark "Don't care" as checked.

mikeker’s picture

mikeker commented
Title: theme_radio doesn't use strict comparison when setting "checked" attribute » theme_radio does PHP type juggling when setting "checked" attribute
OnkelTem’s picture

OnkelTem commented

I've just faced the exact same problem when Better Exposed Filters were checking "No" where "All" was the case.
I initially replaced == with === but then found this issue which is indeed more elaborated.
Applying it to my setup now.

gaëlg’s picture

gaëlg
he/him
Primary language French
Location Lille, France
commented
Status: Needs review » Reviewed & tested by the community

It also worked for me.

David_Rothstein’s picture

David_Rothstein commented
Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D7

Needs to go in Drupal 8 first.

W.M.’s picture

W.M. commented

I confirm that as per Drupal 7.21 the patch under #4 (by mikeker) solved an issue while using BEF module. I do not know if the patch found its way into the most recent official releases of Drupal 7.

mikeker’s picture

mikeker commented

@Geir19, as per #8, this needs a patch for D8 (which I'm working on today as part of a D8 sprint) before it can be backported to D7.

mikeker’s picture

mikeker commented
Status: Needs work » Needs review
Issue tags: +MWDS2013
StatusFileSize
new 1333910-11-radio_button_tests.patch8.01 KB

Added the same type juggling tests for radio buttons as exist currently for checkboxes. (Also generalizes two checkbox-specific helper functions). Testbot should fail on the new radio button tests.

mikeker’s picture

mikeker commented
StatusFileSize
new 1333910-interdiff.txt1.28 KB
new 1333910-12-radio_button_type_juggling.patch9.3 KB

And the fix originally proposed in #4. Interdiff is from #11.

mikeker’s picture

mikeker commented

re: #MWDS2013 -- joining the sprint remotely from Seattle. :)

mikeker’s picture

mikeker commented
StatusFileSize
new 1333910-13-interdiff.txt614 bytes
new 1333910-13-radio_button_type_juggling.patch9.49 KB

Whoops, missed one.

Status: Needs review » Needs work

The last submitted patch, 1333910-13-radio_button_type_juggling.patch, failed testing.

mikeker’s picture

mikeker commented
Assigned: Unassigned » mikeker

Ran out of time in this sprint -- will come back to this in the next few days.

W.M.’s picture

W.M. commented

Thank you, I wish you good luck.

W.M.’s picture

W.M. commented
Issue summary: View changes

fixed bogus code filter issues

mgifford’s picture

mgifford
he/him
Primary language English
commented
Assigned: mikeker » Unassigned
Issue summary: View changes
mikeker’s picture

mikeker commented
Related issues: +#2165639: Patch must be applied to includes/form.inc to make BEF work properly

Marking #2165639: Patch must be applied to includes/form.inc to make BEF work properly as postponed on this.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Crusty jugglers!

rudiedirkx’s picture

rudiedirkx commented

Any chance this will ever be fixed in D7? It's been fixed for 3.5 years...

mikeker’s picture

mikeker commented

@rudiedirkx: This needs to be fixed in 8.x first and then backported. Until then, you'll need to keep applying the patch in #4 with each core update.

mikeker’s picture

mikeker commented
Issue tags: -MWDS2013 +Needs reroll

... and the first step would be a reroll against the latest 8.0.x HEAD.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Title: theme_radio does PHP type juggling when setting "checked" attribute » Radio element does PHP type juggling when setting "checked" attribute
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new theme_radio_does_php-1333910-24-tests-only.patch8.13 KB
new theme_radio_does_php-1333910-24.patch12.63 KB

Here's a re-roll and a fix all the things and update to latest way to deal with FAPI.

The tests pass locally but definitely needs a good review of the tests!

Added a tests only patch because I'm curious if this actually fixes anything in D8;)

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Priority: Normal » Major
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 24: theme_radio_does_php-1333910-24.patch, failed testing.

The last submitted patch, 24: theme_radio_does_php-1333910-24-tests-only.patch, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Well the 9 test failures are expected in the tests-only. The 40 test failures were not expected. With a manual test I am getting back what I'd expect.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt3.55 KB
new radio_element_does_php-1333910-29.patch10.72 KB

Probably don't need the value callback in there for radios. This may still fail, but hopefully less (my guess is by half)

Status: Needs review » Needs work

The last submitted patch, 29: radio_element_does_php-1333910-29.patch, failed testing.

martin107’s picture

martin107 commented
StatusFileSize
new radio_element_does_php-1333910-31.patch10.73 KB
new interdiff-29-31.txt942 bytes

Nothing of significance.

While reading up on this issue, I noticed 2 small cosmetic nits, so the error count will be unchanged.

No need to trigger testbot ....yet.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review

testbot, and thank you for the nits.

Status: Needs review » Needs work

The last submitted patch, 31: radio_element_does_php-1333910-31.patch, failed testing.

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.

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.

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.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review
StatusFileSize
new radio_element_does_php-1333910-31-reroll.patch10.64 KB

Rerolling

Status: Needs review » Needs work

The last submitted patch, 37: radio_element_does_php-1333910-31-reroll.patch, failed testing.

martin107’s picture

martin107 commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-1333910-37-39.txt2.72 KB
new minor-nits-1333910-39.patch10.4 KB

Again just minor nits found while reading along..

@file tags are deprecated.

Converted a few cases of t() into $this->t()

Status: Needs review » Needs work

The last submitted patch, 39: minor-nits-1333910-39.patch, failed testing.

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

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

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

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

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

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

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

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs work » Closed (duplicate)

This reads like a duplicate of #1381140: A #default_value = 0 for #type radios checks all radios which was fixed around the time development here stopped.

If you feel there is still work here that needs to be done, please feel free to re-open this.