Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
When attempting to create a select box with no options, you get the following error message:
warning: Invalid argument supplied for foreach() in D:\php\twins\drupal\includes\form.inc on line 643.
I think the function form_select_options() should also consider the case of empty options, for example with a cast in the foreach:
foreach ((array)$choices as $key => $choice) {
......
}
Comment | File | Size | Author |
---|---|---|---|
#43 | drupal8.form-select-empty.43.patch | 2.99 KB | sun |
#42 | empty_select-78292-42.patch | 3.15 KB | patrickd |
#40 | 78292-40.patch | 3.15 KB | BrockBoland |
#34 | drupal-78292-34.patch | 2.26 KB | thoufek |
#30 | 78292-30.patch | 2.19 KB | BrockBoland |
Comments
Comment #1
chx CreditAttribution: chx commentedThough 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.
Comment #2
drummShouldn't we default #options to array() in system_elements()?
Comment #3
ricabrantes CreditAttribution: ricabrantes commentedAny activity?? moving to new version..
Comment #4
Jaza CreditAttribution: Jaza commentedMoving.
Comment #5
Heine CreditAttribution: Heine commentedFrom #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.
That means our options checker is broken :)
Comment #6
BrockBoland CreditAttribution: BrockBoland commentedConfirmed 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).
Comment #7
BrockBoland CreditAttribution: BrockBoland commentedThe 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.Comment #8
adharris CreditAttribution: adharris commentedPatch 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.
Comment #9
xjmMinor note here; the inline comment is also missing a period. So, it would be good to fix this when we add the test. :)
Comment #10
BrockBoland CreditAttribution: BrockBoland commentedI 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 ofis_array($choices)
, so that it uses the same method asform_process_radios()
.Comment #12
droplet CreditAttribution: droplet commentedit should rewrite to
Comment #13
BrockBoland CreditAttribution: BrockBoland commented#10: 78292-10.fix_.patch queued for re-testing.
Comment #14
BrockBoland CreditAttribution: BrockBoland commentedMade 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:
This broke the existing test, because earlier in that function,
$form['select']
was set with different values. This patch simply removes this section.Comment #15
BrockBoland CreditAttribution: BrockBoland commentedComment #17
BrockBoland CreditAttribution: BrockBoland commentedHurrah! That patch was the test case only, without the fix, and was intended to fail. Switching back to NR for community review.
Comment #18
xjmThanks @BrockBoland! Great to see a working test that exposes the issue. I've a few minor points for cleanup in the patch:
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
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.
This should be one line of 80 characters or less. Reference: http://drupal.org/node/1354#functions
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!
Comment #19
BrockBoland CreditAttribution: BrockBoland commentedNote that the two test files in the patch have since moved: http://drupalcode.org/project/drupal.git/commit/789ab2a
Comment #20
BrockBoland CreditAttribution: BrockBoland commentedUpdated 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:
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.
Comment #22
xjmComment #23
xjmAh, 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!
Comment #24
Traverus CreditAttribution: Traverus commentedI'm here to help!
Comment #25
Traverus CreditAttribution: Traverus commentedI'm kind of a XPath newb, lets see if this works.
Comment #26
BrockBoland CreditAttribution: BrockBoland commentedChanging status for the test bot. I'll manually test it once it clears that.
Comment #28
BrockBoland CreditAttribution: BrockBoland commented#25: 78292-30_fixed.patch queued for re-testing.
Comment #30
BrockBoland CreditAttribution: BrockBoland commentedThis is the patch from #25 with a newline at the end of the file.
Comment #32
xjm#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:
This isn't quite a complete sentence (and needs a period). I'd suggest:
Tests a select element when #options is not set.
Let's remove
t()
from the assertion message. Reference: http://drupal.org/simpletest-tutorial-drupal7#tComment #33
thoufek CreditAttribution: thoufek commentedTrying to reproduce locally.
Comment #34
thoufek CreditAttribution: thoufek commentedI 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.
Comment #35
xjmThis looks great. Thanks @thoufek!
Comment #36
droplet CreditAttribution: droplet commentedCan we set the $choices thought code ?
if YES, $choices = 1 will passed and give errors ?
if NO, why not use ===
Thanks.
Comment #37
sunI 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().
Comment #38
BrockBoland CreditAttribution: BrockBoland commented@sun: I stuck with
count()
to handle a variety of invalid values for (what should be) the$choices
array. Setting'#options' => array()
insystem_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
andFALSE
would be caught byempty()
, but actual values would not.Though, the same is true of
count()
, so really, this check should be:Comment #39
BrockBoland CreditAttribution: BrockBoland commentedWorking on updating this patch for the new location of tests, and to add what I mentioned in the previous comment.
Comment #40
BrockBoland CreditAttribution: BrockBoland commentedUpdated to apply to the new test class and adjusted the condition:
I also added the default value for
#options
insystem_element_info()
.Comment #41
BrockBoland CreditAttribution: BrockBoland commented(just tagging the event)
Comment #42
patrickd CreditAttribution: patrickd commentedpatch did not apply anylonger, reroll
Comment #43
sunSimplified. 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:
That is, because
theme_select()
unconditionally wraps the output ofform_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.
Comment #44
patrickd CreditAttribution: patrickd commentedalso fine for me
Comment #45
catchDoes this absolutely need a full page callback to test it?
Seems like a drupal_get_form() + drupal_render() would be enough?
Comment #46
kscheirerThat seems to be the pattern all throughout FormTest.php - are you asking that they all change?
Comment #47
catchThey probably should, but not in this patch. Committed/pushed to 8.x.