Select element with empty options triggers PHP warning

Though the policy is that we do not babysit broken code, for the sake of JS people, and because the cost is negligible, I say, let's do it.

Shouldn't we default #options to array() in system_elements()?

Any activity?? moving to new version..

Moving.

From #444860: fix php warning when #options is not set in a select field, the use case for non-set options appears to be to bypass the options checker.

 if (isset($elements['#options']) && isset($elements['#value'])) {

That means our options checker is broken :)

Confirmed that this is still a bug in 8.x, if #options is not an array (eg: NULL, 'random string', FALSE).

The attached module demonstrates the problem. Enable "Empty Select Tester" module and navigate to /empty_select_test, and you'll get a warning:

Warning: Invalid argument supplied for foreach() in form_select_options() (line 2664 of /blah/core/includes/form.inc).

The attached patch just checks if $choices is an array, and returns an empty string if it is not. This is only slightly different than chx's solution: it bails out of the function before the checks of the element value, since those aren't necessary if $choices isn't valid.

Patch looks good, but needs tests. Seems the proper way to test would be to add a form to the form_test module which defines a select box with a non-array #options. Rendering the form page should not result in a php warning.

+++ b/core/includes/form.incundefined
@@ -2656,6 +2656,10 @@ function form_select_options($element, $choices = NULL) {
+  // If there are no choices, <option> elements cannot be created

Minor note here; the inline comment is also missing a period. So, it would be good to fix this when we add the test. :)

I think I've got this figured out. This is only the second test I've written, so again: any feedback is very much welcome.

Two patches attached: one with only the new test (which should fail), and the other includes the fix (which should pass tests).

The test case simply requests a form that has a select element with '#options' => NULL. As adharris noted in #8, it doesn't need to do any assertions on the display of that form, because SimpleTest will report the PHP warning if it's thrown.

I tweaked the fix slightly from the one in #7 to check count($choices) instead of is_array($choices), so that it uses the same method as form_process_radios().

