Abstract Field API $form_state usage into an API function

Great!
Subscribing.

@@ -335,7 +335,7 @@ function image_field_widget_process($element, &$form_state, $form) {
   $item = $element['#value'];
   $item['fid'] = $element['fid']['#value'];
 
-  $instance = $form_state['field'][$element['#field_name']][$element['#language']]['instance'];
+  $instance = field_info_form_instance($element, $form_state);

Does field_info_form_instance() actually exists?
I didn't found it in this patch.

Subscribe. Having written the very hacky Flexifield module for D6, I'm hugely in favor of making a non-hacky Combofield module possible in D7.

But this clearly won't get in before beta2. Will need to review more thoroughly, and think about if there's a way to not break BC unnecessarily.

Will need to review more thoroughly, and think about if there's a way to not break BC unnecessarily.

We've discussed this in #942310: Field form cannot be attached more than once.
The problem is that current structure of $form_state['field'] doesn't allows to handle multiple instances of the same field in one form. And there is no way to fix it without API changes because this structure is hardcoded in widget callbacks. E.g.:

@@ -355,8 +355,9 @@ function number_field_widget_form(&$form, &$form_state, $field, $instance, $lang
  * FAPI validation of an individual number element.
  */
 function number_field_widget_validate($element, &$form_state) {
-  $field = $form_state['field'][$element['#field_name']][$element['#language']]['field'];
-  $instance = $form_state['field'][$element['#field_name']][$element['#language']]['instance'];
+  $info = field_form_info($element, $form_state);
+  $field = $info['field'];
+  $instance = $info['instance'];

Proposed solution doesn't changes structure of $form_state['field']. It just changes the way for widgets to retrieve needed information. And also makes it possible to change that structure later without breaking existing widgets.


@all,
if we want to truly abstract Field API from $form_state then we possibly need both 'getter' and 'setter' functions dealing with $form_state['field'] information. E.g.

function field_form_get_info($element, $form_state) { ... }
function field_form_set_info($element, &$form_state) { ... }

As yched explained, we need to have this data in the form as $field, $instance can be different from the stored field API for a widget shown. If it might be different, per-shown-widget, I'd say the data belongs to the form element.

However, as yched also has noted in the original issue, it was like that previously and it just bloated $form. So what about just using the field/instance names there to retrieve the stored definitions with a little helper + add in a possibility to override this "default", e.g. if #instance set too, use that. Then for the common case we don't have the form bloated.

Or, instead of looking for #instance, look for #instance_overrides and apply them in the helper function.

Anyway, what is important is to make this change asap regardless which logic we want to follow later.

field_form_info() looks good to me, I wonder though whether we want to make use of list() instead? Or do we have the currently returned $info structure already anywhere?

Ok. I finally took some more time with this patch. So it actually looks like it sort of maintains BC. Widgets should call the new field_form_info() function. But any contrib widget types that currently still access $form_state['field'][$element['#field_name']][$element['#language']] directly will continue to work. This addresses my concern from #4, so I'm okay with this being RTBC'd if the rest of you are good with it.

Regardless of whether this is committed now on a fast track (to give widget modules a chance to start using the "right" way prior to D7RC1), or done as part of the larger work in #942310: Field form cannot be attached more than once, I think it's very important that the larger issue maintain this BC. Assume there will be some contrib code out there that doesn't use the new function. It should still work with all core forms. Yeah, it might break when used in conjunction with a module that implements multiple field instances with the same name and different settings on the same form, but okay, that's when it can be pressured to be fixed.

ad #11:
Dries and webchick in particular argued about returning a list of different things (return value + status) is bad, not using list() in general. As it's PHP's way of returning multiple things I think we should use that instead of inventing yet another array structure.

Ready to fly then.

Could we rename the helper function to field_get_form_info() or similar? field_form_info() sounds like a info hook name, and actually, http://drupal.org/project/form module is using hook_form_info() to turn forms into first-class citizens. Given module_implements_alter(), I could surely work around that, but would of course prefer to not have to.

Let's put this back to review so others can comment on sun's comment in #16. Like sun, I struggled with the name field_form_info() a bit as well.

It also makes me wonder why we have two functions in case A (i.e. field_info_field() / field_info_instance()) but only one function in case B (i.e. field_form_info()). It seems like this could potentially be made more consistent and more self-explanatory.

Having said that, I'd be OK with this patch. The only thing that comes to mind is to explain _why_ the $field and $instance arrays might differ from the official definitions returned by field_info_field() / field_info_instance(). Most people might not understand that.

It also made me wonder whether we need to update the documentation of field_info_field() / field_info_instance() to clarify when they should be used.

I think we have a bit more brainstorming to do. :)

Just fixed a couple of typos in comment to hook_field_widget_form().

This is a pretty straightforward code refactor. Nice docs as well.

This looks great now. Thanks yched. Committed to CVS HEAD.

This is great! Thank you, yched and an.droid, for the work on this issue and the other one. It'll be great to see combofield a reality in D7.

When announcing this to the dev list, as per #12, I really urge couching it as "it would be best for your widget modules to change from using $form_state['field'] to using the new functions". And perhaps explaining how this will ensure that the widget modules work correctly when combofield and other advanced forms become available during the D7 contrib cycle. But not as "you must do this". A lot of contrib developers have already been getting very frustrated at breaks between alpha and beta, and breaks between beta1 and beta2. We don't want people regretting that they have taken the #d7cx pledge and trying to make good on it. Just my suggestion. Feel free to take it or leave it.

#28: field_form_state_info-doc.patch queued for re-testing.

Committed to HEAD. Thanks!