Field form cannot be attached more than once

Scary but interesting / interesting but scary. This (or something like this) is probably a requirement for anything close to 'fieldable fields' / http://drupal.org/project/combofield.

Subscribe.
Note : you might want to ping fago, effulgentsia, sun - people involved in #367006: [meta] Field attach API integration for entity forms is ungrokable

The proposed solution is an API change, however.
1st break that comes to me is that widgets are currently supposed to refer to $form_state['field'][$field_name] to get the $field and $instance definitions they operate on. If this gets moved to $form_state['field'][$namespace][$field_name], then existing widgets break.

We're past beta stage now, API changes must be absolutely critical so that they are accepted. At this point, breaking widgets is hardly an option.

However, if I get this correctly, the issue is name clashes, i.e an instance of a field inside a 'combofield' would clash with an instance of the same field at the entity top level. I think that's a reasonable limitation and we can live with that. Field API already states that a given field can only appear once on an entity. A combo field is internally a separate entity, but the UX metaphor is to hide that separation and pretend the fields in the combo field are attached to the top level entity. So the limitation would make sense.

About CVS application : meanwhile, you can put your code on github, this would let other people look at the code, test and help (I can't promise I'll have the time to participate actively, but I'm not the only one interested in combofields...)

Ah, you mean for a multivalued combofield (like the one you show on your screenshot), the form has to contain one subfield widget per delta of the combofield.

Right, that's a problem. $form_state['field'][$langcode]['field'] and $form_state['field'][$langcode]['instance'] would be the same for all the copies (and it would be a memory hog to store one copy of those per 'namespace', BTW), but $form_state['field'][$langcode]['errors'] and $form_state['field_item_count'] would not.

We could structure $form_state['field'][$langcode] so that
- 'field' and 'instance' are not namespaced, and stay where they currently are,
- 'errors' is namespaced
- $form_state['field_item_count'] is moved inside this namespaced part of $form_state['field'][$langcode] too (where it sits currently, it seems it's going to break for forms with two languages side by side, which IIRC was the primary reason to have the $langcode key in $form_state['field'][$langcode]),

Then the widget API break I mentioned in #5 is gone, and the changes could be more acceptable. 'error' and 'field_item_count' have moved, but they're internal use only and not supposed to be accessed directly.

The screenshot looks yummy, BTW :-)

+++ includes/form.inc	15 Oct 2010 12:48:53 -0000
@@ -1698,6 +1698,13 @@ function form_builder($form_id, &$elemen
+  // Allow #limit_validation_errors of triggering element of a form
+  // to be altered in #after_build callbacks. This is needed in cases when
+  // actual position of submit button is unknown.
+  if (isset($form_state['triggering_element']) && isset($element['#id']) && ($form_state['triggering_element']['#id'] == $element['#id']) && isset($element['#limit_validation_errors'])) {
+    $form_state['triggering_element']['#limit_validation_errors'] = $element['#limit_validation_errors'];
+  }

This change should be moved into a separate issue, as it touches Form API internals, not Field API.

+++ modules/field/field.attach.inc	15 Oct 2010 12:48:57 -0000
@@ -531,14 +531,23 @@ function _field_invoke_get_instances($en
-function field_attach_form($entity_type, $entity, &$form, &$form_state, $langcode = NULL) {
+function field_attach_form($entity_type, $entity, &$form, &$form_state, $langcode = NULL, $namespace = 'default') {

The default value should be NULL. Meaning: Fields end up in the identical place they end up currently. Unless $namespace is set, in which case its value is used as additional key.

Minimizing the API change.

It would likely make sense to rename $namespace to $parents, and use an array instead of a string. Although we'd technically have to differ between $array_parent and $parents, both are the same in the current Field Attach API.

Something along the lines of the following should work then:

drupal_array_set_nested_value($form, array_merge($parents, array($field_name, $langcode)), $something);
drupal_array_get_nested_value($form_state['values'], array_merge($parents, array($field_name, $langcode)));

+++ modules/field/field.attach.inc	15 Oct 2010 12:48:57 -0000
@@ -531,14 +531,23 @@ function _field_invoke_get_instances($en
+  $form['#namespace'] = $namespace;

I'm not sure I understand this. #namespace seems to be set on the top-level form. So by invoking field_attach_form() multiple times, #namespace gets overwritten. In turn, the form validation and rest of form processing no longer knows the namespace.

+++ modules/field/field.default.inc	15 Oct 2010 12:48:57 -0000
@@ -12,12 +12,26 @@
+  // Check if field language was changed by locale.module or anything else
+  $actual_language = isset($form_state['field'][$namespace][$field_name]['original_language']) ?
+                     $form_state['field'][$namespace][$field_name]['original_language'] :
+                     $langcode;

This looks unrelated for this issue.

+++ modules/field/field.form.inc	15 Oct 2010 12:48:57 -0000
@@ -71,7 +72,7 @@ function field_default_form($entity_type
-    $form_state['field'][$field_name][$langcode] = array(
+    $form_state['field'][$namespace][$field_name][$langcode] = array(

If I understood yched correctly, then we should not namespace fields and field instances in $form_state, as the data is always the same. That makes sense to me.

Powered by Dreditor.

re sun:
well, $namespace and $element['#parents'] do have something in common (each array level where you'd find fields needs to use a separate namespace). Pushing that to "the $element['#parents'] array is the namespace" is interesting.

- in practice that means structuring $form_state['field'][$langcode] as a mirror of the $form structure ?
Well, I guess it's doable - whenever you have an $element, $element['#parents'] and drupal_array_get_nested_value() let you navigate to the right place in $form_state['field'][$langcode]. - works at a time where $element['#parents'] has already been computed, though.

- a unique string is still needed for

-  $wrapper_id = drupal_html_id($field_name . '-add-more-wrapper');
+  $wrapper_id = drupal_html_id($namespace . '-' . $field_name . '-add-more-wrapper');
...
         $field_elements['add_more'] = array(
           '#type' => 'submit',
-          '#name' => $field_name . '_add_more',
+          '#name' => $namespace . '_' . $field_name . '_add_more',

concatenating the #parents array into a string results in lengthy strings, but I guess that's already what we do for form element ids, and this hasn't killed anyone so far...

Note that field_attach_form() is usually invoked from the form constructor itself, so #parents is not yet available. Form API adds #parents during building of the form only.

However, changing $namespace = '' into array $parents = array(), we can manually build and pre-set #parents and make the rest of field widget processing account for it.

Oh wait. We can simply set #parents on the parent $element of a field, plus #tree => TRUE. http://api.drupal.org/api/function/form_builder/7 inherits any previously set #parents of a parent element to all children.

Thanks for this, an.droid.

No time to delve into this just right now, but adding a special-case $parents param to the generic purpose _field_invoke() / _field_invoke_default() is *very* unfortunate. I did not really look into the reasons for this, but it would really be nice if we could avoid that.

+++ modules/field/field.attach.inc	17 Oct 2010 08:20:42 -0000
@@ -531,15 +531,19 @@ function _field_invoke_get_instances($en
-function field_attach_form($entity_type, $entity, &$form, &$form_state, $langcode = NULL) {
+function field_attach_form($entity_type, $entity, &$form, &$form_state, $langcode = NULL, $parents = array()) {
   // If no language is provided use the default site language.
   $options = array('language' => field_valid_language($langcode));
-  $form += (array) _field_invoke_default('form', $entity_type, $entity, $form, $form_state, $options);
+  $form += (array) _field_invoke_default('form', $entity_type, $entity, $form, $form_state, $options, $parents);

So the actual problem we're running into is exactly this line: The additional Field Attach API form elements are tacked onto the top-level $form element.

Because of that, we cannot set $form['#parents'] (which would be automatically inherited to all children, as long as they do not hardcode #parents themselves).

At the very least, it looks like we could pass $options['parents'] to _field_invoke*(), eliminating the need for a new function argument.

Powered by Dreditor.

Adding new arguments to those functions massively decreases the chances of this patch landing for D7.

#21 does not work. Multiple invocations of field_attach_form() will overwrite $form['#field_parents'] = $parents;

All of that being said -- while $field always stays the same for identical field that may appear multiple times, isn't $instance always different if a field appears multiple times?

Anyway, probably rather $options. D7 technically does not allow for any further API changes, so avoid them at all costs.

I'd like to start to see what actually breaks with this patch. Let's implement simple variant of "duplicate fields" into entity_test.module - pseudo code:

function entity_test_duplicate_fields_form($form, &$form_state) {
  // ...
  field_attach_form($entity_type, $entity, &$form, &$form_state);
  field_attach_form($entity_type, $entity, &$form, &$form_state, $langcode = NULL, $options = array('parents' => array('foo')));

  return $form;
}

Subscribing.

I am working on combofield myself and have been working on a D7 version of the subform_element module (Subform), but this is a interesting approach too.

This is getting better IMO. Maybe we can do cleaner.

Solution in the current patch :
- pass $parents to _field_invoke() inside the existing $options arg
- have _field_invoke() move it from $options to $instance so that it can be passed to field_default_X() / hook_field_X()

That's a clever hack to mitigate with the constraints of the func signatures we have now, but :
- $options is supposed to configure how _field_invoke() operates its job of "caller of field ops" (all fields / only a given field, which languages, call default op / call field-type op...). It's not intended to receive additional args for the ops themselves.
- $parents does not really belong in the $instance either.

Proposal : put the $parents param in a $form_state['field']['current_parents'] entry, as a control property describing how the form ops should behave - makes sense in $form_state.

Instead of :

field_attach_form($entity_type, $entity, &$form, &$form_state, $langcode, $parents);

callers do :

$form_state['field']['current_parents'] = $parents;
field_attach_form($entity_type, $entity, &$form, &$form_state, $langcode)

The internal functions read $form_state['field']['current_parents'] to know at which level they should operate.
Drawback : subforms imply recursion. This solution relies on the combofield widget (= a nested level of "parents"), being processed at some point in the middle of the main form, to set back $form_state['field']['current_parents'] to the previous value when its done, so that the rest of the top-level widgets are processed properly.

Could be made a little friendlier by formulating $form_state['field']['current_parents'] as an array stack. $form_state['field']['parents_stack'] : combofield widget pushes its parents level on top of the stack before doing its job, and pops it out when it's done.

sun, an.droid, what do you think ?

Sorry -- reviewed independently of yched's follow-up. Just read it now. Of course, he overrules any of my suggestions about Field API internals. :)

Now that I read it, I'd recommend to move the $form_state['field']['parents'] assignment into field_attach_form(); i.e., by adding the $options argument. Even though the 'parents' key might not make sense to pass on to _field_invoke(), the $options argument itself might be useful for other edge-cases, too. So IMHO, even though it might not be 100% accurate, it sounds like the best compromise we can do for D7.

--

+++ modules/field/field.attach.inc	18 Oct 2010 12:21:04 -0000
@@ -179,6 +179,11 @@ function _field_invoke($op, $entity_type
+    $namespace = count($options['parents']) ? implode('_', $options['parents']) : 'default';

We should keep the array; no implode().

+++ modules/field/field.attach.inc	18 Oct 2010 12:21:04 -0000
@@ -188,6 +193,9 @@ function _field_invoke($op, $entity_type
+    if (isset($options['parents'])) {
+      $instance['parents'][$namespace] = $options['parents'];

Same here, just take over $options['parents'] into $instance['parents']

+++ modules/field/field.attach.inc	18 Oct 2010 12:21:04 -0000
@@ -531,14 +539,21 @@ function _field_invoke_get_instances($en
+ * @param $parents
...
+function field_attach_form($entity_type, $entity, &$form, &$form_state, $langcode = NULL, $parents = array()) {
...
+  $options = array(
+    'language' => field_valid_language($langcode),
+    'parents' => $parents,
+  );

$parents should be renamed to $options, and merged into the hardcoded, separate $langcode option, so $options will be future-proof. For D8, we will likely remove the $langcode argument and force callers to supply $options instead.

+++ modules/field/field.attach.inc	18 Oct 2010 12:21:04 -0000
@@ -786,9 +801,9 @@ function field_attach_validate($entity_t
-function field_attach_form_validate($entity_type, $entity, $form, &$form_state) {
+function field_attach_form_validate($entity_type, $entity, $form, &$form_state, $parents = array()) {

@@ -823,11 +838,11 @@ function field_attach_form_validate($ent
-function field_attach_submit($entity_type, $entity, $form, &$form_state) {
+function field_attach_submit($entity_type, $entity, $form, &$form_state, $parents = array()) {

When we are in form processing, we can leverage the usual $form_state facility to figure out $options/$parents. The signature of these two functions should not change.

Powered by Dreditor.

very interesting, thanks for pinging me about that. I planned to take a stab on that too.

@fieldable fields:
I've been working on quite the same -> http://drupal.org/project/embeddable
The code there is WIP, but works. Currently the code focusses on the data structure, not on the end-user UI. But as it's a field, the widget and formatter is pluggable. It'd be nice to have a widget embedding the form, though I've no concrete plans to work on that soon. But let's discuss that in another issue.

I'm in a meeting right now, will try to review the code asap.

Let's discuss fieldable fields there: #939836: combofield / fieldentity / field-collection / embeddables

I've not look into the details yet, but in profile2 I've just passed a part of $form to the field API attachers like this:

  foreach ($form_state['profiles'] as $type => $profile) {
    $form['profile_' . $profile->type] = array();
    field_attach_form('profile', $profile, $form['profile_' . $profile->type], $form_state);
  }

Haven't looked closer at it yet, but it seems to basically work. However I've not set #tree to TRUE nor used a field multiple times.
Anyway, from an API user perspective, wouldn't it be possible to have it just work that way? Field API should be able to read $form['#parents'] from $form and thus navigate to the right form values for extraction.

What is a bit harder to do but would be nice, is getting multiple - but different instances - of the same field in the same form working. +1 for making that possible.

Multiple instances of the same field instance would be the next thing 2 have. However I'm unsure whether we really want to support such complex cases? Perhaps it would be more useful to get that working by embedding a complete entity form? -> http://drupal.org/project/subform

@fago :

Looking at it, passing a sub-part of $form to field_attach_form_*() is also what Barry did in its http://www.drupal.org/project/combofield attempt.

I can't say I'm really comfortable with this. The various field_attach_form_*() trigger a lot of callbacks and hooks : field_default_form(), hook_field_attach_form(), field_default_validate(), hook_field_attach_validate(), field_default_submit(), hook_field_attach_submit(), hook_field_widget_form(), hook_field_widget(), hook_field_widget_error() ...
For all of those, the contract is "$form is a $form". It's hard to tell what can break if we suddenly say it can by a form sub part.

"multiple - but different instances - of the same field in the same form / Multiple instances of the same field instance" : the challenge is to support that in a way that doesn't break existing widgets - see #5 above.

For all of those, the contract is "$form is a $form". It's hard to tell what can break if we suddenly say it can by a form sub part.

I don't think this should lead to problems. Some elements inside the form don't look any different to the whole $form. As long $form always points to the same part of the form for all field API attachers, everything should be fine. But of course, if we allow that we should update those contracts and document it.

+          '#field_parents' => $parents,
+          '#namespace' => $namespace,

This lets us save $parents another time, we already have that in $form['#parents']. Yes, FAPI doesn't generate them on build, but we don't have to use #parents on build as we can pass the target form element as array too (regardless whether it is $form or an extra argument).

Attached patch is what I had in mind. It should be everything necessary to make embedding field-forms in an arbitrary form position possible having #tree set to TRUE working. It does not fix multiple instance of a field though.

What me somehow confuses is, why do we have $field_info['array_parents']? Isn't the assumption that the form element of a field is always at $form[$field_name] ?

"multiple - but different instances - of the same field in the same form / Multiple instances of the same field instance" : the challenge is to support that in a way that doesn't break existing widgets - see #5 above.

Indeed. I thought some more about this. While it would be neat to support that use-cases in the field API directly, it would necessarily break the API now. But as multiple instances of a field always require multiple entities on the same form, I do think the underlying problem we really want to solve here is embedding a form cleanly into another one. If we can achieve that, multiple forms of the same type of entity in one form can be done too.

I had a short look at the code of what would be requires to support multiple instances. So from that I think we should be able to support that cleanly by scratching the following 'instance', 'array_parents', 'errors' keys:

+      $form_state['field'][$field_name][$langcode] = array(
+        'field' => $field,
+        'instance' => $instance,
+        // This entry will be populated at form build time.
+        'array_parents' => array(),
+        // This entry will be populated at form validation time.
+        'errors' => array(),
+      );

As noted above, 'array_parents' might not be necessary at all(?) and 'errors' could be easily removed by passing the information directly to field_default_form_errors(). Then $field wouldn't be needed their either, as it can be easily get using field_info_field() too. Also button click detection would have to improved to properly identify the right button, but that wouldn't be the problem.
The problem is, that this would pose quite some API changes for widgets, so while it would be nice it's probably not on the table for d7. The only way to properly work around it, is doing a subform.

Indeed, form-level handlers (#validate and #submit) would not work, but I doubt it would be make much sense to use hook_field_attach_form() instead of hook_form_alter() to add those in. Still, once we document this, this would not be supported any more, thus be a small API change.

>And again this doesn't solve problem with multiple instances of one field in the form.
Right. Passing in $parents neither. As written in #40 this impossible without seriously breaking the API for widgets. E.g. see how this would affect http://api.drupal.org/api/function/number_field_widget_validate/7.

Still, attached patch makes field API attachers more flexible to use, so entity forms can decide where they want to embed the fields.

@patch:
It looks like that $form_state information isn't there when translatable fields are involved. I wonder whether this might pose pitfalls for widgets when used with translatable fields?

Anyway, I changed it to just assume $form[$field_name]. However the field UI doesn't follow that, so I've added a fallback for that now.

>Last patch I submitted doesn't breaks existing widgets (testbot proved it), 'field' and 'instance' are stay where they were before.

Well, it does work with the code in core, which only has one instance per field on a form. It might work on a form with multiple entities somehow too, but in the end it will break. It breaks, as it uses those three properties relating to a single instance for multiple different instances at the same time.
E.g. http://api.drupal.org/api/function/number_field_widget_validate/7 will break if you have two different instances of the same field on the same form.

So, either we fix those properties and break the API, or we stay with the one-instance per form limitation.

I'm not sure I get how fago's patches in #39 / #43 get in the mix ?
The two hunks that replace manual array walk with the new drupal_array_get_nested_value() are cool, I've been willing to do that since the function landed. We'll want those changes no matter what happens in this thread, so it would be nice as a separate patch that would be quick to RTBC ?

yched, please have a look at #38 as it explains what my patch does.

But as said also in #38, I wonder what it is about with field-'array_parents'?

>So, either we fix those properties and break the API, or we stay with the one-instance per form limitation.
yched, do you think breaking the API for that is still an option? Widgets would have to look up the field instance from the properties set on the form element with field_info_instances() then.

>The two hunks that replace manual array walk with the new drupal_array_get_nested_value() are cool, I've been willing to do that since the function landed.
Yep, just stumbled over it and replaced it.

>While this patch is indirectly related to this issue it would be better to split it to separate issue, especially if this will speed-up process.

It also intends to solve the parenting issue by just passing a subform as $form, but misses docs for that. Anyway, I'd suggest splitting this into
* make it possible to nest field forms and
* make multiple instance of the same field in one form work

This issue currently mixes both.

#37 has a mix of $form_state['field']['current_parents'] and $instance['current_parents'], which makes it hard to track. I though we agreed on the former ? For the same reason, the hunk touching _field_invoke() should go away - not needed.

It's confusing that field_attach_form(), field_attach_submit(), field_attach_validate_form() don't work similarly when it comes to passing the 'parents' argument. In current #37 :
- f_a_form() takes $options['parents'] and puts it in $form_state['field']['current_parents'],
- f_a_submit() / f_a_validate_form() have the caller set $form_state['field']['current_parents'] himself.
It seems this was what sun recommended in #31 (not fully clear), but is sounds really odd. It limits the API change (API addition, really) to a single function, but at the cost of an API WTF. I'd personally suggest using $form_state['field']['current_parents'] consistently - at least for now. I might have other suggestions to deal with that later.

Other than that - I really needed to *see* some actual code use case to test, figure the code flow and API logic better - without the complexity of the current state of field_collection.
So I started a prototype module for a 'bare simple' combo widget (no separate 'X-entities', just uses nodes of a given, dedicated 'ghost' node type, = circa 150 lines of simple code). Work in progress. I should have a clearer vision on the patch when I'm done.

@fago #49:

yched, do you think breaking the API for that is still an option? Widgets would have to look up the field instance from the properties set on the form element with field_info_instances() then.

Nope, they shouldn't rely on what field_info_instances() returns, that's the whole point of storing the instance and field definitions in $form_state. Sometimes you want to display a widget for a 'variant' of an officially existing $field / $instance.
Example : the widget for 'default value', that appears on the 'instance settings' form, cannot be 'required',even though the $instance is required. It is also currently mono-value, even if the field is multiple. Other use cases might include widgets in Views exposed forms.

hook_field_widget_form() receive as params the $field and $instance they should deal with, but the #element_validate or #value_callback functions that they might define don't receive that. In CCK D6 an '$element['#field'] = $field' entry was automatically added (and thus available for helper FAPI callbacks dealing with $element), but that resulted in massive duplication (one $element per delta for multiple values) and print_r($form) hell. In D7 we stored that in $form_state. Having all Field process info inside a single $form_state['field'] location also makes sense.

*If* we do change the way helper FAPI callbacks can access the working $field and $instance for the widget, we should encapsulate it into an API function : list($field, $instance) = get_me_the_info($element, $form_state); or similar.
That way we'll keep a flexibility on the internal structure of $form_state['field'], and won't have to break APIs again at a later point.

@fago #51:

I'd suggest splitting this into
* make it possible to nest field forms and
* make multiple instance of the same field in one form work
This issue currently mixes both.

To my knowledge, it doesn't. It allows a form to include several widgets for the *same* instance of the *same* field in several places - i.e. (referring to this screenshot ), field SKU in row 1, and field SKU in row 2. Same field, same instance, but separate widgets - each with its own error reporting and 'add more' button (if field is multiple).
Supporting this is, by definition, required for a combo-widget, thus belongs to this patch, and should be possible without API changes.
Supporting different instances of the same field in one form (one instance on top-level, one instance in a sub-entity subform) would definitely be cool, but requires an API change, and is a different scope that should definitely be a followup.

I'd even possibly suggest introducing list($field, $instance) = get_me_the_info($element, $form_state); asap, and have existing widgets move to this now so that they don't break later (possibly in core point releases, which would be worse than anything).

What do you think about fago's suggestion to provide sub-form to the field_attach_* functions?

To be fair, I still don't really know. I have a hard time imagining that this would solve all challenges of nested fields. The issue is not only about assembling the right form values in the right place, it's also about
- handling validation and error reporting (running field-level validation in hook_field_validate() and dispatching errors to the right form element),
- handling 'add more' buttons and the item_count of each 'multiple field' element in the form.
That requires a overhaul of how we use $form_state. I can't see how patches #39 or #43 alone fit the bill.

Which reminds me that I didn't reply to fago in #38 :

What me somehow confuses is, why do we have $field_info['array_parents']? Isn't the assumption that the form element of a field is always at $form[$field_name] ?

This is originally to support modules altering the form structure (i.e fieldgroup moving field elements within the form), without breaking 'Add more' or error assignment. After form is built, we store the location of the field elements within the form. With that map, we can navigate $form to find each field and do stuff of its $element.
In CCK D6, the "add more" callbacks had to do some if (module_exists('fieldgroup')) { dance.
Should be less a problem in D7, since fieldgroup should operate on pre_render and leave the $form structure during processing.
But no form_alter() manipulation should break form error reporting, that would be a security hole.

Example : the widget for 'default value', that appears on the 'instance settings' form, cannot be 'required',even though the $instance is required. It is also currently mono-value, even if the field is multiple. Other use cases might include widgets in Views exposed forms.

I see. Thanks for explaining :)

*If* we do change the way helper FAPI callbacks can access the working $field and $instance for the widget, we should encapsulate it into an API function : list($field, $instance) = get_me_the_info($element, $form_state); or similar.
That way we'll keep a flexibility on the internal structure of $form_state['field'], and won't have to break APIs again at a later point.

Thought about something like that already too, this would certainly make sense.

To my knowledge, it doesn't. It allows a form to include several widgets for the *same* instance of the *same* field in several places - i.e. (referring to this screenshot ), field SKU in row 1, and field SKU in row 2. Same field, same instance, but separate widgets - each with its own error reporting and 'add more' button (if field is multiple).

Got it. Agreed, anything related to different instances of a field should be a follow-up.

@array_parents:
Thanks for explaining. However with #37 this code is rather difficult to follow, so I wonder whether it makes sense too have 'array_parents' in $form_state given there is no real use-case for it. So with #37 we use the original-parents to get the array-parents to finally get to the elements? *help*
I'd vote for just going with $form[$field_name] inside the passed form with the passed parents. This makes the code much simpler to follow and also that way any contrib can expect just that too. In d6, it always caused headache that modules had to support the fieldgroup location too. As it's not necessary, let's avoid it and keep things simple.

To be fair, I still don't really know. I have a hard time imagining that this would solve all challenges of nested fields. The issue is not only about assembling the right form values in the right place, it's also about
- handling validation and error reporting (running field-level validation in hook_field_validate() and dispatching errors to the right form element),
- handling 'add more' buttons and the item_count of each 'multiple field' element in the form.
That requires a overhaul of how we use $form_state. I can't see how patches #39 or #43 alone fit the bill.

Handling validation and error reporting should just work fine, as it's invoked via an field_attacher too, thus it gets the same part of $form as in the first place. Thus, the extracted values would be read for the element $form[$field_name] and the errors can be set there too.
For add-more buttons we just have ensure unique button names, as the patch in #37 already does. Then we have $form_state['triggering_element'], which tells us the button of which field has been pressed, e.g. just look at the button #parents.

So I think it should work like a charm, but if you both think it's cleaner the other way I'm fine with that too.

+  // We can't use drupal_array_set_nested_value() because we need to merge
+  // $form and results of _field_invoke_default().
+  $ref = &$form;
+  foreach ($parents as $parent) {
+    $ref = &$ref[$parent];
+  }
+  if (!$ref) {
+    $ref = array();
+  }
+  $ref = array_merge($ref, $sub_form);

You can still get the elements to merge with drupal_array_get_nest_value, merge it and then use drupal_array_set_nested_value() to set the new values.

Keeping track of this thread

Still wrapping my head around this - meanwhile, let's buy us time with #950138: Abstract Field API $form_state usage into an API function

I'm working on this based off #60. Making good progress :-). Stay tuned.

Subscribing. Is there a summary comment I can read to get up to speed quickly?

@effulgentsia : hmm, not really :-). I'll try to summarize when I post my work (ETA : 1-2 days)

Meanwhile, found that ;-) #953914: #limit_validation_errors fails is parents array contains numeric indexes

#950138: Abstract Field API $form_state usage into an API function was just committed.

So, here's a summary, and an updated patch - unfinished, but fully working as is :-).
It's big enough in itself for today, so I'll post "Still @todo / open questions" in a followup post.

This issue intends to alleviate the following limitations about Field API form integration :
- a given field can only appear once in a form; this forbids combofield (form needs to include the same instance of the same field several times on different sub-entities) or sub-entity forms (e.g profile2 fields on user registration form - form can include several instances of the same field)
- you cannot put widgets in a nested #parent in a form : limitating for sub-entity forms, or for "widgets in Views exposed filters". Field UI currently hacks its way around for the 'Default value' widget that appears on the 'Edit instance settings' form.

Technically, the reasons for those limitations are :
- fields are stuck at the form top-level
field_attach_form() places widgets in $form[$field_name],
field_attach_form_validate() and field_attach_submit() read the corresponding values from $form_state['values'][$field_name]
- some working data for form processing is stored in $form_state['field'][$field_name]

The approach defined in this thread is to :
1) allow field_attach_form(), field_attach_form_validate(), field_attach_submit(), to operate at any given #parents level (top-level by default, doesn't change anything for current entity forms).
2) internally, organize the working data in $form_state['field'] into the corresponding, separate #parents levels, that won't clash with each other.
The limitation then becomes : the same field cannot appear twice at the same #parents level, which is OK since it maps to "you cannot have two instances of the same field on an object".

More precisely, what the patch does :

- field_attach_form(), field_attach_form_validate(), field_attach_submit() take an additional, optional $parents array parameter, defaulting to 'top-level' (the exact formulation for this is not fully settled yet, more on this in a followup "Open questions" post...)
field_attach_form() places the elements at the corresponding place in the form.
field_attach_form_validate(), field_attach_submit() extract values from the corresponding place in $form_state['values'].

- Access to $form_state['field'] data is fully encapsulated into API functions - "read / write the data for this $field_name in this $parents space" :
Functions : field_form_get_state(), field_form_set_state().

- #950138: Abstract Field API $form_state usage into an API function deprecates / discourages direct access to $form_state['field'], but the new structure for $form_state['field'] preserves backwards compatibility for widgets that still read $form_state['field'] directly (they will still work when placed at top level, but will break in combofields / nested forms). See , _field_form_state_path().

- In cases like combofield, form building and processing can be recursive, with various #parents levels along the way :

* field_attach_form(main entity, top-level)
** image_field_widget_form() for an image field on the main entity
** combo_field_widget_form() for a combofield on the main entity
*** field_attach_form(sub entity, nested parents), 
**** text_field_widget_form() for a text field in the combofield
**** combo_field_widget_form() for a combofield in the combofield (ugh, but why not...)
***** field_attach_form(sub sub entity, nested nested parents)
****** other widgets...
** number_field_widget_form() for a number field on the main entity

To this end, $form_state holds a stack of 'parents levels'. When processing starts at a given parents level, we stack it, and the rest of the code can simply refer to 'the current perents level' (the top of the stack). When processing at this level is done, we pop it out, and processing continues at the previous level :

function field_attach_form(..., $parents) {
  field_form_set_parents($parents);
  // Do my job...
  field_form_unset_parents($parents);
}

Functions: field_form_set_parents(), field_form_unset_parents(), field_form_get_parents().

- Moves the 'item count' (used by 'add more' buttons) from a separate $form_state['field_item_count'][$field_name] property, and puts it with the rest of the working data inside $form_state['field']. Patch also largely simplifies the processing logic around the 'items_count' value.

The patch can be tested with the very simple 'combo field + widget' prototype I uploaded in http://github.com/yched/combo.
The module simply uses a dedicated node type (instead of a dedicated entity type) to store the 'combo values', and creates an initial set of fields on install. The code to handle the field + widget is 120 lines with comments.
Simple instructions at the top of combo.module.

I have to say, this works beautifully :-)

Oops, stupid typo in a late variable rename.
I also fixed a bug the install setup in http://github.com/yched/combo.

We're crossposting like mad. Patches #75 and #76 are the same, all is well.

At first look this approach also considers that $form argument passed to field_attach_form() is not always contains full form array.

Yes, for now it does, but that's the last pain point that's not completely settled yet :-) I'll get back at this in a later post asap.

2) internally, organize the working data in $form_state['field'] into the corresponding, separate #parents levels, that won't clash with each other.

Great way to preserve backward compatibility! :)

ad

field_form_set_parents($parents);

Is that really necessary? Why not just read $element['#field_parents']? Or better, don't duplicate parents either but go with the existing #array_parents property. As that is not available during form construction, we'd have to pass it through to the form constructor or postpone the $form_state init to a #process callback.

Having #array_parents, #parents, #field_parents is rather confusing, having two of them is already enough ;)

- patch unwillingly changed the #name property of the 'add more' button, breaking tests that rely on the name.
- updated Field UI the code to use to use field_form_[set|get]_parents() when embedding the 'default value' widget in the 'instance settings' form. Still places the widget at #parents = array(), since nesting them would break on widgets that still read from $form_state['field'] directly, which we're trying to keep working in basic cases. Turns out this form is used in quite a few tests, so this caused lots of failures.

Let's see how this one goes.

just a re-upload last patch with right name

Better fixes for Field UI's "default value" widget.
Let's try this one, file / image tests miserably fail on my box.

So, what is still undecided is what we do with the current $form argument for field_attach_[form|form_validate|submit]().
Either :

1. a full $form array (like in current HEAD).
   An additional $parents array param specifies where to nest fields
2. the subpart of the form where fields should be attached - meaning, an $element (that's what patch #83 does, although it keeps the arg named $form...).
   An additional $parents array param specifies where this element resides in the global form (so that we can cluster data in $form_state['field']),
   or possibly we just read this from $element['#parents'] (which needs to be explicitly set by the caller of field_attach_form(), because form processing hasn't populated it yet)

Down the callstack, and apart from internal Field API functions which can handle both approaches with minor changes, this $form argument is passed to :

• Widget 'hooks' (rather callbacks, as we all know...) :
  • hook_field_widget_form(&$form, &$form_state, $field, $instance, $langcode, $items, $delta, $element):
    I can't think of an actual use case for accessing $form in here, be it the full form or the sub-level where widgets are being attached. Current implementations I know of don't make use of the argument, it's there mainly because it was there in D6, and because we usually provide $form when we provide $form_state. Even a complex widget like image / file upload widget doesn't rely on it. Widgets are self contained.
  • hook_field_widget_error($element, $error, $form, &$form_state):
    Same as above. The goal is to call form_error() on the right sub-$element in the widget depending on the reported error. No use of $form in there.
• field_attach hooks :
  • hook_field_attach_form($entity_type, $entity, $form, $form_state, $langcode):
    2) means hook implementations cannot act on $form['#submit'], $form['#validate']. For those, you need to go through hook_form_alter() and work the top level form. Edge case IMO.
  • hook_field_attach_submit($entity_type, $entity, $form, $form_state):
    "Act on field form values that have just been assembled into the entity". This should mainly work off $form_state['values'] and $entity, and it should make no difference if the $form param becomes a sub-$form.

Option 2 (the whole Field form integration works on '$element's) seems a better API shape. Do we really want to go and rename all those $form params to $element now, though ? (+ some of those functions/hooks already have an $element param, which is the widget itself)
Option 1 sounds a bit awkward - "work on this $form, but only on this $parents sub part"

Thoughts ?

Option 2 seems the best option to me too. To access the complete form you can use $form_state['complete form'] anyways right?

@casey : during process, validate and submit, yes, but not during initial form assembly.
In option 2, hook_field_widget_form() and hook_field_attach_form() have no access to the global form whatsoever. This should be OK; as I wrote above,
- hook_field_widget_form() should not need it (self contained form element),
- and use cases for hook_field_attach_form() acting on $complete_form['#submit'] and $complete_form['#validate'] need to use form_alter(). Or hook_field_attach_form() can add a #process/#after_build callback to the subform, that will be able act on the whole form. Minor workaround for edge cases, but the flexibility gains option 2 brings us are worth it.

I'm moving forward on option 2).

I'm still not sure if we want to rename the $form argument in the functions and hooks below field_attach_form() and friends.

re #87:
1) no, I need those changes right now to check validation errors work correctly. When #953914: #limit_validation_errors fails is parents array contains numeric indexes gets in (ping @effulgentsia : RTBC ?), I'll remove the hunk from this patch.
2) I don't know exactly - the patch includes #745590: #managed_file element does not work when #extended not TRUE, or when ancestor element doesn't have #tree=TRUE for now, and an additional hunk that makes file widgets "almost" work in combo (upload is OK, but you don't get a new empty upload box). It's possible that file widgets will need to be fixed for 'combo' in a separate patch

Added exhaustive tests for fields within a nested form.
Still a couple bits to polish, will provide an updated patch in a day or two.

Will post an updated and polished patch tomorrow.

Meanwhile, bumping to 'critical' as per http://drupal.org/drupal-7.0-beta3.
We need this in, if only to settle the 'multigroup WTF' in D7 and let the (many) people that have been using the 'forever experimental' multigroup feature in D6 CCK 3.x branch know that there is a clear way forward in D7.

If the bot doesn't prove me wrong, this should be ready for review.

For those following the thread, main differences with the previous patch are:
- $parents is simply specified by setting $form['#parents'] on the incoming $form param - that's what #parents is here for :-).
- this removes the need for the recursion-friendly $form_state['field']['parents_stack'] and the associated 'internal API' funcs.
- docs + exhaustive tests

I chose not to rename the $form param in the end. Renaming to $element would cause massive confusion (already used for something else), and no other name makes better sense. The change is fully transparent for 95% cases anyway.

Updated summary in the next post.

Updated summary :

The patch alleviates the rigidness of Field API form integration (widgets in the top-level of a form, values in the top-level of $form_state['values'], only allowing one occurrence of a given field in a form).
Those limitations prevent a couple rich features (profile2 fields on user registration form...), and are the only blocker for a sane D7 solution to the long-standing 'combofield / multigroup' issue.

- lets fields be attached in nested form elements,
by letting field_attach_form(), field_attach_form_validate(), field_attach_submit() accept sub-elements as their $form parameter
(minor code adjustments, mostly already works in current HEAD)

- lets field values appear in custom #parents spaces in $form_state['values'],
according to the '#parents' property of the incoming $form parameter
(mostly affects field_default_extract_form_values())

- internally, organizes the processing data stored in $form_state['field'] into corresponding #parents spaces,
with API functions to access it (internal use)
(accounts for most of the code changes)

- as a side effect, streamlines the code behind 'add more' buttons.

Half of the patch size is docs and tests. Actual code changes are in fact fairly limited for the flexibility gain, which is a nice hint about the field/form API.

Sample code, taken from the 'field_test_entity_nested_form_submit' test form :

function foo_form($form, &$form_state, $entity_1, $entity_2) {
  // First entity.
  field_attach_form('test_entity', $entity_1, $form, $form_state);

  // Second entity.
  $form['entity_2'] = array(
    '#type' => 'fieldset',
    '#title' => t('Second entity'),
    '#parents' => array('entity_2'),
  );
  field_attach_form('test_entity', $entity_2, $form['entity_2'], $form_state);

  $form['save'] = array(
    '#type' => 'submit',
    '#value' => t('Save'),
  );
}

(see the hunks in modules/field/tests/field_test.entity.inc for full code)

API impact :

- The $form parameter received by hook_field_widget_form(), hook_field_widget_error(), hook_field_attach_form(), hook_field_attach_submit() is not necessarily a full form.
As explained in #84, this should only really affect hook_field_attach_form() implementations willing to do stuff with the global #submit or #validate - which I'm not sure have actual use cases. Such cases can set a #process callback and act on $complete_form in there, or go through a regular hook_form_alter().

- widgets doing really advanced stuff with $form_state['values'] or $form_state['input'] directly (read: upload widget for filefield) need to account for the fact that field values don't necessarily live at $form_state['values'][$field_name][$langcode]. Nothing breaks for 'regular' entity forms, though.

Forgot to git-commit some test fixes before rolling #91.

"The test did not complete due to a fatal error"
Those tests run successfully on my setup. Retry.

#93: field_form_parents-942310-93.patch queued for re-testing.

Beats me for tonight. Any way to have more info on a "test did not complete due to a fatal error" message that you can't reproduce locally ?

Gosh. Was just a stupid typo in the test module (& in front of a variable in a function *call*). For some reason my PHP (5.2) swallowed it, but the bot clients (probably 5.3) didn't compile any time the module was enabled.

Anyway. Green and ready for review. Summary is in #92.

and this has API changes, detailed in #92.

Great work!

>- lets field values appear in custom #parents spaces in $form_state['values'],
I'd have preferred not having to set that explicitly, however I see that this would require even more changes. In particular we would have to postpone $form_state information initialization, which would be an API change for which it is too late. So +1 for that.

>by letting field_attach_form(), field_attach_form_validate(), field_attach_submit() accept sub-elements as their $form parameter
(minor code adjustments, mostly already works in current HEAD)

Awesome, as said previously this makes much sense to me! Also it means that the profile2-form attacher functions don't have to change.

@patch:
I've gone over it and fixed some doc comments to match the guidelines (@see at the end of doc blocks, empty line before @return, no @return if nothing is returned).

and to ease reviewing, here is a inter-patch-diff.

The link to the interdiff patch in #101 is broken, but patch #100 is fine by me. Thks :-)

Looks like the pound causes troubles, so again without it.

Cool - RTBC anyone ?

Beside the tests, I also tried the patch with profile2 -> works fine! So as everyone seems to be happy with it, I mark it RTBC.

Summary is at #92.

I'd love to see a review by eff.

+++ modules/field/field.api.php
@@ -808,6 +809,7 @@ function hook_field_widget_info_alter(&$info) {
+ *   - #field_parents: the '#parents' space for the field in the form.

Should start with a capitalized letter (The).

But also, the "#parents space for the field in the form" sounds odd. It doesn't clarify whether #field_parents is identical to #parents (it sounds like it might be), and if that is the case, then the description should clarify why #field_parents exists as separate property in the first place, so developers know when they should use #parents and when to use #field_parents.

+++ modules/field/field.api.php
@@ -1098,7 +1106,39 @@ function hook_field_formatter_view($entity_type, $entity, $field, $instance, $la
 function hook_field_attach_form($entity_type, $entity, &$form, &$form_state, $langcode) {
...
+function hook_field_attach_submit($entity_type, $entity, $form, &$form_state) {
...
@@ -1132,17 +1172,6 @@ function hook_field_attach_validate($entity_type, $entity, &$errors) {

@@ -1132,17 +1172,6 @@ function hook_field_attach_validate($entity_type, $entity, &$errors) {
-function hook_field_attach_submit($entity_type, $entity, $form, &$form_state) {

_validate() should always come before _submit() in code. Not sure why the existing order of functions was changed.

+++ modules/field/field.attach.inc
@@ -442,9 +442,26 @@ function _field_invoke_get_instances($entity_type, $bundle, $options) {
+ *   '#parents' => the location of field values in $form_state['values'],
+ *   '#entity_type' => the name of the entity type,
+ *   '#bundle' =>the name of the bundle,

1) Descriptions should start with an capital letter.

2) Last description is missing a leading space.

+++ modules/field/field.default.inc
@@ -11,13 +11,36 @@
+  $path_exists = FALSE;
+  $values = drupal_array_get_nested_value($form_state['values'], $path, $path_exists);
+  if ($path_exists) {

Normally, we use $key_exists here, initialized to NULL. See http://api.drupal.org/api/drupal/includes--common.inc/function/drupal_ar...

+++ modules/field/field.form.inc
@@ -308,18 +309,20 @@ function theme_field_multiple_value_form($variables) {
+ * After_build callback: stores information about the built form structure.

Should be

#after_build callback for field_default_form() to store information about the form structure.

+++ modules/field/field.form.inc
@@ -387,24 +397,22 @@ function field_add_more_submit($form, &$form_state) {
+  // Go one level up in the form, to the widgets container.
+  $element = drupal_array_get_nested_value($form, array_slice($button['#array_parents'], 0, -1));

(and elsewhere) This construct and assumption looks fragile to me. Would it be possible to also add the #field_parents property directly on add more buttons; i.e., pointing to the field?

That said, now I see that it's #array_parents here... so most probably, the suggestion is not possible. The mixed usage of #parents, #array_parents, and #field_parents worries me a bit.

+++ modules/field/field.form.inc
@@ -413,6 +421,81 @@ function field_add_more_js($form, $form_state) {
+function field_form_set_state($parents, $field_name, $langcode, &$form_state, $field_state) {
+  $path = _field_form_state_path($parents, $field_name, $langcode);
+  drupal_array_set_nested_value($form_state, $path, $field_state);
...
+function _field_form_state_path($parents, $field_name, $langcode) {
...
+  return $path;

_field_form_state_parents() would be a better name.

Also, all $path variables should be renamed to $field_parents.

Thus, $parents == parents of the form, and what you get is $field_parents == parents for a specific field in the form.

Powered by Dreditor.

Subscribing

Subscribe

subscribing. All of you are doing some impressive work here!

subscribing

Just to help this move on, I addressed sun's comments except:

But also, the "#parents space for the field in the form" sounds odd. It doesn't clarify whether #field_parents is identical to #parents (it sounds like it might be), and if that is the case, then the description should clarify why #field_parents exists as separate property in the first place, so developers know when they should use #parents and when to use #field_parents.

and

1) Descriptions should start with an capital letter.

Thanks, much better.

+++ modules/field/field.api.php
@@ -1098,47 +1106,68 @@ function hook_field_formatter_view($entity_type, $entity, $field, $instance, $la
- * Act on field_attach_load().
+ * Act on field_attach_validate().
...
- * Act on field_attach_validate().
+ * Act on field_attach_submit().
...
- * Act on field_attach_submit().
+ * Act on field_attach_load().

Can we just leave the order of hooks in field.api.php as is, please?

Fixing the capitalization of descriptions should be doable in no time. Seems to be the last blocker to get this RTBC again.

Powered by Dreditor.

Sure :)

Thank you!

Looks acceptable to me now, if testbot will pass. The context-specific usage of #parents, #field_parents, and #array_parents is still slightly confusing, but I can't think of a better way, so we likely just need to accept and live with this for D7.

I'd love to see a review by eff.

I won't be able to get to it before Wednesday, but will review it then, whether it's landed or not. The community process here has been good. No need to wait on me. I'll post follow-ups if necessary.

Thks sun & amateescu.

Updated patch
- clarifies #field_parents a little
- tweaks one or two other docs.
- gives a clearer / less confusing name to the variable around _field_form_state_parents().
(was : "all $path variables should be renamed to $field_parents" in sun's #107 - I go for $form_state_parents instead. This is a path within $form_state and, because of the BC considerations in _field_form_state_parents(), is slightly different from what we refer to as #field_parents in other parts of the code. Internal code only, no biggie)

Interdiff with patch #114 attached.
Docs + var rename only, keeping RTBC.

Summary is in #92.

re sun #107 :

+ $element = drupal_array_get_nested_value($form, array_slice($button['#array_parents'], 0, -1));
This construct and assumption looks fragile to me. Would it be possible to also add the #field_parents property directly on add more buttons.

The 'managed_file' FAPI element use that construct for its 'upload' and 'remove' buttons. What it means in practice is that you can't form_alter those buttons out of their parents (at least not before render time), which only seems reasonable. It also means less duplication of #custom_properties at multiple places in the form array (the button's parent has them, the button itself doesn't need them too).

Thanks to sun and yched who helped walk me through this patch on IRC.

yched's posts in #84 and #92 are helpful in explaining all the changes here. It looks pretty scary but the impact is mainly in Field API internals, so only those modules that are modifying those inetrnals (such as filefield's upload widget) should be affected.

It will, however, allow something like this http://drupal.org/files/issues/field-collection.png to be done in contrib. It's also the last major Field API problem on yched's radar. Given that it has approval from all the right corners (with the exception of effulgentsia, who as mentioned can review it after), I think this is good to go.

In the future, though, please move documentation nit-picking and the like to separate patches. It makes this already humongous patch look even more intimidating, and it causes reviewers/committers to burn a bunch of time trying to understand the impacts.

Major yay. Thanks webchick and everyone involved here. Field API uses just got much wider.

Special thanks @an.droid for kicking this off and going the hard way to identify the internal bottlenecks - which revealed this was much more doable than I would have thought off-hand.

As mentioned in #88, there's still a glitch with filefield upload widgets in combofield - works fine in a 'simple' nested form though, so I preferred to not hold the patch and investigate in a followup. Will create that one asap.

Congratulations on getting this in.

Is #92 the still-valid API description? Please explain what impact this will have on people. I assume this should be announced.

@rfay - API change summary :

API addition :

field_attach_form() can can attach fields at any nested location within a form structure, and place submitted field values at any nested location within $form_state['values']
(instead of currently : fields are always at the top-level of the form, and values always at the top-level of $form_state['values'])

This flexibility allows contrib to do things like 'two entities in one form without name clashes', and opens the door to a sane D7 solution for 'Multigroup / Combo field' (awesome screenshot : http://drupal.org/files/issues/field-collection.png - kudos @an.droid)

API changes :

- As a result, the $form parameter received by
hook_field_widget_form()
hook_field_widget_error()
hook_field_attach_form()
hook_field_attach_submit()
is no longer guaranteed to be a full form structure, but can be a nested part of a larger form.

In practice, this should only affect hook_field_attach_form() implementations willing to do stuff with the global form's #submit or #validate properties - which should be pretty rare. Workaround : set a #process callback and act on $complete_form in there, or use a regular hook_form_alter().

- widgets doing advanced funky stuff with $form_state['values'] directly, need to account for the fact that field values don't necessarily live at $form_state['values'][$field_name][$langcode] anymore.
To my knowledge, this only affects core's file upload widget.

@an.droid : #977890: Display collection fields within entity form (subform/embedded) :-)

follow up : #977898: Filefield upload widget broken in combofield

Great to see this landing! -> I've just committed the profile2 registration integration based on that patch, works great!

Beautiful. Simply beautiful. Thank you, an.droid, yched, and reviewers for making this happen! Very nice solution, with the right amount of BC for simple entity forms. Reading #117 and seeing that it landed just made my day!

@eff: cool to read - thanks :-D

Holy crap. Didn't realize this landed, but it totally simplifies some hackery I had to use to support this before. Huge thanks to everyone for making this happen. : )

Moving back to fixed just so I can add: Holy crap! This is awesome. Congrats to everyone who worked on this and managed to pull it off, and I'm sorry to have missed out on all the fun.