Currently the add-more functionality of the field APIs multiple value field form just disables the form API persistence mode of the current form. That way forms are required to have their own persistence mode although the form API is able to deal with that. Thus it's not only harder to attach field API forms, it's also impossible to attach those field API form elements to regular drupal forms as required for #634984: Registration form breaks with add-more buttons.
To fix that issue, we need this. So I'm taking over priority.

We need to remove this code field_multiple_value_form() and make sure everything stays working:

  // This form has its own multistep persistance.
  if ($form_state['rebuild']) {
    $form_state['input'] = array();
  }

I think we have to care about re-ordering elements when it's gone. But as a start, here is a patch that just removes it. Let's see what tests break else.

CommentFileSizeAuthor
#13 field_form_fix.patch7.3 KBfago
#11 field_form_fix.patch3.65 KBfago
#3 field_form_fix2.patch3.3 KBfago
#1 fix_field_form.patch624 bytesfago

Comments

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Active » Needs review
StatusFileSize
new fix_field_form.patch624 bytes

Status: Needs review » Needs work

The last submitted patch, fix_field_form.patch, failed testing.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs work » Needs review
StatusFileSize
new field_form_fix2.patch3.3 KB

ok, I had a closer look at it. Even re-ordering the items is fine with that change - the only difference now is that the values get assigned to the original form elements now. Example given:

Before the rebuild:

delta  |  weight  | value
 0     |    0     |    1
 1     |    1     |    2
 2     |    2     |    3

After rebuild previously the elements were generated that way again, thus when I move value 3 to the top it would generate the elements like this:

delta  |  weight  | value
 0     |    -1    |    3
 1     |    0     |    1
 2     |    1     |    2

But when working with the form API persistence mode we get:

delta  |  weight  | value
 2     |    -1    |    3
 0     |    0     |    1
 1     |    1     |    2

Thus the original delta is preserved, but still the ordering is fine as the #weight is properly reflected. Also saving it that way is fine.

As preserving the original delta during a form rebuild is fine, there is nothing to fix apart from the tests which assume the delta to change. Updated patch that does so attached.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue tags: +D7 Form API challenge

only subscribing and tagging for now

yched’s picture

yched commented

To be fair, I don't remember when or why the snippet mentioned in the OP was added. Possibly as a compatibility fix during one of the $form_state / rebuild refectors ?

The remarks in #3 make me wonder if validation errors are still reported against the correct form element.
E.g :
- take an integer field, fill in values 1, 2, 3, submit
- edit the field settings and set 2 as maximum allowed value
- edit the field values, submit - the 3rd input should be flagged with an error
- grab the 3rd value and drop it in 1st position, submit - the 1st input should be flagged with an error.

(sorry, I'm on the road, hard to test by myself)

effulgentsia’s picture

effulgentsia commented

Subscribing. +1 in principle. Will try to review soon.

fago’s picture

fago
Primary language German
Location Vienna
commented

@yched:
I just gave that a test - it's working fine with and without JS.

And yes, the fix got in there when we introduced the form API persistence.

fago’s picture

fago
Primary language German
Location Vienna
commented

Imho this a rather important fix as it's also required to do a clean entity form workflow. Thus it would help us fixing #735800: node form triggers form level submit functions on button level submits, without validation and allow contrib to build a clean form workflow. So please review!

yched’s picture

yched commented

FWIW, this looks RTBC to me, but we could use the review effulgentsia mentioned in #6

effulgentsia’s picture

effulgentsia commented

This is a really nice improvement, and I'm glad tests pass with no code changes needed other than the main fix to field_multiple_value_form(). The 2 test changes are improvements as well: bonus!. Another bonus that #5 was tested successfully too. I'm not familiar enough with the innards of the entity/field system to know if there's some weird side effects this might cause to contrib modules, but if yched considers this RTBC, that makes me feel better. I tend to agree with fago that this patch is much more likely to help contrib modules that have field forms on them, than it is to hurt them, just like it is needed to help core forms like #634984: Registration form breaks with add-more buttons.

Small nits, but once those are fixed, RTBC.

+++ modules/field/tests/field.test
@@ -1562,18 +1562,17 @@ class FieldFormTestCase extends FieldTestCase {
+      $weights[$delta] = $weight;

6 lines above this is $weights[] = $weight, which does the exact same thing, but given the other changes in this patch, I agree with using $delta as an explicit index, so please remove the earlier line.

+++ modules/field/tests/field.test
@@ -1632,7 +1631,8 @@ class FieldFormTestCase extends FieldTestCase {
+      $weights[$delta] = $weight;

See above.

Powered by Dreditor.

fago’s picture

fago
Primary language German
Location Vienna
commented
StatusFileSize
new field_form_fix.patch3.65 KB

Thanks, nice catch. Fixed patch attached.

effulgentsia’s picture

effulgentsia commented
Status: Needs review » Needs work
Issue tags: +Needs tests

In addition to the nits in #10, can we bring over the tests from #634984-10: Registration form breaks with add-more buttons. I think just the test that fails in HEAD is needed: a test that adds a multi-valued field to the registration form.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs work » Needs review
StatusFileSize
new field_form_fix.patch7.3 KB

I planned to roll that as a separate patch, but you are right it belongs to this one. I've added in the whole test, as I think it's better to have both. That way if something goes wrong, one can determine easier what's wrong.

Patch including that test attached.

effulgentsia’s picture

effulgentsia commented
Status: Needs review » Reviewed & tested by the community

Textbook-quality patch: a single targeted code change, the improvement of existing tests to not do a weird workaround due to oddly coupled delta and weight, and the addition of a test that fails in HEAD and works with the patch. Awesome!

effulgentsia’s picture

effulgentsia commented 
+++ modules/simpletest/tests/form_test.module
@@ -922,3 +922,30 @@ function form_test_programmatic_form_validate($form, &$form_state) {
+    // The form API requires the builder function to set rebuilding, so do so.
+    $form['#builder_function'] = 'form_test_user_register_form_rebuild';

This does not change RTBC status, but I really don't understand this. Is it the Form API that requires this, the Field API, or the entity API (do we have an entity api)? Is it related to #367006: [meta] Field attach API integration for entity forms is ungrokable? Should this comment add a link to that issue?

The reason this doesn't change RTBC status for me, is that I do believe it's the correct code, and it's just that the whole #builder_function concept is confusing (to me), and we already have #367006: [meta] Field attach API integration for entity forms is ungrokable trying to deal with that, but coming to the conclusion that short of documentation, there's not much we can do about it for D7.

145 critical left. Go review some!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

LOL -- yeah, that's exactly part of the insanity that #367006: [meta] Field attach API integration for entity forms is ungrokable tries or tried to tackle. ;)

fago’s picture

fago
Primary language German
Location Vienna
commented

Indeed :) Furthermore this patch is the key to make it at least optional, as the last patch in #735800: node form triggers form level submit functions on button level submits, without validation does.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Nice clean-up, and glad to see it comes with well-written tests. Committed to CVS HEAD. Great work.

Status: Fixed » Closed (fixed)
Issue tags: -Needs tests, -D7 Form API challenge

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +Needs tests, +D7 Form API challenge

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.