The CR checkout pane form misuses the Field API when handling entity fields, causing numerous issues

removing unnecessary header

clarification about "dropping the value" vs. "not allowing it into the form_state array"

I updated the issue to be more clear about an important distinction:
The IEF form values are correctly rendered into the $form array on page load. They are just missing from the $form_state['values'] array on submit callback. It's almost like the $form_state is somehow built before the IEF form gets added to the $form.

I ran a few tests and found that this has to be related to the commerce_registration_information_checkout_form() function, because the registration form works great on standard registration entities on ER's default /registration/%registration/edit path (which uses registration_form() directly)

Around line 86 of commerce_registration.checkout_pane.inc there is some odd use of the registration_form() function that seems to be the source of this problem:

  $tmp_form_state = array();
  $register_form = registration_form($form, $tmp_form_state, $entity);

The rest of the function then "glues" together parts of the registration form with the checkout pane form. If i drop out all of the glue and pass the real $form_state into registration_form(), then my $form_state passes the right data through to the IEF callbacks and my error goes away.

Can anyone explain why there is a need to add the registration form is such a strange way (with use of a phony $tmp_form_state and odd corrections of $form['#parents'], etc?

I'm changing the title to more accurately describe this issue.

The problem arises with IEF because IEF needs the form #parents to be constructed properly in order to function. The #parents array is glued to together in commerce_registration_information_checkout_form() in a way that breaks the $form_state structure.

This reconstruction of the parents hack can easily be avoided with a small alteration to the code described above.

Rather than going back into the form after calling field_attach_form() and trying to alter the parents configured there, the parents can be set on the $form[$prodkey][$prodkey . '-reg-' . $i] element before calling field_attach_form().

This is how the documentation on field_attach_form() says how to alter the parents in such situations:

By default, submitted field values appear at the top-level of $form_state['values']. A different location within $form_state['values'] can be specified by setting the '#parents' property on the incoming $form parameter. Because of name clashes, two instances of the same field cannot appear within the same $form element, or within the same '#parents' space.

Unfortunately there are some other issues with the way the registration entity form is attached. The instance ID's on individual entity fields are not unique. This causes failures in AJAX callbacks and in the Conditional Fields module when there is more than 1 registration form on the pane.

Attached is a patch to illustrate the correct use of field_attach_form() and the $form['#parents'] param, but I think this needs more work... more patches to come..

more clarification and formatting

updating to cover broader problem, rather than specific symptom

Reattaching last patch for review. After some testing on my end I can see that this does not noticeably break a basic commerce registration process. Conditional fields and IEF fields, however, are still not fixed.

More work on the way..

Thanks jpontani

I found another issue related to this process that was causing more problems for me with use of IEF:

commerce_registration_entity_update_fields() will take form state values and assume they are ready for direct insertion to the entity object:

  foreach ($values as $key => $value) {
    $entity->$key = $value;
  }

The problem is that some fields, like IEF, will execute code in field_attach_submit() that will manipulate the form_state, modify the entity object and prepare the field for saving.

The problem is that after this might happen, commerce_registration_entity_update_fields() bulldozes the construction of the entity's field by overwriting it with the exact structure of the $form_state array.

I have to call it a day today, but I will pick it up here and patch tomorrow.

added related issue to summary

found another related issue

edited to be more about found deeper issues, less about original issue

found another related issue

ok, so simply removing commerce_registration_entity_update_fields() does the trick just fine.

I've tested this fields with unlimited values (text, file upload and entity reference) and it works great.

I imagine commerce_registration_entity_update_fields() used to be necessary before the last patch I put up that prevents $form_state from coming through all messed up. As far as I can tell the "update fields" function is longer necessary. I need to test results on multiple registrations at once -- i was seeing "Instance ID" problems on individual fields earlier.

Attached is an updated patch.

crap there are some issues here i don't think this patch aligns with the most recent 7.x-2.x version -- re-rolling...

re-rolled, tested with unlimited values on individual text, file upload and entity reference fields. Still need to fix possible issues with multiple registrations per page.

so far so good on multiple registrations with a variety unlimited field types. IEF is acting strange on the AJAX callback, but that might actually turn out to be an issue with IEF

I'm still having issues on multiple registrations at once: both the conditional fields module and the IEF module don't do well with the second registration form on one page. The IEF module uses the field instance ID on to identify an area for JS to manipulate. I think conditional fields does the same thing. The problem is that neither module expects to find the entity add/edit form loaded twice onto one page. I wonder which would make more sense:

• refactor this module to use mulitple checkout pages, one for each registration
• patch IEF and conditional fields to allow multiple entity forms on one page

@jpontani, any thoughts?

nevermind!

I think the issues I'm still seeing are in fact IEF/Conditional Fields issue. If #13 is committed I think this issue can be closed.

IEF/Conditional Fields issues:
#1803568: Multiple calls to field_attach_form() causes conflicts
#1740074: Fix IEF to work when IEF is nested into another IEF
#1835852: Multiple same-entity add/edit forms on one page results in duplicate DOM Id's

patch referenced addresses part of the issue, not the whole issue

crap this breaks the default values of "This registration is for:" when using the "Go Back" button

nevermind -- that's an unrelated issue. The current 7.x-2.x branch does not work when using "Go Back" even without the mod above. I still stand by #13!

@jpontani -- did #13 get committed? I'm trying to re-roll this to the most recent version of 7.x-2.x and it looks like it did.

however, there is still a documentation reference to the removed function -- updated patch addresses this