I have a nested field collection that looks like this:

Category (field collection)
> FAQ (field collection)
> Question (text field)
> Answer (text field)

I can add one or more categories and under each category I can add one or more FAQ items. But, when removing one of those FAQ item field collections it removes all FAQ items under the parent category.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

arknoll created an issue. See original summary.

tim.plunkett’s picture

Priority: Normal » Critical

Data loss is critical.

balsama’s picture

Technically, there does seem to be a workaround for this that avoids data loss. If you click the "Add another" button on the parent field collection after clicking delete, the previously erroneously deleted child field collections reappear (without the one that was intentionally deleted). I've verified that clicking save at that point does not result in data loss - and does, as expected, delete the single child field collection.

Maybe this can be a clue when troubleshooting?

juampynr’s picture

Status: Active » Needs work
FileSize
4.74 KB

This is where I am at. I could verify that for nested field collections, the second level elements are removed as expected.

I still need to work in the following interactions for this patch to be complete:

* Adding nested field collections (the wrapper id changed).
* Removing parent field collections.
* Adding test coverage.

juampynr’s picture

By the way, my patch is heavily influenced by FileWidget::submit(), which accomplishes something pretty similar when you click at the Remove button of an uploaded file.

tim.plunkett’s picture

  1. +++ b/src/Plugin/Field/FieldWidget/FieldCollectionEmbedWidget.php
    @@ -344,23 +331,40 @@ class FieldCollectionEmbedWidget extends WidgetBase {
    +    $parents = array_slice($button['#parents'], 0, -3);
    ...
    +    $submitted_values = NestedArray::getValue($form_state->getValues(), array_slice($button['#parents'], 0, -3));
    ...
    +    NestedArray::setValue($form_state->getValues(), array_slice($button['#parents'], 0, -3), $submitted_values);
    ...
    +    $updated_element = NestedArray::getValue($rebuilt_form, array_slice($button['#array_parents'], 0, -3));
    ...
    +    $wrapper_id = 'edit-' . str_replace('_', '-', implode('-', array_slice($button['#parents'], 0, -3))) . '-wrapper';
    

    Can we reuse this variable instead of redoing the array_slice?

  2. +++ b/src/Plugin/Field/FieldWidget/FieldCollectionEmbedWidget.php
    @@ -344,23 +331,40 @@ class FieldCollectionEmbedWidget extends WidgetBase {
    +    $parents = array_slice($button['#parents'], 0, -3);
    ...
    +    // Go one level up in the form, to the widgets container.
    +    $element = NestedArray::getValue($form, array_slice($button['#array_parents'], 0, -2));
    

    Every time we do an array_slice it'd be nice to have a comment explaining the 3rd param. This second call says "Go one level up" but has a -2

Otherwise, looks good so far!

juampynr’s picture

I made these array indexes as meaningful as possible, plus added comments. Now I will continue with the remaining tasks listed above.

juampynr’s picture

Adjusted wrapper identifiers so now both the Remove in nested field collections and the "Add another item" button in parent ones work. I am working now in fixing the Remove button of a parent field collection.

Here is a screenshot with annotations describing what I am up to:

juampynr’s picture

This patch simplifies the logic to generate wrapper identifiers. I am still debugging why the Remove button works sporadically. At least it does not delete unexpected field collections.

juampynr’s picture

Status: Needs work » Needs review
FileSize
1.62 KB
5.6 KB

This patch works much better but when you play with the add and remove buttons for a while, sometimes you see stalled values that were removed (not saved in the database, but temporarily in $form_state). I would like to get some feedback as it works much better than what we currently have in the module.

In the meantime, I will try a different approach which is to re-paint the whole field collection when any add/remove button is clicked. This should maintain values consistent.

juampynr’s picture

Oops, fixed a couple typos in comments.

juampynr’s picture

We already have code in FieldCollectionEmbedWidget::removeSubmit that does what I am doing at FieldCollectionEmbedWidget::ajaxRemove. I am debugging the former to see if this is the reason of some weird behaviors that I have found when you add and remove several items within the same page load.

juampynr’s picture

Here it is. I am not attaching an interdiff because this patch follows a different approach by doing the following:

* Fixes AJAX wrappers so they are unique.
* Fixes indexes in the removeSubmit() handler.
* Simplifies ajaxRemove() considerably, making it just to return the field collection item.

yannickoo’s picture

FileSize
1.28 MB

Thank you for you patch Juampy! It works well but when I'm trying to add new field collection items and delete them I experienced something funny with creating new items:

juampynr’s picture

Status: Needs review » Needs work

Thanks @yannickoo! I will debug that scenario and report back.

juampynr’s picture

Field collections are piling up within the widget state because the Remove submit handler is not removing them as expected. I am looking at the Drupal 7 code to understand how this should work.

juampynr’s picture

Drupal 7 piles up field collections in widget state but this does not seem to affect how the form gets rebuilt.

Here is how to reproduce the error in drupal 8:

Given a node multiple field collection item with a text field:

1. Go to add content and click on create a node.
2. Fill out the first field with a.
3. Click on Add another item.
4. Enter a b.
5. Remove the item with a.
6. Remove the item with b.
7. Enter c, click on Add another item.
- Expected: a new empty item is added.
- Actual: a new item containing c is added.

It looks like there is something when the form rebuilds that causes this. I am debugging the code.

juampynr’s picture

Status: Needs work » Needs review

@yannickoo, after debugging, I think that the issue that you describe at #14 is a different bug than the one described at the top of the issue. I could reproduce your issue without applying my patch at #13. Therefore, let's go step by step and set this to needs review again. I will create a new issue with your feedback.

@tim.plunkett, can you review this patch please?

juampynr’s picture

tim.plunkett’s picture

  1. +++ b/src/Plugin/Field/FieldWidget/FieldCollectionEmbedWidget.php
    @@ -252,6 +252,7 @@ class FieldCollectionEmbedWidget extends WidgetBase {
    +    $address_state = array_slice($button['#parents'], 0, -3);
    
    @@ -265,13 +266,11 @@ class FieldCollectionEmbedWidget extends WidgetBase {
    -      $old_element_state_address = array_merge($address, array($i + 1));
    -      $new_element_state_address = array_merge($address, array($i));
    +      $old_element_state_address = array_merge($address_state, array($i + 1));
    +      $new_element_state_address = array_merge($address_state, array($i));
    

    Nice! That explains a lot of the weirdness I was seeing while debugging.

  2. +++ b/src/Plugin/Field/FieldWidget/FieldCollectionEmbedWidget.php
    @@ -342,25 +341,13 @@ class FieldCollectionEmbedWidget extends WidgetBase {
    +   * @see $this::removeSubmit()
    

    This should be @see self::removeSubmit(), and should have a newline above it. I can fix on commit.

I think this looks good. Going to check with some coworkers about manual testing before I commit it.
I agree that the "add another item" bug can be a follow-up.

phenaproxima’s picture

Tested this manually and I can confirm that it fixes the issue described in the IS. Nice!

juampynr’s picture

Here is another patch with @tim.plunkett's suggestion.

balsama’s picture

Status: Needs review » Reviewed & tested by the community

I've also manually tested this and confirmed this works and it's integrated into our D8 Contrib build.

Marking as RTBC.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Fixed

Committed, thanks!

  • tim.plunkett committed 0145447 on 8.x-1.x authored by juampynr
    Issue #2613584 by juampynr, tim.plunkett: Removing field collection item...
yannickoo’s picture

<3

Status: Fixed » Closed (fixed)

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