Introduce Form API #type 'text_format'

I agree line-by-line with Heine concerns. Moving to a separate FAPI element would:

(1) be cleaner,
(2) remove the ugly belly-dancing that was introduced by #125315: Textarea with input format attached: when you add a #type => 'textarea' with a #text_form, form_process_text_format(): copies the textarea as a child of itself ($element['value']), and add the filter selection widget as a child element ($element['filter']), but mess up parenting so that the $element['value'] will be attributed to $form_state['values'][''], and $element['filter'] to $form_state['values']['_format'],
(3) will allow us to simplify some aspects of #304330: Text format widget.

Subscribing. I've also pinged sun on IRC to take a look at this issue.

subscribing.

I agree with the reasoning, but not with the ranting. In fact, #125315: Textarea with input format attached was a corner-stone for considering and using two (thus far separate) elements in a combined fashion. It laid the ground for #304330: Text format widget (which wouldn't have been possible without this initial step) and overall better Wysiwyg support in Drupal. We are able to advance on it now.

Once #304330: Text format widget and #412016: Custom key name for #text_format. are in, let's do the following:

- Introduce a new FAPI #type 'text_format' (or similar)

- Use the #rows property to determine whether we render a textfield or textarea as widget (thus, 1 FAPI element type)

- Use further FAPI properties specific to this #type to allow to customize the arguments for filter_form() and widget display (such as #weight-s).

- Use a #theme_wrapper function to render the text widget, format widget, and form element description (see process function in #304330: Text format widget)

- Make Field API's text field use this widget.

- Cleanup: Move all of that stuff out of form.inc and put it into filter.module. Move core functions from filter.module into common.inc. filter.module is needlessly loaded on all pages currently, but almost never required, because we are caching filtered content.

This one is critical.

So here is the battle plan:

1) #304330: Text format widget, required by

2) an option widget validation issue yched mentioned recently in IRC, but also #412016: Custom key name for #text_format., which is required by

3) #375907: Make text field use D7 '#text_format' selector

Work on this issue can start at any time, but I will personally focus on the aforementioned issues first.

After further discussion with Damien in IRC, I think we can't move everything into the theme function of this new #type, because we will still have to ensure proper access checks for the text format using a #process function (see #91663: Permission of text format is not checked when editing an entity and instead reset to something a user can use.).

Subscribe. This certainly seems like the right way to go.

Thinking aloud, another advantage of going this route is that the current #text_format approach requires code like this to specify the format:

 '#text_format' => isset($node->format) ? $node->format : FILTER_FORMAT_DEFAULT,

which is not only ugly but also prone to error, especially when it's changed via hook_form_alter (there are security risks to assigning the wrong one).

Presumably, if this were its own form API type, you could instead replace the above code with something like:

'#existing_format' => $node->format,

There would also be a #default_format property available, although most people would not need to use it. The processing code that decided which format to preselect on the form would then work like this:

1. If #existing_format is set and the user has access to it, preselect that.
2. If #existing_format is set but the user doesn't have access to it, deny access to editing the field, as per #91663: Permission of text format is not checked when editing an entity and instead reset to something a user can use..
3. If #existing_format is empty, preselect #default_format (if it's set and the user has access to it), or FILTER_FORMAT_DEFAULT otherwise.

Also, minor bikeshedding, but I think the name of the new #type would be much better off as something like 'rich text' or 'formatted text' (rather than 'text_format').

I've read #412016: Custom key name for #text_format. and #375907: Make text field use D7 '#text_format' selector and a couple of related issues and I agree that this is the right way to go.

I don't think we should go from filtered to non-filtered text -- it seems like it is up to the _developer_ should decide up-front. I'm wondering what the use case is to change the type in mid-air ... given that I can't think of any, this seems like the right way to go.

I'd love to see us prioritize this, but in absence of code, we can go ahead and hack the hack. :-)

I'd prefer to see us focus on this patch instead of #375907: Make text field use D7 '#text_format' selector but I'm comfortable committing #375907 because it comes with tests.

Parts of the tests in #375907 are re-usable for the work in this patch, and the parts that fail when this patch lands will tell us what code to undo/redo.

Ultimately, it comes down to timing. If someone volunteers to start work on this patch in the next day or two, I might hold back #375907. If not, I'll proceed with #375907.

It's not about hacking the hack, but about consistency. Working on a patch for this issue is much easier if all of core uses the current implementation consistently. Whether #412016: Custom key name for #text_format. is required or should come first, is certainly dependent on how many other issues are blocked due to it.

I support this, and I also agree with David in #10. "richtext" would make more sense than "text_format". Assuming that "richtext" actually means what we think it means, and is not just a Microsoft document format.

Aside from bikeshedding, a bigger design consideration: Can the element receive an array of selectable filters? Will this array intersect with user access control?

Before we get to work on a patch, there should be a code example of setting this element in the form builder, and saving its submitted contents.

Say,

function richtext_form() {
$form['body'] = array(
  '#type' => 'richtext',
  '#title' => t('Body'),
  '#description' => t('The text of your node.'),
  '#rows' => 8,
);

$form['pastebin'] = array(
  '#type' => 'richtext',
  '#title' => t('Syntax highlighted code'),
  '#description' => t('Paste your code'),
  '#formats' => array(4, 13, 21), // eg. PHP, CSS, Javascript syntax, subject to user_access.
  '#rows' => 16,
);
}

function richtext_form_submit($form, &$form_state) {
  $object['body'] = $form_state['values']['body']['text'];
  $object['body_format'] = $form_state['values']['body']['format'];
  richtext_save($object);
}

Please adjust to how you think the element should work.

#14 goes way too far for D7.

However, we need to clean up the new API feature we introduced.

In a quick IRC discussion, Heine had the frickin' awesome idea to ditch filter_form() and turn it into a #process for this new #type.

A proper #type name is not really clear, but since the UI refers to "text format", the API documentation refers to "text format", the most natural form element #type seems to be 'text_format'.

So this is what I think would make sense.

The patch will fail, because there's a certain amount of direct filter_form() invocations that fatal out. Let's focus on the architectural stuff first.

+++ modules/filter/filter.module	9 Sep 2009 02:00:38 -0000
@@ -589,28 +611,101 @@ function check_markup($text, $format = F
+  // @todo The default was 'format' previously, only the #text_format property
+  //   resulted in 'PARENTS_format'. Is that an issue at all?
+  $parents = (isset($element['#parents']) ? $element['#parents'] : 'format');
+
+  // Determine the form element parents and element name to use for the input
+  // format widget.
+  $parents = $element['#parents'];
+  $element_name = array_pop($parents);
+  $parents[] = $element_name . '_format';

ok, seems we have a conflict here. #412016: Custom key name for #text_format. was all about adding a consistent #text_format_parents (or similar) property that Field API could use to assign different #parents for the text format, but NOT for the textarea/textfield.

So it seems we indeed have a name clash here. #format_parents to the rescue?

I'm on crack. Are you, too?

Implements #format_parents, defaulting to KEY_format if not defined.

oh, wow, Field API makes such patches much smaller than I initially thought. :)

