Form elements cannot be rendered without form_builder()

Subscribing.

First, I'm not sure why this should be allowed. form_builder() has legitimate reasons for being a necessary function.

Second, if it should be allowed, the patch should be applied to http://api.drupal.org/api/function/system_element_info/7, where the default items are added to rendered forms. These are not 'common elements,' they are form specific elements.

Personally, I don't see this as 'critical' as the request is essentially: "I cannot bypass the FormsAPI and arbitrarily render form elements." You couldn't do this in prior versions of Drupal, either.

Considering that, I don't think this is a bug, either. It's a feature for developers. But I'll leave that status alone.

Well, the default values for form elements (as defined is system_element_info()) should be the only thing required for rendering. This is a bug. Not critical, but still a bug.

Right, but it's a simple change. Just move the += code into the proper function and you're done.

I would argue that this issue is part of the "render system" component, which doesn't exist as a component choice yet, so moving to "base system".

We should start with a patch that adds a test that fails: a page callback that returns a render array containing an element of each type, and see what happens. We'll then have to decide which types we don't wish to support outside of the FAPI pipeline (for example, "checkboxes" has logic in #process that we probably don't want to move to #pre_render), remove those from the tests, and add documentation somewhere explaining those types are for FAPI-use only. For the types that make sense to support outside of FAPI, we'll need to make those tests pass.

@makononov: are you up for writing the test?

The only #types that are valid to use outside of Form API are

- fieldset
- vertical_tabs

Perhaps more. But anything that can receive user input needs to go through FAPI. Also allowing to render form elements receiving input without Form API would just introduce a pile of security issues.

I disagree with #9 by sun. Please make a case why #title_display is a form specific property and not a render property. I don't think we can. So, it should be declared in system_element_info(), not form_builder().

IMO if people want to bypass FAPI, they are on their own. We don't need a framework that plays nanny for you and prohibits reusing form elements as developer sees fit.

For example, assume I am building a form which submits to a 3rd party site. Its silly IMO to prohibit dev from using our render and alter system while completing this task.

ok, let's simply see how far we can go with moving stuff off to hook_element_info()

Note that most form.inc changes in this patch are already covered in #690980: Disabled form elements not properly rendered, which is hopefully committed ASAP.

Revamped this patch. Looks like it's time to vastly decrease the file size of form.inc ;)

Let's only handle the general issue of rendering form elements here. Separate from the specific collapsible fieldsets + vertical tabs cases, as those are much more complex and should be resolved in #857124: Collapsible fieldsets and vertical tabs do not work without form_builder() (Form API). I.e., here, we only prepare theme_fieldset() to work without form_builder(), nothing else.

form_get_error(). What kind of an odd function is that. Sheeeesh.

This is quite possibly one of the most awesome Form API theme function clean-up patches thus far.

Heh, nice promotion work :-)