+++ b/core/includes/form.incundefined
@@ -2662,6 +2662,10 @@ function form_select_options($element, $choices = NULL) {
+  if (!(count($choices) > 0)) {

it should rewrite to

  if (!isset($choices)) {
    $choices = $element['#options'];
    if (empty($choices) && !is_array($choices)) {
      return '';
    }
  }

#10: 78292-10.fix_.patch queued for re-testing.

Made a foolish mistake in my last patch: I left in something I had added to an existing test case when I was first trying to figure out how to write the test:

+++ b/core/modules/simpletest/tests/form_test.moduleundefined
@@ -1113,6 +1120,15 @@ function form_test_select($form, &$form_state) {
+  $form['select'] = array(
+    '#type' => 'select',
+    '#title' => t('Select'),
+    '#multiple' => TRUE,
+    '#description' => t('The description appears usually below the item.'),
+    '#options' => array(),
+    '#default_value' => -1,
+  );

This broke the existing test, because earlier in that function, $form['select'] was set with different values. This patch simply removes this section.

Hurrah! That patch was the test case only, without the fix, and was intended to fail. Switching back to NR for community review.

Thanks @BrockBoland! Great to see a working test that exposes the issue. I've a few minor points for cleanup in the patch:

+++ b/core/modules/simpletest/tests/form.testundefined
@@ -618,6 +618,12 @@ class FormElementTestCase extends DrupalWebTestCase {
+  // Verify that an empty select element does not result in a PHP warning.

This needs to be a docblock rather than an inline comment, and follow the standards for that (so "Verifies" rather than "Verify"). Reference: http://drupal.org/node/1354#functions

+++ b/core/modules/simpletest/tests/form.testundefined
@@ -618,6 +618,12 @@ class FormElementTestCase extends DrupalWebTestCase {
+    // No need to check the page output: SimpleTest will report the PHP warning.

I'd change this comment to describe what the line is doing, not what it's not doing. Also, for completeness, I'd actually prefer to add an assertion for the expected markup.

+++ b/core/modules/simpletest/tests/form_test.moduleundefined
@@ -2012,3 +2019,17 @@ function form_test_required_attribute($form, &$form_state) {
+ * Builds a form to test that no PHP warnings are thrown when #options is not an
+ * array for a select element.

This should be one line of 80 characters or less. Reference: http://drupal.org/node/1354#functions

+++ b/core/modules/simpletest/tests/form_test.moduleundefined
@@ -2012,3 +2019,17 @@ function form_test_required_attribute($form, &$form_state) {
\ No newline at end of file

We need a newline at the end of the file here.

Tagging novice to clean up these points and add an assertion to the test. (The additional assertion does not need to fail; it just needs to codify the expected behavior.) Thanks!

Note that the two test files in the patch have since moved: http://drupalcode.org/project/drupal.git/commit/789ab2a

Updated with xjm's feedback and the new path for the form_test module.

I'm waiting to see what the testbot kicks back for the assertion I added to check for the warning:

$this->assertNoText(t('Warning: Invalid argument supplied for foreach()'));

My local dev instance is NOT printing the warning to the page (thought my error_reporting is cranked up), so this assertion never fails. The test still fails, though, because of the PHP warning, but that assertion doesn't fail. This may well be a local configuration issue, though, which is why I'm curious what the bot says.

Ah, I see. Generally testing for the text of a PHP error like that won't work. What I suggested in #18 was to add an assertion for what should happen, for the content we expect to see on the page when the bug is not present. In this case, I think it's an empty select element that we expect to be rendered, which we can test for with DrupalWebTestCase::assertFieldByXPath().

The idea is that the automated test is not merely checking to make sure the particular error is not there, but also adding the test coverage that was missing, to codify the expected behavior of the element under this circumstance. Like I said, this additional assertion does not need to fail.

The rest of the cleanup looks good. Thanks @BrockBoland!

I'm here to help!

I'm kind of a XPath newb, lets see if this works.

Changing status for the test bot. I'll manually test it once it clears that.

#25: 78292-30_fixed.patch queued for re-testing.

This is the patch from #25 with a newline at the end of the file.

#30 is the complete patch, so we'd expect it to pass if the test were working properly. However, the assertion is failing.

I'd recommend running the test locally and inspecting the verbose output to verify that this element is in fact printed, and then adjusting the xpath from there until the test passes locally. Then, upload both a test-only patch and a complete patch to expose the expected failures and confirm that the patch resolves them.

Additionally, there are a couple things to clean up:

+++ b/core/modules/system/tests/form.testundefined
@@ -620,6 +620,14 @@ class FormElementTestCase extends DrupalWebTestCase {
+   * Verifies that an no empty select elements exist

This isn't quite a complete sentence (and needs a period). I'd suggest:
Tests a select element when #options is not set.

+++ b/core/modules/system/tests/form.testundefined
@@ -620,6 +620,14 @@ class FormElementTestCase extends DrupalWebTestCase {
+    $this->assertFieldByXPath("//option/select[1]", NULL, t('Empty option element found.'));

Let's remove t() from the assertion message. Reference: http://drupal.org/simpletest-tutorial-drupal7#t

Trying to reproduce locally.

I incorporated the feedback from Comment #32:

Changed the function header doc to "Tests a select element when #options is not set."

Test verifies that a select element is present, and that it contains no option element.

This looks great. Thanks @thoufek!

Can we set the $choices thought code ?

if YES, $choices = 1 will passed and give errors ?

if NO, why not use ===

Thanks.

+++ b/core/includes/form.inc
@@ -2662,6 +2662,10 @@ function form_select_options($element, $choices = NULL) {
+  if (!(count($choices) > 0)) {

I don't understand why this isn't just simply empty() ?

Furthermore, the suggestion in #2 makes sense to do in addition to that; i.e., default #options to an empty array in system_element_info().

@sun: I stuck with count() to handle a variety of invalid values for (what should be) the $choices array. Setting '#options' => array() in system_element_info(), as suggested in #2, is a good start because it sets the default for #options, but the warning will still be thrown if a module author sets #options to an invalid value in the form API: NULL, TRUE, FALSE, a number or string, etc. NULL and FALSE would be caught by empty(), but actual values would not.

Though, the same is true of count(), so really, this check should be:

if (!is_array($choices) || !(count($choices) > 0)) {
  return '';
}

Working on updating this patch for the new location of tests, and to add what I mentioned in the previous comment.

Updated to apply to the new test class and adjusted the condition:

  // If there are no choices, <option> elements cannot be created.
  if (!is_array($choices) || !(count($choices) > 0)) {
    return '';
  }

I also added the default value for #options in system_element_info().

(just tagging the event)

patch did not apply anylonger, reroll

Simplified. We only need to check once when the function is initially called from theme_select().

Please note that this can produce invalid HTML markup under certain conditions, since the spec for select says:

• A select element with a required attribute and without a multiple attribute, and whose size is “1”, must have a child option element.
• The first child option element of a select element with a required attribute and without a multiple attribute, and whose size is “1”, must have either an empty value attribute, or must have no text content.

That is, because theme_select() unconditionally wraps the output of form_select_options() into a <select></select> element, regardless of whether it is empty.

However, this entire case of empty #options is so extremely bogus in the first place that I don't think it makes any sense to cater for that.

Thus, I think this is RTBC.

also fine for me

Does this absolutely need a full page callback to test it?

 
   /**
+   * Tests a select element when #options is not set.
+   */
+  function testEmptySelect() {
+    $this->drupalGet('form-test/empty-select');
+    $this->assertFieldByXPath("//select[1]", NULL, 'Select element found.');
+    $this->assertNoFieldByXPath("//select[1]/option", NULL, 'No option element found.');
+  }

Seems like a drupal_get_form() + drupal_render() would be enough?

That seems to be the pattern all throughout FormTest.php - are you asking that they all change?

They probably should, but not in this patch. Committed/pushed to 8.x.