Tried to revamp the patch, but I'm not entirely sure what to do with #process + inheritance of the properties. We can't reset everything to element_info($type_minus_format), because hook_form_alter() wouldn't have an effect anymore.

So it seems that we need inheritance of properties in either case. Next best question is whether 'filter_process_form' should come first or last in #process. My impression was that it should come first, because other #process functions want to work on the respective, injected element instead.

Introducing a new tag for feature freeze: API clean-up.

I'm trying to port Wysiwyg module to D7. As of now, I have no idea how to determine/access all elements in a form that may or may not have a #text_format property, so I'm basically back to recursing into the entire form to find all candidates. Not good.

Hence, this is a release blocker.

Tagging absolutely critical clean-ups for D7. Do not touch this tag.

1) This is a release blocker, as stated by the security team lead, and I agree.

2) I'll work on a patch today.

3) I can't promise to get it to fully work today, but I'll try hard.

First of all, re-rolled that approach against HEAD. CNW, 'cause it will fail equally.

Learned some new rendering stuff in the past days, so I'll try to come up with a sexy solution.

Back into business.

HEAD was broken.

ok. I badly need help here.

This patch could be considered complete, if there wasn't this total mess of a text.module + Field API... I debug_backtrace()'d to figure out what exactly is guilty in the past hours, but I still have no clue at all.

Here's what I can provide:

1. Apply the patch and go to node/add/article.
2. Fill in a title, summary, and body for the node, and hit Preview (that's sufficient to replicate).
3. The body in the preview as well as in the form values will have the 1st character replaced with the chosen text format id.

Additionally:

• It does not seem to be caused by the altered text_field_widget_formatted_text_value().
• text_textarea_with_summary_process() already gets the malformed values in #default_value, but not in #value
• text_field_widget() already gets the messed up values in $items[$delta], this whole thing must be caused earlier in the process.
• In node_form() as well as field_attach_form(), the values are still correct (IIRC).
• Additionally assigning #array_parents in filter_process_format() does not help.

<rant>
And, btw, we can't release with this mess of a text.module. For one, I have no clue why it doesn't simply use the already existing #types. Secondly, everything happens in #process, so no one except Form API experts will be able to alter those widgets, since there is *nothing* in hook_form_alter() that you could alter. However, that is probably a different issue...
</rant>

subscribe

Hrm. Sorry, that error was rather a Form API hickup.

To make any sense of text.module during debugging, I *had to* remove this fake Form API #type along with the strange #process functions.

So this should actually work -- and pass the tests.

I can't think of a cleaner solution. And this really removes a bloat of WTFs.

ok, this is only failing, because all tests need to be updated to post and check the form values for blocks, comments, nodes, and their respective text formats in the proper locations... that will take some time.

So, before doing that, I need feedback/thumbs-up/confirmation/approval for the taken approach in this patch.

I took a look at this patch.

Overall it makes sense and looks very nice, especially with all the code that it removes!
12 files changed, 254 insertions(+), 392 deletions(-)

This isn't a super in-depth review (especially not of the Field API parts), but...

1. +  $type['textfield_format'] = array(
   +    '#process' => array('filter_process_format'),
   +  );
   +  $type['textarea_format'] = array(
   +    '#process' => array('filter_process_format'),
   +  );
   

   At first glance, I liked your previous suggestion (from comment #4) of using #rows to avoid having two separate elements (or maybe if not that, using some other property to indicate the type) - any reason for not doing that? There are definitely places in the patch that seem pretty odd due to the "_format" convention used here, e.g.: + $element['value']['#type'] = strtr($element['#type'], array('_format' => ''));....

   Also, the names still sound awkward to me - I think the problem may be that they make 'format' sound like the object, but 'textarea' is the actual object. The best I can come up with is 'formatted textarea' (or 'formatted text' if using one element). It's still not great.

2. +    // Signal filter_process_form() to not expand the form element into 'value'
   +    // and 'format', since we need all submitted form values on one level.
   +    $element[$field_key]['#format_tree'] = FALSE;
   

   The code comment should explain why we need all submitted form values on one level, because I don't understand why :) I assume it's because the Field UI submit handler expects it that way?

   Ideally, the Field UI would take care of pulling array elements out of $form_state['values'] itself, but I have no idea if that's simple or not.

3. +  // We cannot re-use #tree itself here, because form_builder() recursively
   +  // takes over the parent element's #tree property automatically.
   

   I didn't really understand this comment. Other process functions (for example, checkboxes) set #tree... why can't this one?

4. +  $element['#type'] = 'markup';
   

   I realize the previous code did this too, but I don't understand why it should have to... again, other process functions don't do anything like this, right? As far as I have seen, they just leave the #type alone and expand out the children and everything works OK.

5. +  $element['#theme_wrappers'] = array('filter_format_wrapper');
   

   Similarly, couldn't this have been defined in hook_element_info?

6. +  // Use the default format for this user if none was selected.
   +  if (empty($element['#format'])) {
   +    $element['#format'] = filter_format_default($user);
   +  }
   

   With only a few more lines of code here, I think it would be easy to implement what I suggested in #9 (separate keys for #existing_format and #default_format).

   I think we should do it, because module developers are going to try to hook_form_alter() this to change the default format, and keeping them separate makes it simple for them to do that without risking security issues.

7. +  // Get a list of formats that the current user has access to.
   +  $formats = filter_formats($user);
   

   Similarly, wouldn't it only take a couple more lines of code here to do something like what @Arancaytar suggested in #14? For example:

     // If a list of formats is provided, limit the format choices to the ones
     // from that list that the current user has access to (while making sure
     // that the current selected format is among them).
     if (isset($element['#format_options'])) {
       $formats = array_intersect_key($formats, array_flip(array_merge($element['#format_options'], array($element['#format']))));
     }
   

   This is maybe less critical, but it's also something module developers are going to want to do and it also has some security implications if they get it wrong. I think as long as we are changing the API here anyway, it's best to get the API right.

8. +  $process_key = array_search('filter_process_format', $element['value']['#process']);
   +  unset($element['value']['#process'][$process_key]);
   

   Using array_diff() might be cleaner here?

9. -function filter_default_format($account = NULL) {
   +function filter_format_default($account = NULL) {
   

   This function renaming (as well as a few other cleanups in the patch) seems unrelated to the issue. I think it would be best to remove them, especially since we are in code freeze and the patch is already somewhat complex...

   Also, if we rename filter_default_format(), I think we would need to similarly rename filter_fallback_format() for consistency?

About 'widgets as FAPI #type expanded on process' (re sun #36 - #39) :
That approach was introduced in CCK D6 in an attempt to make field widgets reusable outside CCK (as in webform.module components, custom forms etc...), or to have widgets reuse one another (for instance noderef autocomplete widget reused text.module's textfield widget).

I don't think it really lived to the expectations so far. And the trend from D7 up is now 'make more stuff fieldable' + 'make powerful, Field-agnostic FAPI #types' (e.g this 'text_format', quicksketch's file uploader) rather than 'be able to use powerful Field-specific widgets on non Field stuff'.

I personally wouldn't mind dropping this #type / #process approach and switch back to the simple approach where hook_field_widget() uses existing FAPI types.
As this (CCK D6) article by Karen explains, we're supposed to support both (see her example_widget() function and its PHPdoc at the end of the code) - except that we found in ParisDC that there were bugs in the simple approach; see #567064: Widgets done "the easy way" have too many limitations.

Also, most current core widgets (text, number, list, but not file and image) use that 'expand on #process' pattern; so they would need to be converted too...

@David_Rothstein:

1. On #rows: I know I suggested it earlier in the first place, but I no longer think it is a good idea to have one #type that can mean multiple things. One #type means one Form API widget, and not following that pattern would be highly confusing. Furthermore, IIRC, CCK 1.x had this "feature", but it was removed later on.

   On the new #type names: I think the names are good for consistency, because the difference between the "textarea" and "textarea_format" Form API widgets is very clear at first sight. This convention would basically also allow to text-format-enable other Form API #types in the very same way as being done for textfield and textarea. (Not sure whether there are any possible use-cases, but at least, it would be possible.)

2. As perhaps visible from my previous comments/rants, I tried to understand Field API's form value handling for several hours. It seems to boil down to the fact that any Field API widget is supposed to deliver all form values at the array (parents) level of the field widget in the form, 1:1 matching the field's columns in the storage.

   I tried several other approaches before the totally simple solution of this patch crossed my mind. First, I tried to revamp the #value_callback function that existed before, which didn't work out, because Form API's input value handling could not be stopped from recursing further into the form (and messing up the already set #values for the widget). Second, I tried to assign hard-coded #parents in the text field widget itself, so filter_process_format() would just add children to the element's parent #parents level, but that really required a lot more explanation.

3. Lastly, I figured that all we want is to disable the #tree behavior to make the new children in the submitted form values appear on the same level as the original element itself.

   However, I first tried with the #tree property itself, which worked fine for Field API widgets, but not for implementations like custom blocks. That is, because the custom block body Form API widget does not specify #tree at all - and Form API's form_builder() recurses into all elements in a form and inherits the #tree property for all children from the parent element, if not explicitly defined. Hence, since #tree is not defined, it is inherited as FALSE by default - which means that regular implementations would get 'value' and 'format' at the top level of the submitted form values, instead of the expected location further down the tree.

   Therefore, I went with a property #format_tree that is custom for filter_process_format() and specifically tells whether to enable/disable the extra-parenting for the expanded element.

   A ugly workaround for that would be to force developers to properly assign #tree => TRUE/FALSE for *_format #types.

   Now that you mention it, it may be possible to define #tree = TRUE as default in filter_element_info(). We can try that. :)

4. On #type = 'markup': This was required to make the original element disappear, i.e. disable input processing and outputting no Form API widget at all. Other #process functions need to do this as well, in case they expand a #type that would output a regular Form API widget normally.

   Now that you mention it, #type = '*_format' does not output anything, so we might be able...

5. No, now that you mention that as well, here's the reasoning: The original element can have arbitrary #theme and #theme_wrappers (manually) applied. Manually applied properties are not overridden/expanded with the definition in hook_element_info(). So, to prevent the original element from being processed by theme wrappers that were intended for the original element (which lives in the sub-key 'value' now), we need to override those properties.
6. On your #existing_format/#default_format: I specifically changed those property assignments to either assign the "existing" (when editing) format id or just NULL (when no format info is present yet). Primarily, to decrease the required API knowledge for developers working with text formats. Previously, developers had to use the FILTER_DEFAULT_FORMAT constant. Since that is gone now, they have to invoke a function that retrieves the default format. All of that really is logic that can be handled internally by filter_process_format(), and developers only need to set the stored #format or NULL if none is stored.

   Developers cannot use hook_form_alter() to change this behavior, because #process runs after form alterion. However, while developing this, I specifically had contributed modules like Better Formats in mind - which would just add another #process function after filter_process_format() via hook_entity_info_alter() to do their customizations. However, it is the responsibility of such a module to not break security (and I'm not even sure they are able to do so, because everything is still validated by Form API and Filter API).

   So the approach taken here is KISS.

7. On #format_options: That is definitely outside the scope of this issue, and, the module defining the form wouldn't even know what to define here. So it would boil down to having a Better Formats module that assigns this property. But, a module like that can equally simply define a #process to do its magic. So I'm very opposed to that proposal.
8. On array_diff: Possible, we can try.
9. On renaming filter_default_format(): Yes, we can revert that, because I only later found out that we can just assign NULL or nothing at all if there is no stored format for something. Hence, this function is called only internally now, so its messed up name of subject_verb_subject() doesn't matter at all anymore.

@yched:

1. I think that CCK tried to introduce a separate "widget" layer on top of an widget layer that already exists. That's why above explanation is very confusing, because I needed to talk about both Field API widgets and Form API widgets.
2. A separate layer of widgets, à la "text_textarea" doesn't make sense at all to me, because the form element is still a "textarea".
3. Hence, Field API widgets should re-use the existing Form API widgets (#types). That, of course, does not mean that Field API widgets are not re-usable. However, we should understand them as "wrappers" around the existing Form API #types, i.e. a Field API widget may consist of one or more arranged Form API widgets. A nodereference field widget can still use #type = 'textfield', but I don't see any connection to a text field widget here.
4. Furthermore, to completely enable a separate form widget layer on top of the existing, we would have to re-implement the entire Form API handling for those.
5. #567064: Widgets done "the easy way" have too many limitations nicely explains what I discovered in my debugging as well: The default values, which are assigned after hook_field_widget() has been invoked, do not have any effect on field widgets like this, because the $element they are applied to may not even be of a #type that outputs a form element. What I do not understand is why we do not simply prepare those values before hook_field_widget() is invoked and pass them on in $field and/or $instance, to let the field widget handle them properly.
6. Please note that #type 'textarea_format' and 'textfield_format' are not Field API widgets, but regular Form API elements.

I wouldn't put it as in "@yched bullet 1", because CCK D6 also got rid of 'CCK specific' bits and tricks for existing FAPI stuff, but I basically agree with the rest ;-).

What I do not understand is why we do not simply prepare those values before hook_field_widget() is invoked and pass them on in $field and/or $instance

field.form.inc has to massage some of the $field / $instance properties because for instance, for a 'required, multiple' field where we call hook_field_widget() several times, only the first widget should be '#required', and the title (label) and description are displayed by field.module above and below the table and should be left empty by the widgets themselves.
I don't think we want to fake these into the $field and $instance params we provide to hook_field_widget(), this probably needs a separate $options parameter or whetever.

No biggie in itself, other than updating all existing widgets, and the bumps we'll find on the road. Widgets are funky stuff :-(

Here's a proof of concept refactor of field.form.inc and hook_field_widget(), based on patch 39.
We provide hook_field_widget() with a $base array, containing the values to be used for #title, #description, #required.

This applies fine for text widgets as sun refactored them, but will choke on any other field type for now. I'm afraid this same patch will have to convert all existing widgets...

To check that it works, it should be tested with a required, multiple text field.
(Also, note that I couldn't get formatted widgets to actually display with patch #39, so be sure to use an unformatted text)

I took my proposal into a separate patch in #567064-4: Widgets done "the easy way" have too many limitations, which is thus now a prerequisite for this issue in its current (non-#process) approach for the text widgets.
So forget the patch in #46, current state is #39. Sorry for the hijacking.

#567064: Widgets done "the easy way" have too many limitations is in. Re-rolled against HEAD.

Due to that, text.module turns into a really nice and clean implementation with this patch.

Working on the other stuff from #44 now.

FYI: #626354: Remove #process pattern from number field starts with a patch to 'simplify' number widgets.

I think we can also ditch the abstraction on 'columns' inside the text widgets. They will be used on text fields, let's not pretend we don't know that $element['#columns'][1] is 'value'.

- Clarified why #tree does not work in an inline comment. (wrong assumption)

- Implemented the array_diff() proposal by David. Nice one.

- Cleaned up quite a lot of the processing in filter_process_form(). Much nicer now.

- Reverted renaming of filter_default_format().

- Moved JavaScript for the text format widget into filter.js.

- Fixed #array_parents processing in form_builder().

- Fixed theme_container() to respect existing #attributes.

I will now proceed to fixing all the tests, which will probably take some hours.

So let's see where we are...

AAAAAAAAAH!

So who spots where we did go wrong in this entire #text_format thingy here?

So.

Let me provide some hints for you. :P

+++ modules/block/block.module	8 Nov 2009 02:01:39 -0000
@@ -423,10 +423,11 @@ function block_custom_block_form($edit =
+    '#format' => isset($edit['format']) ? $edit['format'] : NULL,
+    '#format_tree' => FALSE,
+++ modules/comment/comment.module	8 Nov 2009 01:19:36 -0000
@@ -1841,11 +1841,12 @@ function comment_form($form, &$form_stat
+    '#format' => isset($comment->format) ? $comment->format : NULL,
+    '#format_tree' => FALSE,
+++ modules/field/modules/text/text.module	7 Nov 2009 02:28:49 -0000
@@ -569,8 +530,58 @@ function text_field_widget(&$form, &$for
+    $element[$field_key]['#format'] = isset($items[$delta]['format']) ? $items[$delta]['format'] : NULL;
+    $element[$field_key]['#format_tree'] = FALSE;
+++ modules/taxonomy/taxonomy.admin.inc	8 Nov 2009 01:50:03 -0000
@@ -651,11 +651,12 @@ function taxonomy_form_term($form, &$for
+    '#format' => $edit['format'],
+    '#format_tree' => FALSE,

So what could be the issue?

I'm on crack. Are you, too?

So where did we go wrong?

1. #125315: Textarea with input format attached introduced a #text_format Form API property.
2. It introduced a #process callback that takes the original element 'foo' and adds another element next to it 'foo_format', which holds the format - changing all implementations accordingly.
3. This implementation was done under the assumption that we do not want to override (or potentially nuke) another existing 'format' key on the same form structure array level as 'foo'.
4. Hence, we chose to go with a suffix of 'foo_format' for the submitted text format widget value.
5. The reality.
6. However.
7. Is different.
8. There is not a single widget nor implementation in core that really wants this, and I can hardly think of any implementation in contrib that would want this.
9. Instead, all implementations in core - including Field API - want the 'format' value to appear next to the original element.
10. That's because form implementations mostly follow the data storage. Which means that you have 'comment' and 'format'. Or 'value' and 'format'. Or 'description' and 'format'.
11. If you really have two text format enabled values at the same level, then there is no reason why you couldn't change your form structure, say, from $form['foo'] to $form['foo']['foo'], to allow for a separate text format enabled form element $form['bar'] on the same level. Or, alternatively, if you know about #parents, then you just assign custom #parents (just like this patch does).
12. The reality. Again. Is different. We are far away from abstract representation of stored values in forms.
13. Hence, introducing the option to allow for a different representation is nuts on its own. No one is going to use it. And everyone will have to code around the optional edge-case.
14. Hence, we remove this awkward stuff now.

I'm a bit embarrased about myself, because I didn't realize this earlier. It made us waste the entire time between #10 and #57, plus countless of hours of trying to make this totally optional egde-case work. :(

Yes, it basically means to go back to D6. But no entirely.

This patch is almost the same as the previous, but without the conditional support for expanding the original element into separate 'value' and 'format' sub-keys in the submitted form values.

So what's about this patch?

1. We remove a clusterfuck of mad assumptions and workarounds from core.
2. Text format enabled elements use their own #type, i.e. 'textarea_format'.
3. Those elements are automatically expanded into text format widgets, without nasty invocations of filter_form() or filter_default_format().
4. The custom #format property indicates the preselected text format, and it can be NULL, so no developer has to really care about the filter system innards. Either there is a stored format or there is not. And if there is one, you simply assign it.
5. The original element expands into two form elements, 'value' and 'format', but only 'format' will end up additionally in the submitted form values.
6. All text-format-enabled elements can be already be altered within hook_element_info_alter(), to potentially apply further #process callbacks and whatnot - instead of recursing into the form and searching for a #text_format property.
7. Custom form elements (e.g. 'foo') can be text-format-enabled by registering the #type 'foo_format' accordingly in hook_element_info().
8. The Text field widget is revamped accordingly to #567064: Widgets done "the easy way" have too many limitations and #626354: Remove #process pattern from number field.

+++ modules/field/modules/text/text.module	8 Nov 2009 03:41:48 -0000
@@ -569,8 +530,57 @@ function text_field_widget(&$form, &$for
+      $element[$field_key] = array(
+        '#type' => 'textfield',
(...)
+        '#title' => $instance['label'],
+        '#description' => $instance['description'],
+        '#required' => $instance['required'],

We don't want to do that. For #title, #required and #description, we want to use the properties that were set in the incoming $element passed by Field API to hook_field_widget.

This the reason why in #567064-14: Widgets done "the easy way" have too many limitations I was promoting a $base argument, which you freely use in the $element you build, instead of a 'pre-cooked' $element that you add to. Because more often than not, those properties wlll not be used directly on the $element you build. Most of the time, you'll want to do

$element[$field_key] = $base + array(
  '#type' => ...
);

Just a variable name stuff, really, but providing those values to hook_field_widget() in a variable named like the form part it's going to return (usually, by convention, '$element') is really misleading - as we see here ;-)

Right. Thanks for reminding! So we can remove even more code here :)

The approach we took allows each widget to decide whether $element is $element or $base on its own. Nifty! ;)

The approach we took allows each widget to decide whether $element is $element or $base on its own. Nifty! ;)

Hm. Not untrue. I didn't think of that. number widget should do the same, I guess. Might warrant a comment, though ?

My last remark on the patch is that I think we should ditch the $field_key = $base['#columns'][n] abstraction, and use the actual column names for text field ('value', 'format', 'summary'). Those widgets will be used for text fields, period. The code is now simple enough that a different field type with different columns that wants similar widgets can just reimplement them.
This abstraction IMO only makes sense to keep in options.module widgets (select, radio, checkboxes), since they're more tricky, and are already used by list and taxo fields (and will be used by noderef / userref in contrib). But text.module is the #1 module people look at to build their own widgets, it's best to keep it simple.

This can also be done in a separate task, if needed.

There is not a single widget nor implementation in core that really wants this, and I can hardly think of any implementation in contrib that would want this. Instead, all implementations in core - including Field API - want the 'format' value to appear next to the original element...

If you really have two text format enabled values at the same level, then there is no reason why you couldn't change your form structure, say, from $form['foo'] to $form['foo']['foo'], to allow for a separate text format enabled form element $form['bar'] on the same level.

It's very, very neat that it's possible to keep 'format' on the same level without hurting the Field API, since that's certainly the 95% use case for a form that would have more than one format-enabled textarea. I would never have thought that was possible :) However, I'm still quite a bit worried about the other 5%. With this patch, if you build a form with two format-enabled elements on the same level, the format of the second will completely clobber the first. It seems like it will be totally not obvious to a module developer how, why or even if it is doing that, or what the method is for fixing it. This clobbering can lead to weird bugs and even security issues (see below).

In addition, we need to consider what happens with hook_form_alter(). Basically, with this patch the recommended procedure would need to be that if you are adding a format-enabled element via hook_form_alter(), you always must put that element inside a structure with #tree = TRUE, because otherwise the auto-generated 'format' key might conflict with that of someone else's, and you can't predict beforehand whether that will happen. Again, this seems quite different from anything else in form API.

I realize tons of work went into trying the previous solution and it caused big difficulties with Field API, but overall it really seems to me that having a form submit handler receive $form_state['values']['something']['value'] and $form_state['values']['something']['format'] is natural because those two things are inextricably linked together as part of 'something' -- even if it does lead to a bit more code in some places for processing $form_state['values'] before putting it in the database. And if we really can't do that, then I think we need some serious documentation somewhere (I'm not sure where?) explaining to module authors exactly how they should be adding format-enabled elements to forms and how to deal with the case of more than one of them.

+ *   - #type: A form element #type suffixed with '_format'. The form element for
+ *     'value' will use this #type without the '_format' suffix. For example,
+ *     if the to be processed form element specifies 'textarea_format', then the
+ *     new child element in 'value' will use the #type 'textarea'. By default,
+ *     Filter module registers 'textarea_format' and 'textfield_format' in
+ *     filter_element_info(). Custom implementations needs to register the
+ *     suffixed '_format' types in hook_element_info().

While I do like the power of being able to format-enable any element type, the part where we express that relationship by magically appending '_format' still makes me uneasy. Since they have to register these things in hook_element_info() anyway, maybe we can make the relationship more explicit rather than relying on magic naming? For example:

$type['textarea_format'] = array(
  '#base_type' => 'textarea',
  '#process' => array('filter_process_format'),
);

Seems to me like it would make the code cleaner and easier to understand if we used an explicit property like '#base_type' to specify this information.

+    '#type' => 'textarea_format',

I'm still not a big fan of this name, but I also still don't have an amazing suggestion for improving it :) However, perhaps 'textarea_with_format' would be better? I think this is easier to read, plus it fits the pattern that is being used elsewhere (e.g., 'text_textarea_with_summary').

+ * Which results in the submitted form values:
+ * @code
+ *   // Original form element value.
+ *   $form_state['values']['some']['field'] = 'Some to be filtered text.';
+ *   // Chosen text format.
+ *   $form_state['values']['some']['format'] = 1;
+ * @endcode

If I read the code example correctly here, 'some' would not actually appear in the $form_state, because the code above this never set #tree to TRUE...

-function filter_form($selected_format = NULL, $weight = NULL, $parents = array('format')) {
+function filter_process_format($element) {

This is indeed a great change, but it seems the new function is very large and trying to do two totally different things at once:

1. Complex form API manipulation and filling in of security-related defaults for the format.
2. Theming-related stuff, like adding the filter guidelines, theme wrapper, etc.

I think it would be better to split this up into two separate process functions, since modules might reasonably want to totally replace the second one (but probably don't ever want to touch the first).

   // Get a list of formats that the current user has access to.
   $formats = filter_formats($user);

...

+  // Use the default format for this user if none was selected.
+  if (empty($element['#format'])) {
+    $element['#format'] = filter_default_format($user);
+  }

I still think we should expand this to include the #default_format and #format_options properties discussed in previous comments. Given that this patch is totally replacing the existing API anyway, I don't see it as out of scope for this issue - it seems worth it to make sure we get the new API right.

Really, lots of modules want to introduce this kind of functionality of dynamically changing the list of formats and the default based on a variety of properties (content type, whatever). You said above that they can do it by adding their own #process functions and taking care of security on their own, but hook_form_alter() is generally a lot simpler, and wouldn't we want to provide a clear way to do it that takes care of security for them? (BTW, the security hardening part of this is a bit subtle - it is not that modules would be able to expose formats that the current user doesn't have permission to use, but rather that they would accidentally switch an existing format. For example, someone tries to type in an XSS attack on my site, which does not work because they are only able to save the content as Filtered HTML. Later, I as the site admin go to edit this content, and due to a bug the format selector accidentally defaults to Full HTML. So I go to save the node, without ever having touched the selector and therefore completely not noticing that it has switched on its own, and as soon as the node is saved the XSS attack succeeds.)

Again, it is really only a tiny amount of code we are talking about adding, and I mostly wrote it all above :) Rewriting it a bit for the current code and putting it all together, all we'd need to do is replace the second hunk of the above snippet with something like this:

// If no format is stored with the element, then:
// - If the element specifies a particular default format and the user has
// access to it, use that.
// - Otherwise, use the default format for the user.
if (empty($element['#format'])) {
  $element['#format'] = !empty($element['#default_format']) && isset($formats[$element['#default_format']]) ? $element['#default_format'] : filter_default_format($user);
}

// If a list of formats is provided, limit the format choices to the ones from
// that list which the current user has access to, while making sure the
// current selection is among them.
if (isset($element['#format_options'])) {
  foreach (array_keys($formats) as $format_id) {
    if ($format_id != $element['#format'] && !in_array($format_id, $element['#format_options'])) {
      unset($formats[$format_id]);
    }
  }
}

This tiny bit of code (which would be enhanced via #91663: Permission of text format is not checked when editing an entity and instead reset to something a user can use. to make sure to deny access whenever the current user doesn't have permission to use $element['#format']) would mean that modules could feel free to easily set the #default_format and #format_options completely however they want, and easily change them via hook_form_alter() based on whatever criteria they want, with all the security aspects taken care of for them behind the scenes.

I really badly need to get some client work done, so my reply will be short.

1. The 5% case does not exist in core. And based on my relatively large knowledge of contributed modules, the case also does not exist elsewhere.
2. The two format-enabled elements on the same level issue we can explain in PHPDoc.
3. #tree has a completely different meaning. This patch/implementation does not touch the #tree behavior. Instead, it assigns _always_ the proper #parents for the new born child elements, regardless of whether #tree is TRUE or FALSE. That is, because #parents overrides the #tree behavior in form_builder().
4. It is possible that sub-keys would be preferable from a certain perspective. However, not. a. single. implementation. neither. in. core. nor. in. contrib. wants. that. Instead, _all_ modules implement partially heavy workarounds to get the form elements and submitted form values back into shape. Hence, regardless of whether the perspective might have its point - the technical consequences do not. It's a WTF for implementors, a WTF for people trying to understand the code, a WTF for people trying to understand the code flow, and a WTF in terms of code bloat. Yes, we can add more documentation.
5. #base_type makes sense, and tinkering about your suggestion, we might as well drop the entire '_format' suffix that way. Just have a single, unique #type "text_format" or similar and specify #base_type = 'textarea' (which should be set by default in hook_element_info()). Hence, no clusterfuck with removing the '_format' suffix.
6. The #type name should be short and to the point. This is a form element, no field widget. As you can read from the above mentioned Field API widget issue and all follow-up issues on it, we just removed all #types for field widgets, because they were duplicating form elements. While I like #type 'text_format', I'm a bit unhappy about the namespace. 'filter_format' would work for me.
7. The example is possibly wrong, yes. As you can see from earlier comments (and as clarified in code), the #tree behavior is a bit special.
8. Splitting that other stuff out, potentially into an own #type with its own #process, sounds not bad. However, I'm not sure how much sense it makes, because those elements heavily depend on the parent element and they wouldn't have access to the parent element. Rather doable is possibly a dedicated #theme function, which allows to alter the output.
9. Again, no. The form builder functions will not specify any #default_format or #format_options, because that would be total code bloat, and, the developers of form builder functions should NOT have to care about that. If the form builder functions will not specify this, then your module will have to add a #process anyway to inject #default_format and #format_options. If your module needs to implement an additional #process anyway, then I see no point in supporting this in the first place. I will not agree to this for D7. And my plans for the D8 filter system look entirely different anyway.
10. I already stated in #91663: Permission of text format is not checked when editing an entity and instead reset to something a user can use. that this patch is the corner-stone to eliminate the issue. And we do not need additional #default_format and #format_options properties for that.

Please let me know what needs to be done to get this in.

The 5% case does not exist in core. And based on my relatively large knowledge of contributed modules, the case also does not exist elsewhere.

It might not exist because it was either impossible, or a royal pain to date. I can certainly imagine it being possible, especially with CCK in core. Going forward, we'll see bigger and more complex applications being built on Drupal. Maybe I'm missing something, but it doesn't sound crazy to have a form with two textareas that each have input formats. Do you disagree with that sun -- or am I missing something?

The case certainly exists. But not in the way we are foreseeing it here. It was already possible before, and you had to assign special parents for filter_form() to not overwrite the 'format' of another element.

Use-case 1) Multiple fields next to each other. No issue here, because fields use a more complex form structure anyway.

Use-case 2) A field widget containing multiple text format-enabled form elements next to each other. Due to field storage, those need to be in a hierarchy or use special #parents anyway. Sidenote: We'll rather see the rise of a "combofield" or "multigroup" field that allows to add multiple fields as one field to an entity.

Use-case 3) A custom form wanting to use multiple text format-enabled form elements next to each other. As mentioned before, I do not know of any module that needs that, and with the rise of fields in D7, more and more stuff will use text fields. Even if the use-case really exists, it can be done by using a hierarchical form structure or just by assigning custom #parents.

Use-case 4) A form_alter that wants to add a text format-enabled form element to a form structure on a level that already contains a text format-enabled form element. In almost all cases, this very form_alter is required to set custom #parents anyway, so the submitted form values do not end up within the form values that will be processed by the module that "owns" the form and the form values can be properly extracted for custom storage.

As already mentioned in #59, I think that already the current implementation of the #text_format property suffered from trying to make it "also work" for the almost non-existing edge-cases and thereby required us to introduce plenty of workarounds and hacks to make it work for all known cases.

If anyone in here is actually able to point me to a module that would suffer from this, then I'd perhaps reconsider my position. But as of now, I see lots of ugly code that's just a workaround for a problem we introduced earlier.

Regarding points in #64:

2. Yes, documentation of these cases seems like a must, even though they are rare (since they'll be totally unexpected if someone encounters them and it's not documented.) Otherwise, I'll stay out of the decision :)

3. Hm, the patch may not touch the #tree behavior, but #tree is still required in the way I'd expect it to be. I can't decide from your description if you think this is the way it's supposed to work or not, but here's what I tried. First, I put the following code at the bottom of the form builder for custom blocks:

  $form['test1']['test2'] = array(
    '#type' => 'textarea_format',
    '#title' => 'test',
    '#format' => NULL,
  );

As I expected, the text format selected for this element clobbered the one selected for the block body.

Then I added this line of code:

$form['test1']['#tree'] = TRUE;

After that, the block body text format selector worked correctly.

So regardless of what is going on internally with array parents, it seems to me like from a module developer's perspective, #tree is still what you need. (I think this also makes sense, by the way.) @sun, is that the behavior you expected from this code or not?

5. Yes, that is great! A single element that defaults to a #base_type of 'textarea' makes total sense.

6. OK, given that we are now back to finding a name for a single element rather than a bunch of them, that puts us back to where we were at the beginning of this issue, where type names like 'richtext' and 'formatted_text' were floated around. The problem I see with a name like 'text_format' is that a "text format" in Drupal refers to the format itself. But the element you're adding to your form isn't just a text format - it's a text area with a format attached to it. So I think 'text' needs to be the noun here.

Anyway, this can be the last thing we fiddle with before the patch goes in. But my vote right now is '#type' => 'formatted_text'.

8. I actually wasn't necessarily thinking of two separate elements with two separate #types but rather a single element with two separate process functions. So in hook_element_info():

$type['formatted_text'] = array(
  '#base_type' => 'textarea',
  // The first function handles the internal form API stuff and default format
  // selection. The second function turns it into a select box, adds JS and CSS,
  // adds the guidelines, etc.
  '#process' => array('filter_process_format', 'filter_process_format_selector'),
);

A dedicated theme function does sound like it might work also (and might actually work better). Either way, this would be good, but doesn't seem totally critical to getting the patch committed.

9. The proposed #default_format and #format_options properties are totally optional - no form builder function has to care about them unless it wants to. You can see from the code I wrote in #63 that in the case where an element doesn't specify either of these properties, it reduces to behaving exactly like the code that you already have in the patch.

As far as I can see, without adding these options, modules like Better Formats (and that's not the only one) that want to do this kind of thing actually would need to do it in two separate functions - they need to initially modify #format to set a default before the filter module process function acts (because as soon as the process function moves it away from being NULL they lose context of when it is safe to modify or not), and then they need a separate process function to modify the selector #options after the filter module acts (because the options don't exist until then). And while doing that, they more or less need to duplicate the code I wrote above anyway, if they want to get it right. So why not move these ~10 lines of code into core where they are centralized and let modules safely use and manipulate these as optional element properties just like they normally would anything else?

In most cases I wouldn't continue to push this point (because in theory it could be a followup issue), but for this patch we are going to be breaking the code freeze anyway, so it's important to break it only once and make sure it is right the first time.

Straight re-roll against HEAD for now. I'll come back to + answer the last follow-ups, but I'll extract the rather unrelated changes to text.module and form.inc into separate issues + patches first to decrease the size of this patch.

@yched: See you in #639428: Remove #process pattern from text field :)

All extracted changes have been committed.

Re-rolled against HEAD.

Three conflicts were introduced within two weeks after the patch was uploaded. Two definitely trivial in php.test and filter.module (too much fuzz), and one in common.inc that probably shouldn't be a problem. Here is a resolution - let's see if it passes.

Whoops, forgot to diff a new file.

What is it with me?

Now I somehow managed to drop the changeset in text.module.

ok. I already lost hope in trying to solve this, but finally was able to come up with a solution (and fix for form.inc).

This is the result of a discussion with Damien in IRC some time ago. While my arguments raised earlier in this issue still hold true, I can see the general sense and reasoning for expanding a form element into two proper child elements 'value' and 'format', and to submit those values accordingly in separate 'value' and 'format' sub-keys.

Now, this patch is the compromise of both, the theoretical and security-wise correct case of having separate 'value' and 'format' keys, and the practical reality of not having (and knowing) any implementation that would want those form values to appear as sub-keys of the original element.

Therefore, this patch sets a #value_callback for both child elements by default. Those default #value_callbacks make the submitted form values appear next to each other on the same level, as mentioned earlier in this issue. They solely work off from the assigned #parents of the original element. So if your module needs multiple text format-enabled elements on the same level in the form structure, then you can simply assign different #parents. Alternatively, you could remove the #value_callbacks and do your own, custom processing (or nothing at all). All of that is also explained in PHPDoc.

This means that values in $form_state['input'] are not touched. Only values in $form_state['values'] are altered, so that form submit handlers will get them. Example from the PHPDoc:

/**
 * For example, considering the input values:
 * @code
 *   $form_state['input']['body']['value'] = 'foo';
 *   $form_state['input']['body']['format'] = 'foo';
 * @endcode
 * The value callbacks will process them into:
 * @code
 *   $form_state['values']['body'] = 'foo';
 *   $form_state['values']['format'] = 'foo';
 * @endcode
 */

This patch also incorporates the #base_type suggestion we discussed earlier.

I hope we can agree on this one. It simplifies a lot, while still keeping sanity.

While a proper review is not in my time frame right now I am OK with the form.inc changes. You might call that a bugfix actually.

Quickly discussed the form API change in form_builder() with chx. We investigated whether this code shouldn't live directly in form_set_value(), but no, it seems to be right location, because we only want to conditionally skip the form_set_value() during automated $form_state['values'] processing in form_builder(), resp. input handling. Manual invocations (overrides) of form_set_value() should still be able to reset values in $form_state['values'].

oh my, requires update of all tests that happen to post a value to text format-enabled form elements somewhere. :-(

First pass.

This is a major challenge. Submitting a node form with an empty body doesn't work yet, and I don't really understand why. It works properly if body is non-empty, but not if not.

I didn't find the error submitting an empty node body, but the patch needs a re-roll due to comment changes.

Re-rolled against HEAD + comment body as field.

I like it!

$edit['comment_body[' . LANGUAGE_NONE . '][0]['value'][value]'] = 'some random comment';

Check the quotes and the syntax highlighting. You'll get an unexpected T_STRING error.

You need to lose the quotes around the first 'value'.

Let's try this one.

I forgot to diff the new filter.js, but apparently that wasn't the only problem...

Re-rolled against HEAD (node title field changes).

This is required to fix #91663: Permission of text format is not checked when editing an entity and instead reset to something a user can use.

Poor issue. [value][value]? We can live without that. Accidentally, it allows removing some code and changes too.

I have tweaked the after build function.

+++ modules/block/block.admin.inc	2010-01-20 18:28:55 +0000
@@ -439,11 +439,12 @@ function block_add_block_form_validate($
+  debug($form_state['values']);

debug :-)

+++ modules/field/modules/text/text.module	2010-01-20 19:39:52 +0000
@@ -553,11 +555,19 @@ function text_field_widget_form(&$form, 
+    // Conditionally alter the form element's type if text processing is enabled.
+    if ($instance['settings']['text_processing']) {
+      $element = $main_widget;
+      $element['#type'] = 'filter_format';

We should put the previously defined $main_widget['#type'] as $element['#base_type'].
Text Fields can use a 'textfield' widget and still hold formatted text.

Powered by Dreditor.

Incorporated yched's changes and added some fixes so some of the tests failed now pass for me.

Hm, it seems vocabularies have textfields as descriptions they are not textareas... Edit: they are not without format, that's what matters.

Wrong patch, sorry.

If I can make this pass then I guess it's time to assign to myself.

Removed debug from index.php, removed commented out code from filter.module and tried to modernize a bit the comments on filter_process_form.

#108: drupal-text-format.patch queued for re-testing.

Re-rolled against HEAD. This works great. Thank you very much for stepping in and finding the proper way to implement this, chx!

Can someone please summarize (or point me to the summary) on why this extremely, extremely late API change is a release-blocker and doesn't belong with D8?

1. Required to fix the long-standing security issue in #91663: Permission of text format is not checked when editing an entity and instead reset to something a user can use., which became critical in D7, since the naive workaround of previous versions is no longer doable.
2. Required to fix the security issue that #text_format introduced: it is simply not possible to simply switch non-filtered input to filtered input and vice-versa. The format needs to be stored and check_markup() has to be invoked elsewhere. The original assumption of "just add a #text_format property" was plain wrong.
3. The automatically named and re-parented "$key_format" value in submitted form values is an insane hack. Filtered input needs to properly end up in [value] and [format] subkeys that belong together and need to be processed together.
4. #text_format was introduced in D7 for "Better WYSIWYG support", but it actually did not improve anything. Only this patch does.
5. #text_format actually requires further hacks like #412016: Custom key name for #text_format., which was not committed, but marked as duplicate of this issue.

I'll look into that exception now.

That exception was caused by some improper testing code, which must have been introduced elsewhere since chx's last patch. Fixed that test, back to RTBC.

Excellent. Thanks for the great summary. I agree with the case for this being a D7 patch.

Upon closer inspection, it looks like the majority of this is just moving code from form.inc to filter.module, and doing some name swapping. The one that has me the most eeked is the fact that body is now not a text element, but an array of 'value' and 'format' (as opposed to body and body_format). I think that's going to be a god-send for WYSIWYG editors, but will throw a wrench at people who have already started porting their modules and writing SimpleTests. But the extra security/predictability this brings seems worth the trade-off.

The one thing I'm not clear on is why "filter_format" was the name of the text element chosen. "text_format" is much clearer that this will be a *text* field with a *format* underneath it. I would never guess from reading the code that "filter_format" gave me what I was looking for. I guess we're trying to name-space it or something, but we call text fields 'text' fields, not 'system_text' fields, so this feels unnecessary.

Can we swap the name? Then this is good to go for me. I also asked Damien to take one last look.

This is a really nice - and required - cleanup.

Frankly, I'm still not sure why we are moving the values higher up in the $form_state['values'] array (ie. why the form structure is different from the values structure), but if both chx and tha_sun are confortable with it, I can live with that.

In-patch rename of #type 'filter_format' to 'text_format' (also: theme_text_format_wrapper())

Tests passed, back to RTBC.

Awesome. Thanks for the quick turn-around.

Committed to HEAD. Thanks!

This change needs documenting in the module upgrade guide. Please do so ASAP, so that the docs are in place in time for alpha 3/beta 1.

This looks mostly good - although I don't claim to understand every bit of the Form API stuff in here - but I definitely (still) share Damien's concern. In particular, it looks like in order to make things slightly more convenient for certain form submit handlers, i.e., in order to make it more likely that someone can do this:

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

without having to massage $form_state['values'] beforehand, we are:

1. Adding a somewhat complicated-looking #after_build function to the filter module.
2. Requiring that anyone who potentially wants to hook_form_alter() a text-format-enabled textarea onto an existing form either always use a #tree structure (I think?) or else read and completely understand this:
   

   + * 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.
   

   And if they don't figure out what that means or how to do that correctly, the result will be some nasty, hard to track down critical bugs, not to mention security issues.

So essentially, I know it's been discussed a lot above, but it still really seems unclear why that proposed tradeoff is a good one?

****

Minor things:

+  foreach ($element as $key => $value) {
+    if ($key[0] === '#' && !in_array($key, $blacklist)) {
+      $element['value'][$key] = $value;
+    }

This should use element_properties() rather than explicitly checking for '#'.

+    '#value_callback' => 'filter_form_value_callback_format',

Maybe I missed it, but is this used anywhere?

+ * callback. If no custom processing occurs, then the submitted form values will
+ * appear like in the $form_state['input'] array above.

I found this sentence a little confusing... when it says "no custom processing", it really means no custom processing including the custom #after_build that the current function adds, right?

Crosspost with the commit, but above comments still stand.

Final tweaks: Removed trailing-whitespace and stale #value_callback.

Does this address David's comments in #121?

Hm. Doesn't look like it.

1) Re-rolled against HEAD.

2) We forgot to add filter.js before commit.

3) Incorporated David's feedback.

--

@David:

# Adding a somewhat complicated-looking #after_build function to the filter module.

That #after_build function is quite trivial. Very much simpler than the previous form_process_format() munging to get the value into $key_format.

# Requiring that anyone who potentially wants to hook_form_alter() a text format onto an existing form either always use a #tree structure (I think?) or else read and completely understand this.

No, #tree is applied automatically by filter_process_format(). By default, the (now) current default behavior is identical to D6. For your special use-case, you had to pass an additional $parents array in D6. In D7, you can either define custom #parents (same as passing $parents in D6), or alternatively, remove the #after_build.

D'oh! My bad. filter.js is committed now. Can we get a quick re-roll of #126?

filter.js was added.

Removed old #text_format docs, added new #type 'text_format' docs: http://drupal.org/update/modules/6/7#text_format

#128 looks good.

***

By default, the (now) current default behavior is identical to D6. For your special use-case, you had to pass an additional $parents array in D6.

D6 had issues too but I think the difference is that there your code would break entirely if you didn't use $parents, but here it appears to work for your module, but breaks others. (Plus we have a chance to correct it.)

For example, how many people can spot what is wrong with this D7 code, and why it causes the block module to break in a critical way (and possibly introduces a security issue)?

function mymodule_form_block_add_block_form_alter(&$form, &$form_state) {
  $form['mymodule_extra_field'] = array(
    '#type' => 'text_format',
    '#title' => 'Whatever',
  );
}

Also, by #tree I meant using that on a wrapper, not on the element itself. For example, if the above code is put in a 'mymodule_wrapper' element with #tree set to TRUE, then it no longer causes problems. Maybe at the very least we should be explicitly recommending that people use that when altering forms to add these kinds of elements.

Plus JS behavior fixes I found and fixed in #91663-96: Permission of text format is not checked when editing an entity and instead reset to something a user can use.

Committed #131 to HEAD. Since this is documented (yay! thank you!!), I think I can mark this fixed now.

David, I asked sun about your concerns, which sound quite valid to me. He said that the behaviour D7 is currently doing is the same as D6. So could we move your concerns to a separate issue about further hardening this code, and cross-reference here?

I created #735662: hook_form_alter() can easily clobber a text format

+++ includes/form.inc	7 Mar 2010 16:34:14 -0000
@@ -1435,7 +1435,15 @@ function _form_builder_handle_input_elem
-  form_set_value($element, $element['#value'], $form_state);
+  // Set the element's value in $form_state['values'], but only, if its key
+  // does not exist yet (a #value_callback may have already populated it).
+  $values = $form_state['values'];
+  foreach ($element['#parents'] as $key) {
+    $values = (isset($values[$key]) ? $values[$key] : NULL);
+  }
+  if (!isset($values)) {
+    form_set_value($element, $element['#value'], $form_state);
+  }

Please see #335035-89: drupalPost() incorrectly submits input for disabled elements for a fix needed as a result of this change.

The upgrade docs at http://drupal.org/update/modules/6/7#default-text-formats are still seem to be outdated.

.

There seems to be a bug in this new form element implementation, see #820816: #type text_format not compatible with system_settings_form($automatic_defaults = TRUE), please.

Updated http://drupal.org/update/modules/6/7#text_format