+++ modules/field_ui/field_ui.admin.inc	1 Sep 2010 17:16:52 -0000
@@ -1480,13 +1480,16 @@ function field_ui_field_edit_form($form,
-  $form['instance'] = array(
+  $form['instance'] = array_merge(element_info('fieldset'), array(

why on this specific fieldset amongst all core fieldsets ?

Powered by Dreditor.

That's due to a yet not well known problem that will bite all of us very badly in D7:

That particular fieldset manually sets a #pre_render, i.e.

  $form['myfieldset'] = array(
    '#type' => 'fieldset',
    '#pre_render' => array('yay_prerender_yay'),
  );

form_builder(), however, does:

  // Use element defaults.
  if (isset($element['#type']) && empty($element['#defaults_loaded']) && ($info = element_info($element['#type']))) {
    // Overlay $info onto $element, retaining preexisting keys in $element.
    $element += $info;      // <<------------------------------------
    $element['#defaults_loaded'] = TRUE;
  }

So unless you know that system_element_info() defines:

  $types['fieldset'] = array(
    '#process' => array('form_process_fieldset', 'ajax_process_form'),
    '#pre_render' => array('form_pre_render_fieldset'),
    '#theme_wrappers' => array('fieldset'),
  );

You are overriding (skipping) form_pre_render_fieldset() entirely, which is NOT the intention in 99.99% of all cases.

Hm, bad DX mojo indeed.

We didn't have that in D6 ?

(as a side note, I don't even agree with that specific #pre_render callback in field_ui - field_ui_field_edit_instance_pre_render

We had the problem in D6 already, but it wasn't as evident as it is in D7, because not many modules were actively making use of #process, #pre_render, #after_build, and whatnot.

TBH, when I saw that #pre_render in Field UI, my reaction was: Oh dear lord, this module needs some love. ;) But frankly, I'd prefer to defer that to #616240: Make Field UI screens extensible from contrib - part II or a new issue.

Field UI does needs some love.
And that part won't be adressed by #616240: Make Field UI screens extensible from contrib - part II. Anyway, different issue indeed.

Can we add a comment for this array_merge(element_info('fieldset'), ...) ?

Sure thing :)

Any other feedback?

#37: drupal.render-form-elements.37.patch queued for re-testing.

Any feedback?

Looks like another nice cleanup. I just found a couple comment snafus.

+++ includes/common.inc	3 Sep 2010 00:53:16 -0000
@@ -5582,6 +5582,30 @@ function element_get_visible_children(ar
+ *   except for the leading '#', then an attribute name value is sufficientn and

typo: sufficientn

+++ includes/form.inc	3 Sep 2010 00:53:16 -0000
@@ -1369,17 +1369,20 @@ function form_get_errors() {
+ * Form errors higher up in the form structure override more deeper errors and

remove 'more'. is redundant

+++ includes/form.inc	3 Sep 2010 00:53:16 -0000
@@ -3338,10 +3319,16 @@ function theme_file($variables) {
+  // This function is invoked as theme wrapper, but themed form elements may
+  // not necessarily went through form_builder().

busted 'went'

+++ includes/form.inc	3 Sep 2010 00:53:16 -0000
@@ -3486,10 +3473,12 @@ function _form_set_class(&$element, $cla
+  // This function is invoked from theme functions, but themed form elements may
+  // not necessarily went through form_builder().

busted 'went'

Powered by Dreditor.

Thanks for reviewing, moshe! Fixed all of those.

Good to go.

Alright. Committed to CVS HEAD. Thanks.

We are trying to solve the default properties problem outlined in #33 over in #914792: Custom element properties entirely override default element info properties -- and we would love some developer feedback on two different resolution attempts, so please review them and share your thoughts over there!

Grendzy pointed out that this caused #926016: Several bugs when trying to remove files from a multivalued file/image field - cross-linking.

I think this has been my first WTF with Drupal 7. I have a simple use case to append a fieldset with a large help section to the content. This is what is being return from the page callback: (Running Drupal dev 30 Sept, 2010)

<?php
  return array(
    'table' => array(
       '#markup' => 'The main content',
    ),
    'help' => array(
      '#type' => 'fieldset',
      '#title' => t('Help'),
      '#collapsible' => TRUE,
      '#collapsed' => TRUE,
      'subcontent' => array(
        '#markup' => 'This is the help',
      ),
    ),
  );
?>

The code generates a fieldset just fine, but it lacks all of the functionality. Pasting the code into an existing form, no issues at all. This lead me here.

The separation of the rendering and processing is a bit alien to me. When skim reading over the changes introduced in Drupal 7, I assumed that all content, either $forms or $output would have been run through the exact same rendering & processing functions, with the exception of the $form element that would have had additional processing to append the wrapping form, cached values, etc. Now it seems like a hybrid mix. Anyway that's my 5 min rant.

The workaround is to do the additional step when generating the content. This is not the best solution as you invoke the Forms API completely, but it works (full version from the module this time):

<?php
function my_page_callback() {
  ...
  return array(
    'table' => array(
       '#markup' => theme('table', array('header' => $header, 'rows' => $rows)) . $help . theme('item_list', array('items' => $help_items)),
    ),
    'help' => drupal_get_form('_name_format_help_form'),
  );
}

/**
 * A workaround to display a collapable fieldset inside a standard page.
 */
function _name_format_help_form() {
  $form['help'] = array(
    '#type' => 'fieldset',
    '#title' => t('Format string help'),
    '#collapsible' => TRUE,
    '#collapsed' => TRUE,
    'format_parameters' => array(
      '#markup' => theme('name_format_parameter_help'),
    ),
  );
  return $form;
}
?>

+++ modules/simpletest/tests/common.test	15 Sep 2010 02:50:08 -0000
@@ -1470,6 +1470,132 @@ class DrupalRenderUnitTestCase extends D
+    $this->assertRenderedElement($element, '//fieldset/legend[contains(., :title)]', array(

this line actually tests that fieldsets work outside of forms. so please help isolate if the test is wrong or otherwise.

Powered by Dreditor.

The system goes through and process's the fieldset through drupal_render(). Very cool except that half of the functionality is in the FAPI #process callback: as per the dump after += element_info() shows below. Thus the eariler comment about not liking the split between the two processing systems if both are meant to do the same thing!

#process (Array, 2 elements)
            0 (String, 21 characters ) form_process_fieldset
            1 (String, 17 characters ) ajax_process_form
#pre_render (Array, 1 element)
            0 (String, 24 characters ) form_pre_render_fieldset

The function form_process_fieldset() adds the magic for the fieldset, but this is never called. Moving the code from #process to #pre_render works on this singular use-case, but I'm not sure if it would break things in other places.

Totally untested code as I do not have a working HEAD version nor SimpleTest set up, and I had to step through with manual debugging on another Drupal 7 setup. :(

At least this lead to a cleaner workaround:

<?php
  $form = array(
    '#type' => 'fieldset',
    '#title' => t('Format string help'),
    '#collapsible' => TRUE,
    '#collapsed' => TRUE,
    '#parents' => array(),
    'format_parameters' => array(
      '#markup' => theme('name_format_parameter_help'),
    ),
  );

  // Add the collapsible magic from the FAPI system.
  $form_state = array();
  $form = form_process_fieldset($form, $form_state);
  
  return array(
    'table' => array(
       '#markup' => theme('table', array('header' => $header, 'rows' => $rows)) . $help . theme('item_list', array('items' => $help_items)),
    ),
    'help' => $form,
  );  
?>

Window line endings, this time with *NIX

Please read the issue before re-opening. See #857124: Collapsible fieldsets and vertical tabs do not work without form_builder() (Form API)