This is a relatively easy problem to grasp, even though it's a bit strange to explain.

In element_children() we have code like the following:

  foreach ($elements as $key => $value) {
    if ($key[0] !== '#') {
      ...
    }
  }

Now this can cause a problem if $key is an empty string, which is common when you want to have something like "Select..." as the first option, which will trigger the "Required" error if the user doesn't choose an option. In other words, a form like this will cause an error:

$form['radios'] = array(
  '#type' => 'radios',
  '#options' => array(
    '' => t('Select...'),
    'one' => t('Option one'),
  ),
);

This throws the following PHP warning:

Error message
Notice: Uninitialized string offset: 0 in element_children() (line 5284 of /Users/nate/Sites/drupal7/includes/common.inc).

The simple fix here is we just check that $key has at least one character before trying to check the first character.

CommentFileSizeAuthor
#25 empty_radio_option_734234-23.patch2.64 KBfloretan
#23 734234-empty-radio-option_4.patch4.7 KBekes
#18 empty_radio_option_4.patch655 bytesMelissamcewen
#5 empty_radio_option_3.patch659 bytesmoonray
#4 empty_radio_option_2.patch659 bytesmoonray
#2 empty_radio_option_1.patch481 bytesmoonray
empty_radio_option.patch582 bytesquicksketch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

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

moonray’s picture

moonray CreditAttribution: moonray commented
Status: Needs work » Reviewed & tested by the community
FileSize
empty_radio_option_1.patch481 bytes

Rerolled the patch. Tested it to work.

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented
Status: Reviewed & tested by the community » Needs review

Subscribe. Need this for Skinr :(

moonray’s picture

moonray CreditAttribution: moonray commented
FileSize
empty_radio_option_2.patch659 bytes

Re-re-rolling. Hoping the test bot will like it.

moonray’s picture

moonray CreditAttribution: moonray commented
FileSize
empty_radio_option_3.patch659 bytes

OK, lets try that again with a patch that won't break drupal (oops).

sociotech’s picture

sociotech CreditAttribution: sociotech commented

Third time's a charm! No more errors. Thumbs up and RTBC.

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented
Status: Needs review » Reviewed & tested by the community

Same here. Marking RTBC.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

Thanks moonray! I should've rerolled this after it failed to apply. :-(

Moonray's #5 is identical to my original patch, just it has the file path correct for common.inc. As such I know that it works and prevents the PHP warning correctly. RTBC + 1.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Sorry, but we need tests for tweaky esoteric bugs like this.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
Status: Needs work » Needs review

Really? This is a PHP programming mistake. Clearly all strings aren't always longer than 1 character. It's equivalent to an isset($variable) versus an implied $variable.

moonray’s picture

moonray CreditAttribution: moonray commented

I second what quicksketch said in #10. There's no way to really write a test for a programming mistake like this.

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented
Status: Needs review » Reviewed & tested by the community

Ok, let's try again.

Melissamcewen’s picture

Melissamcewen CreditAttribution: Melissamcewen commented

The patch is working for me. Thanks!

sociotech’s picture

sociotech CreditAttribution: sociotech commented

Patch applied cleanly, resolved uninitialized string offset errors when using Skinr. RTBC +1

chx’s picture

chx CreditAttribution: chx commented
Status: Reviewed & tested by the community » Needs work

ABSOLUTELY NOT! if ($key === '' || $key[0] !== '#') { is the correct fix. Empty string is a perfect child it's not a property.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
Status: Needs work » Reviewed & tested by the community

Doh. Chx is totally right (and webchick too), since the fact is that this patch is the opposite of the correct thing.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
Status: Reviewed & tested by the community » Needs work
Melissamcewen’s picture

Melissamcewen CreditAttribution: Melissamcewen commented
Status: Needs work » Needs review
FileSize
empty_radio_option_4.patch655 bytes

Here is a patch based on chx's wisdom

casey’s picture

casey CreditAttribution: casey commented
Status: Needs review » Reviewed & tested by the community

per #15

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work

Hit this with Flag: #874292: Notice: Uninitialized string offset: 0 in element_children() Which I assume is why Nate wanted this fixed. ;)

Anyone up for writing that test? The code is practically written for you in the OP. We just need a select/radios element with a "" option in one of the forms in the form test. Just viewing the form's enough to trigger this issue.

ekes’s picture

ekes CreditAttribution: ekes commented
Assigned: Unassigned » ekes

Looking at test; and it only makes sense to test radio fapi display / validation generally - as is done for checkbox. Related issues spotted so far:-

And for present 'desired' behaviour. #621366: Enforce comformance to W3C recommendations on radio buttons? is not implemented.

However it's probably easiest to do this in stages; so small test to cover this first, and notes.

ekes’s picture

ekes CreditAttribution: ekes commented
Assigned: ekes » Unassigned

Oh wow.. posted that the wrong issue :) Well not the one I started working on.
But it's good to have all of them collected together

ekes’s picture

ekes CreditAttribution: ekes commented
FileSize
734234-empty-radio-option_4.patch4.7 KB

Patch from #18 with added simpletests starting on #21

ekes’s picture

ekes CreditAttribution: ekes commented
Status: Needs work » Needs review
floretan’s picture

floretan CreditAttribution: floretan commented
FileSize
empty_radio_option_734234-23.patch2.64 KB

@ekes has been faster than me, but I'm not sure we need so much details in the test. We already have the "Form element validation" tests that test a bunch of stuff with different element types and options. Simply adding an option with an empty key throws a warning, which is solved by this patch. Note that only elements of type "radios" throw errors, but I'm also adding an option with an empty key to the select tests for completeness.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented
Status: Needs review » Needs work 
+++ includes/common.inc	27 Aug 2010 15:20:38 -0000
@@ -5528,7 +5528,7 @@ function element_children(&$elements, $s
+    if ($key === '' || $key[0] !== '#') {

$key can also be a numeric index.

I think the proper condition is

    if (is_int($key) || $key === '' || $key[0] !== '#') {

What about NULL? Would be weird, but is it illegal? Test for empty()? That would also cover $key===''.

salvis’s picture

salvis
he/his
CreditAttribution: salvis commented
Status: Needs work » Needs review

I'm sorry for having derailed this. I thought I had a case of notices tracked back to this code and integer $keys, but now I cannot reproduce it anymore.

So, please ignore my #26 above.

The fix is fine, but I can't say how much testing is needed.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs review » Fixed

Yep, #25 looks like exactly what I had in mind.

Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Needs tests

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +Needs tests

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.