Removing Multifield followed by 'add another item' adds back the removed item

So, I've been working on looking at all the linked issues I just added and starting to refactor those updates into Multifield. Not trivial and there are many patches stacked on top of each other in the current state of Field Collections but I do think that those issues hold the answer to this.

Just throwing notes in here:

The 'Added a remove button to field submit widget' was added with http://cgit.drupalcode.org/multifield/commit/?id=15683eb1b68f75a37df4a2e... on 2013-07-10 22:57:55 which consists of a copied field_collection_remove_submit() and field_collection_remove_js() but it was not precisely a copy and appears to have been modified a little bit when comparing against the field_collection code base around that time.

I did a git history for selection in PhpStorm on just the functions field_collection_remove_js() & field_collection_remove_submit() and these are the changes between the time these functions were copied from FC, 2013-07-10 and now:

1. https://www.drupal.org/node/2416237 - 2016.07.28 - Deleting field collection items from node, then adding more creates multiple revisions of same field collection entity on save
2. https://www.drupal.org/node/2456307 - 2016.05.02 - Item Remove Button confuses #parents and #array_parents
3. https://www.drupal.org/node/1675522 - 2015.10.19 - Remove button broken if form is rebuilt (via ajax or failed validation)
4. https://www.drupal.org/node/2242751 - 2014.07.03 - Ajax form page callback require updates for Drupal 7.27 (SECURITY) list() = drupal_get_form()/Implemented #2242663 for SA-CORE-2014-002 (Ajax form page callback security fix for anonymous users).
5. https://www.drupal.org/node/2123559 - 2014.03.18 - - Removing field collection item also removes sibling's nested child content/ Fixed form_state on javascript remove button.

So, a while ago I did reapply all of these patches but that did not resolve this issue.

Field Collection does still work fine, these seems like an off by 1 issue.

Just so I don't lose the work and maybe it will be useful, here are the patches in #7 backported to Multifield. https://github.com/ElijahLynn/drupal-multifield/commits/7.x-1.x

I don't remember why #2416237: Deleting field collection items from node, then adding more creates multiple revisions of same field collection entity on save isn't there.

update: Oh yeah, because it is trying to get $field_state['entity'] and that appears to be something Field Collection puts there. I don't think that patch is valid for multifield but maybe the concept is in a different way.

+++ b/field_collection.module
@@ -1087,9 +1087,14 @@ function field_collection_remove_submit($form, &$form_state) {
+  // Use the actual array of field collection items as the upper limit of this
+  // for loop rather than 'item_count'. This is because it will be creating extra
+  // dummy items here and the two measures go out of sync after the fist delete.
+  $field_collection_item_count = count($field_state['entity']) - 1;
+
   // Go ahead and renumber everything from our delta to the last
   // item down one. This will overwrite the item being removed.
-  for ($i = $delta; $i <= $field_state['items_count']; $i++) {
+  for ($i = $delta; $i <= $field_collection_item_count; $i++) {
     $old_element_address = array_merge($address, array($i + 1));
     $new_element_address = array_merge($address, array($i));

Okay, so I deep dove into this and I have a working patch.

I tried not to but ended up doing mostly what Field Collection is doing. FC is managing the field state with $form_state['field']['entity'] where they update it on the remove_submit() handler and rebuild the form based off those values. I couldn't find any other way.

This patch introduces $form_state['field']['state'] (I didn't want to call it entity). It is initialized in multifield_field_widget_form() and then updates the state in multifield_field_widget_item_remove_submit().

The remove (delete) button functionality was originally introduced in this patch in this issue #1234574: Add a delete button on the field-collection item edit form which is also the source of field_collection_remove_submit() that we copied into multifield_field_widget_item_remove_submit(). The issue came across the same issue this issue is about (https://www.drupal.org/node/1234574#comment-5205538). I think we didn't copy the parts that said 'entity' because it made sense we didn't have to deal with that. But although it was for entities, the field state saving part is what really mattered. In my patch I don't have entities but enough information to be useful within multifield_field_widget_form().

Thanks for the review.

1. Yeah, it would delete the 3rd item. The logic took me a bit to figure out. $i is actually set from a $delta earlier on. When that submit handler is called it is passed the delta of the item being removed. So the for loop starts with $i being the item being removed. Here is the full context:

 // Go ahead and renumber everything from our delta to the last
  // item down one. This will overwrite the item being removed.
  for ($i = $delta; $i <= $field_state['items_count']; $i++) {
    $old_element_address = array_merge($address, array($i + 1));
    $new_element_address = array_merge($address, array($i));

    $moving_element = drupal_array_get_nested_value($form, $old_element_address);
    $moving_element_value = drupal_array_get_nested_value($form_state['values'], $old_element_address);
    $moving_element_input = drupal_array_get_nested_value($form_state['input'], $old_element_address);

    // Tell the element where it's being moved to.
    $moving_element['#parents'] = $new_element_address;

    // Move the element around.
    form_set_value($moving_element, $moving_element_value, $form_state);
    drupal_array_set_nested_value($form_state['input'], $moving_element['#parents'], $moving_element_input);

    // We use $field_state['state'] so that we can manage removed fields with
    // the 'remove' button.
    // @see multifield_widget_form()
    if (isset($field_state['state'][$i + 1])) {
      $field_state['state'][$i] = $field_state['state'][$i + 1];
    }
    else {
      unset($field_state['state'][$i]);
    }
  }

2. Yeah, sometimes I leave comments some breathing room. The standards say cannot be more than 80 characters but don't say that I must use all 80 characters when possible ;).

3. Yeah, those comments should actually be todos. But it was super late and I definitely just left them in there. If we re-roll this patch it would be nice to turn them into real "why" comments. There is a lot of "why" missing from this code.