backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 39 forks source link

#default_value may or may not work in #options for radios depending on PHP version, option order, and/or key of 0 #6024

Open bugfolder opened 1 year ago

bugfolder commented 1 year ago

Description of the bug

In checkboxes and radios, if the #options property contains a key of 0 and the #default_value is set to a different option, whether that default value is applied depends on (a) the order of the options, (b) the PHP version.

To test this, put this code into a form and render it:

  $types = array(
    'select' => t('Select'),
    'radios' => t('Radios'),
    'checkboxes' => t('Checkboxes'),
  );
  $default_values = array(
    'select' => 'foo',
    'radios' => 'foo',
    'checkboxes' => array('foo'),
  );
  foreach ($types as $type => $name) {
    $form[$type] = array(
      '#type' => 'fieldset',
      '#title' => $name,
    );
    $form[$type]["{$type}_1"] = array(
      '#type' => $type,
      '#title' => t('Group 1'),
      '#options' => array(
        'foo' => t('Foo option'),
        0 => t('Zero option'),
      ),
      '#default_value' => $default_values[$type],
    );
    $form[$type]["{$type}_2"] = array(
      '#type' => $type,
      '#title' => t('Group 2'),
      '#options' => array(
        0 => t('Zero option'),
        'foo' => t('Foo option'),
      ),
      '#default_value' => $default_values[$type],
    );
  }

Here's the result under PHP 7.4.33. Note that in every case, 'foo' is the #default_value.

options

If the 0 option is first, #default_value doesn't work in radios. If the 0 option is second, it works properly in radios. Selects and checkboxes all work.

That was in PHP 7.4. It works properly in PHP 8.1. Here's a summary of PHP versions I tested:

So the problem can be avoided by either (a) not putting your 0 key first, or (b) using an empty string, rather than 0, as the key. But it would be nice if the problem wasn't there, as it's $#@!! puzzling when it crops up (e.g., https://github.com/backdrop-contrib/ubercart/issues/436#issuecomment-1464941747).

Additional information

Add any other information that could help, such as:

klonos commented 1 year ago

Another interesting problem! 😅 ...good find @bugfolder 👍🏼

I will try to find time to reproduce this, but it seems that we need tests for it. In the meantime, I have some questions:

bugfolder commented 1 year ago

Some investigation. Using this as a simpler test case:

  $form['radios_1'] = array(
    '#type' => 'radios',
    '#title' => t('Radios 1'),
    '#options' => array(
      'foo' => t('Foo option'),
      0 => t('Zero option'),
    ),
    '#default_value' => 'foo',
  );
  $form['radios_2'] = array(
    '#type' => 'radios',
    '#title' => t('Radios 2'),
    '#options' => array(
      0 => t('Zero option'),
      'foo' => t('Foo option'),
    ),
    '#default_value' => 'foo',
  );

Under PHP 7.4, examining the HTML reveals that in both cases, ALL of the radio button <input> elements have the checked="checked" attribute, so always the second button is the one that's shown as checked, but the real problem is that they ALL have the checked attribute, not just the selected one.

The difference arises in theme_radio(), which has these lines:

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

For the "Zero option" radio buttons, $element['#value'] is 0, $element['#return_value'] is 'foo'. So one might expect that that last comparison would fail, except for this:

https://stackoverflow.com/questions/672040/comparing-string-to-integer-gives-strange-results

Which basically says that doing a comparison between a numeric 0 and a string 'foo' converts the latter to an integer for the comparison, resulting in 'foo' getting converted to the value 0, and so the equality evaluates to TRUE.

Forcing that comparison to convert both to strings (like this) fixes the issue, at least in this test case:

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

Or we can use strcmp(), which casts the integers to strings and is a bit more compact:

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

This works with this test case in PHP 5.6 through 8.1, so I'll try that in the PR.

bugfolder commented 1 year ago

@klonos, I didn't see your questions until after I'd finished typing my analysis, so I'll answer them here:

Does the same happen in Drupal? If so, is there an issue in d.org for it?

Dunno.

Have you tried adding another item after 0? (so that it's not the last option)

Yes. This still fails:

  $form['radios_1'] = array(
    '#type' => 'radios',
    '#title' => t('Radios 1'),
    '#options' => array(
      'foo' => t('Foo option'),
      0 => t('Zero option'),
      'bar' => t('Bar option'),
    ),
    '#default_value' => 'foo',
  );

Have you tried with a string '0' instead of numeric 0?

Yes. This still fails:

  $form['radios_1'] = array(
    '#type' => 'radios',
    '#title' => t('Radios 1'),
    '#options' => array(
      'foo' => t('Foo option'),
      '0' => t('Zero option'),
    ),
    '#default_value' => 'foo',
  );

Does this issue happen when all options are numeric? (instead of a mix or numbers/strings as keys)

All numeric works.

klonos commented 1 year ago

Yes. This still fails:

Didn't expect that TBH.

All numeric works.

Yeah, I thought so.

Excellent analysis @bugfolder 👍🏼 ...and thanks for the PR 🙏🏼 ...I'll try to "kick the tires" as soon as possible.

argiepiano commented 1 year ago

Great sleuthing! I can confirm that the same issue exists in Drupal 7.