hook_form_alter() functions affecting the node form should be their own function

I believe that we should go one step further, and make all those elements form_altered into the node form into proper FAPI #types.

Maybe not all cases would be logical as their own form elements, would they? Do we have a good list?

Quick list of form_alter functions in core that affect the node form:

• book_form_alter
• comment_form_alter
• locale_form_alter
• menu_form_alter
• path_form_alter
• taxonomy_form_alter
• translation_form_alter
• upload_form_alter

so I understand we're not touching the hook_form_node_type_form_alter right now, which are

• comment_form_node_type_form_alter
• locale_form_node_type_form_alter
• translation_form_node_type_form_alter

and what should these helper functions be called? modulename_node_form_element?

ok, how's this?

not sure if the Adds the book fieldset to the node form. is a proper description for this

Moved contents of comment_form_alter() for extending node forms to separate function comment_node_form_element().
Will that do?

Moved code in menu_form_alter() to separate function menu_node_form_element().

Moved code in locale_form_alter() to separate function locale_node_form_element()

noticed that locale_form_alter takes $form_state as reference, I left it as it was, but I'm still unclear on why -> #503892: hook_form_alter() and hook_form_FORM_ID_alter() take $form_state as reference

Moved code in path_form_alter() to separate function path_node_form_element()

Moved code in translation_form_alter() to separate function translate_node_form_element()

same weirdness as described in [#503829]

Moved most of the code in taxonomy_form_alter() to (new function) taxonomy_node_form_element().

Moved the node form related code to separate function upload_node_form_element() and noticed that the node_type form code wasn't extracted to the upload_form_node_type_form_alter() yet, so I also did that as per #287178: Use hook_form_FORM_ID_alter() where possible and added some doxygen comments

Attached.

btw: patches should be made from the root of your Drupal tree -- not from the module folder that you are working on. A number of these were made from the modules/___ directory.

Woops.

I understood that these will eventually be reusable hooks, but (at least right now) they also need to be called from the regular hook_form_alter, because the hook isn't invoked yet, but could easily be done

personally, I'm not that excited about the naming either, mainly because the name suggests that it only adds 1 element, where most core modules add more than 1 element so maybe hook_node_form_elements() would be better than hook_node_form_element()

I think this is still pretty much important, but I'm not sure about the way forward. I would like to make those proper form elements, so that you can do:

 function comment_form_alter(&$form, $form_state, $form_id) {
   if (!empty($form['#node_edit_form'])) {
    // Add elements to the node form.
    $form['comment_settings'] = array(
      '#type' => 'comment_node_settings',
      '#node' => $node,
      '#weight' => 30,
      '#access' => user_access('administer comments'),
    );
  }
}

Those elements would be expanded with a proper #process function and could be used as-is by the form builder. What do everyone think?

We definitely need a longer API clean-up phase for D8.

How much of this issue/patch is still relevant today?

Okay, looking at the form alters listed in #4 and in most cases locating their D7 equivalents:

• book_form_alter() is now book_form_node_form_alter() and has a wonky "private" callback to construct the form that passes the $form array by reference, D6-style: _book_add_form_elements()
• comment_form_node_form_alter(), menu_form_node_form_alter, path_form_node_form_alter, translation_form_node_form_alter() add the items directly to the $form array.
• locale_form_alter() now includes a separate form builder for locale_language_selector_form().
• taxonomy_form_alter() and upload_form_alter() no longer exist because their functionality is now part of the Field API.

However, it's not clear to me why this is necessary. Clearly we have a working form builder in D7, contrary to the original post. Can anyone clarify?

Yes, I don't understand either why this is necessary.

@sun, can you clarify?

My guess is that you are referring to http://drupal.org/project/form_builder?

Berdir and I agree that this is at most a nice refactor for reusability and code cleanliness, but it really doesn't seem to be major. Downgrading and tagging for a summary so we can get something more accurate than "Forms won't work in D7!" which is a bit anachronistic. :)

Oh, it's still a good novice task though. :)

Merely refactoring this code into individual functions instead of pure Form API callbacks isn't going to be sufficient and I don't see how that is going to improve the situation in any way. It's also not really clarified in this issue.

Instead, we should have a proper discussion for how to handle those entity form widget additions (of which some aren't really field_extra_fields) into a general pattern - ideally re-using generic (field/property) widgets and formatters, and possibly also upcoming theme components.

Given that form_builder seems to work without this, let's won't fix this issue.