Widgets done "the easy way" have too many limitations

crosslinking to #414424-43: Introduce Form API #type 'text_format'

subscribing - left some further comments in the issue yched pointed to in #1

Posted a proof of concept patch in comment #46 over there.

Moved the proposed solution out of the patch in #414424-46: Introduce Form API #type 'text_format'.

hook_field_widget() receives the additional informations in an additional $base array param, that contains the #title, #description and #required properties.
It is then free to use them whichever way its sees fit, in simple cases by simply adding them (array addition) to the FAPI structure.

The patch converts all current core widgets to this approach.
I tried to minimize the amount of extra information added, compared to what we previously added in $default (see the snippet in the OP above). Each widget then has the ability to add the extra information it specifically needs. If, as per the discussion in #414424: Introduce Form API #type 'text_format' (comments 43 to 46), we move away from the '#process' model (which I support - followup patch, obviously), then this extra information will be skimmed down a lot anyway.

Posting this here for peer review.
I tested text, number and taxo autocomplete widgets, still need to check optionwidgets (list) and file/image widgets.

Fixed filefield failures, and tested optionwidgets, file and image widgets.

Tagging for later review.

Nice start! How about this instead?

1) We prepare an $element with basic properties upfront.

2) We pass that $element to hook_field_widget(), which is free to just re-use/extend $element and return it.

3) We drop #object_type and #bundle in favor of #field and #field_instance, which additionally allows further clean-ups in related field widget hook implementations.

The difference is that your "$element" in patch #9 is my "$base" in patch #4/6 with additional properties, right ?
'#field' => $field,
'#field_instance' => $instance,
'#field_name' => $field['field_name'],
'#delta' => $delta,
'#columns' => array_keys($field['columns']),

Leaving those out was the reason i wrote the "I tried to minimize the amount of extra information added (...)" paragraph in #4.
hook_field_widget() already receives $field, $instance and $delta as parameters, it's free to extract those info and embed them in its own element itself if it needs them downstream. This kind additional information will be mainly needed by '#process'-style widgets, which I think we should walk away from in followup patches (as your #414424: Introduce Form API #type 'text_format' already does).

#field and #instance, notably, will make the size of the FAPI array explode...

#columns, notably, is used for widgets that want to apply to several field types having different column names - that's the case of 'options' widgets (used for 'list' and 'taxo term' fields), but that's really an edge case, most other widgets (core, contrib or custom) don't actually need that level of engineering.

subscribing

Thinking about this a little more.

We currently already include $field and $instance arrays for reference at the top-level of the form ($form['#fields'][$field_name]['field'] and ['instance']).
They must be included *somewhere* in the form because widget processing code should rely on the actual field / instance info provided to field_default_form() rather than the stored field_info_[field|instance]() definitions. This lets us display widgets for 'variants' of a field (e.g: field_ui's 'default value' widget should be 'single value, not-required' even if the field is 'multiple, required') or use widgets in Views exposed filters.

$field and $instance being arrays and not objects (that's the step we didn't take in D7), duplicating them in several places in the form has a memory cost, and also makes painful print_r's when inspecting forms (OK, 'use devel dsm() and krumo'...). Most FAPI callbacks now have access to the $form array, so I think $form['#fields'] is OK for the needs of widgets. I'm even wondering if they would be better off in $form_state - but that would be a different issue.

At any rate, encouraging every widget to embed #field and #instance themselves as patch #9 does looks wrong because it causes one extra copy per 'multiple value', and most non-#processed widgets (which we want to encourage as the common pattern) won't actually need that information past their hook_field_widget().

So I'm shooting down patch #9 :-p, and stick with #4-#6 - rerolled and re-attached here, because it seems stuck in retesting.

I'd like to talk with you about that, but you're not in IRC... so I'd be ok with only relying on top-level field information in a form and invoking field_info functions where required.

However, that $base implementation is ugly, and we need to go with the $element approach of my patch. Also, as mentioned in a @todo of my patch, we need to at least additionally prepare #delta, because the widget doesn't have the $get_delta information, hence is unable to rebuild that info.

So I'll merge the two patches now, skipping the #field and #field_instance changes.

Though what I'd like to discuss is whether we shouldn't leave those three other properties (#object_type, etc) in for all widgets. Those don't hurt at all, and I'd prefer to have those values consistently available for form_alter and #process and #after_build implementations. (which is why I wanted to go with the #field + #field_instance properties, which would make that data + more equally available, but I can see your point of repeated data bloat, so I'd be fine with just keeping the minimal info required to fetch the data on demand)

So I hope we can agree on this compromise.

Yeah, IRC is not easy for me right now, sorry about that. I'll try to pop in some time tomorrow.

- I'm OK about #field_name.
- #object_type, #bundle IMO only make sense as helpers to call field_info_[field|instance]() within the widget FAPI callbacks, and as explained in #14 we *don't* want widgets to do that - those that currently do are buggy and should be fixed in a separate task. They should use $form['#fields'][$field_name] instead (which, the more I think about it, is exactly the kind of stuff that $form_state is about)
- #columns is overkill for widgets that don't need to abstract on field-type columns (all current core widgets except options.module widgets)
- #delta I simply don't see the need. Not used in current core widgets, AFAICT.

+ I don't think I actually see the difference between the 'ugly $base implementation' in #14 and the $element in #16 ;-)

Well, #field and #instance being ruled out already, the rest of the discussion is nitpicking and shouldn't hold the patch, esp. since it's a blocker for #414424: Introduce Form API #type 'text_format'. Thus, marking RTBC.
The separate task of moving number, taxonomy and option widgets away from #process should let us see more clearly what's actually needed in the 'base element'.

OK, that is much better indeed, and it borderline critical. Plus, given that it removes more code than it adds, I committed this to CVS HEAD.

+++ modules/field/field.api.php	31 Oct 2009 19:47:21 -0000
@@ -632,11 +632,15 @@ function hook_field_widget_info_alter(&$
+ * @param $element
+ *   A form element array containing basic properties for the widget: #title,
+ *   #description, #required, #field, #field_instance, #field_name, #delta,
+ *   #columns.
...
+function hook_field_widget(&$form, &$form_state, $field, $instance, $langcode, $items, $delta, $element) {
+  $element += array(
+++ modules/field/field.form.inc	31 Oct 2009 20:09:25 -0000
@@ -57,18 +57,18 @@ function field_default_form($obj_type, $
+      $element = array(
+        '#object_type' => $instance['object_type'],
+        '#bundle' => $instance['bundle'],
+        '#field_name' => $field['field_name'],
+        '#columns' => array_keys($field['columns']),
+        '#title' => check_plain(t($instance['label'])),
+        '#description' => field_filter_xss($instance['description']),
+        // Only the first widget should be required.
+        '#required' => $delta == 0 && $instance['required'],
+        '#delta' => $delta,
+      );

Sorry, I forgot to update the PHPDoc.

This review is powered by Dreditor.

Let's get this out of the mission critical list.

Committed to HEAD.