hook_form_alter() can easily clobber a text format

In #414424: Introduce Form API #type 'text_format' the text format handling in D7 was fixed and greatly improved. However, one feature of the new system is that the default behavior is to always put the submitted text format in a standalone 'format' property within $form_state['values']. In many cases, this winds up at the top level where it can conflict with other elements on the form.

The main benefit of this approach is that it's the place where a majority of use cases want it to be. Thus, it's more likely that form submit handlers can continue to do things like this when they have text formats in them:

drupal_write_record('some_table', $form_state['values']);

without having to massage $form_state['values'] beforehand.

However, the disadvantage is that anyone who uses hook_form_alter() to add a text-format-enabled textarea onto an existing form has to find, read, and completely understand this code comment in the PHP Doc of filter_process_format():

+ * If multiple text format-enabled elements are required on the same level of
+ * the form structure, modules can set custom #parents on the original element.
+ * Alternatively, the #after_build may be unset through a subsequent #process
+ * callback.

This is really easy to get wrong, even in very simple uses of hook_form_alter(). For example, code like this:

/**
 * Implements hook_form_FORM_ID_alter().
 */
function mymodule_form_block_add_block_form_alter(&$form, &$form_state) {
  $form['mymodule_extra_field'] = array(
    '#type' => 'text_format',
    '#title' => 'Whatever',
  );
  $form['#submit'][] = 'mymodule_custom_submit_function';
}

looks fine, and from the perspective of the module author will work fine; on the form they will get a new textarea with text format selector attached, and in their submit handler will receive $form_state['values']['mymodule_extra_field'] storing the entered text, and $form_state['values']['format'] storing the chosen text format.

However, this code actually leads to a major problem. The selected text format for this new element will additionally clobber and replace the one selected for the custom block body (which also appears on that form). When the form is saved, the custom block body will therefore be silently assigned the incorrect text format, which is a critical bug and in some cases a security issue. I think it is very difficult for the module author to track this down unless they know about it.

It's true that Drupal 6 had a similar issue where the default behavior was to put 'format' on the top level of the form. However, (a) this is still bad, and (b) I think the difference is that there you were a fair amount less likely to introduce this bug on your own because in that case, you had to be more explicit and your code wouldn't work if you did hook_form_alter() the naive way. For example, in D6:

  $form['mymodule_extra_field'] = array(
    '#type' => 'textarea',
    '#title' => 'Whatever',
  );
  $form['mymodule_extra_field_format'] = filter_form();

would simply never pass anything at all into $form_state['values']['mymodule_extra_field_format'], so your code would be broken if you left it like that.

To solve this, I would suggest:

1. Going back to the earlier proposal discussed in #414424: Introduce Form API #type 'text_format', which would always put the text format somewhere in the array structure where it would not interfere with other text formats; e.g., in the above D7 example, you would get $form_state['values']['mymodule_extra_field']['value'] and $form_state['values']['mymodule_extra_field']['format'] in your form submit handler.
2. If that is not possible, at least documenting much more clearly and prominently that this is a gotcha and simple ways to get around it. For example, I think the simplest way to get around it might be to encourage anyone using hook_form_alter() to add something with a text format to always put it in a wrapper. Code like this does not experience the bug I described above:
   

   /**
    * Implements hook_form_FORM_ID_alter().
    */
   function mymodule_form_block_add_block_form_alter(&$form, &$form_state) {
     $form['mymodule_wrapper'] = array(
       '#tree' => TRUE,
     );
     $form['mymodule_wrapper']['mymodule_extra_field'] = array(
       '#type' => 'text_format',
       '#title' => 'Whatever',
     );
     $form['#submit'][] = 'mymodule_custom_submit_function';
   }
   

   i.e., because it results in $form_state['values']['mymodule_wrapper']['mymodule_extra_field'] and $form_state['values']['mymodule_wrapper']['format'] in the submit handler, it will not interfere with other form elements by default.