PHP warning from assuming #collapsed is defined. [#329019]

I just noticed some PHP warnings on a D6 test site for a form that had a fieldset where '#collapsible' was TRUE and '#collapsed' wasn't defined. I thought '#collapsed' just defaulted to FALSE if you didn't specify it. See attached (trivial) patch. Applies with minor offset to D6, too. In D5 you need a different patch because of a code style error on the line below (missing a space).

Comment	File	Size	Author
#7	drupal.fieldset.patch	871 bytes	sun
#7
	fapi_collapsed_not_defined.patch	570 bytes	dww

	fapi_collapsed_not_defined.d5.patch	643 bytes	dww

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 2 November 2008 at 00:43

Should we be accommodating for this misuse of the FAPI?

Comment #2

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 2 November 2008 at 00:44

Oh wait...nevermind. I read #collapsible and #collapsed backwards...

Comment #3

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 2 November 2008 at 00:52

Hmm...there are default values for #collapsible and #collapsed defined in system_elements:

  $type['fieldset'] = array(
    '#collapsible' => FALSE,
    '#collapsed' => FALSE,
    '#value' => NULL,
    '#process' => array('form_process_ahah'),
  );

Comment #4

dww

we/he/they

CreditAttribution: dww commented 2 November 2008 at 01:06

Well, tee hee, I see now I'm only getting the warnings in a place where I had done this:

 $fieldset = array(
    '#title' => t('Table in a fieldset'),
    '#collapsible' => TRUE,
    '#value' => theme('table', $header, $rows),
  );
  return theme('fieldset', $fieldset);

Of course, now that I know, it's trivial to add '#collapsed' => FALSE, in there, but I don't see what's the harm in making the default theme_fieldset() get this right. It already successfully tests if #collapsible is defined, why not handle a missing #collapsed, too? Surely the !empty() isn't a performance concern, is it?

Comment #5

sun

German

Karlsruhe

CreditAttribution: sun commented 2 November 2008 at 01:16

Hm. Did you think about using drupal_render() instead?

$element = array(
  '#type' => 'fieldset',
  '#title' => t('Table in a fieldset'),
  '#collapsible' => TRUE,
  '#value' => theme('table', $header, $rows),
);
return drupal_render($element);

Comment #6

sun

German

Karlsruhe

CreditAttribution: sun commented 2 November 2008 at 01:39

Status:

Needs review

» Reviewed & tested by the community

Anyway, I think we can fix this for D7. All FAPI tests pass. Additionally checked fieldset behaviors manually. Probably too minor to backport though, and I'd generally recommend the way in #5, since that will load the default element info for all element types.

Comment #7

sun

German

Karlsruhe

CreditAttribution: sun commented 2 November 2008 at 01:43

File	Size
drupal.fieldset.patch	871 bytes

Actually, no - if we fix #collapsed, then we should also properly test for #collapsible.

Comment #8

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 2 November 2008 at 02:07

Does calling theme() on other $form type elements create errors as well? I'm still not convinced this needs fixing since we're not allowing default values from system_elements to get loaded in.