To reproduce

- install inline_entity_form_test
- navigate to /node/add/ief_test_nested1
- Create node incl. both nested referenced nodes
- Save
- Edit node that you just created
- Open first IEF node
- Open second IEF node
- Modify something on last level
- Close second IEF node (save changes)
- changes appear in the table
- Close first IEF node (save changes)
- Save host node
- Go back to edit page
- Open both IEF levels
- Expected result: changes on second level were saved
- Actual result: changes were not saved

This bug does not appear if we don't close IEF forms (submit with all levels open).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

slashrsm created an issue. See original summary.

IT-Cru’s picture

Same problem here with with D8.0.3, paragraphs, media_entity and IEF 1.0-alpha4 (without and with patch from #2649710-25: Required nested IEF forms don't work).

* node
** paragraphs (gallery bundle)
*** IEF media (gallery bundle)
**** IEF media (image bundles)

No errors thrown or logged.

Create new image media works.
Updating existing image media do not work.
Editing image media via /media/galleryID works. (work-around)

IT-Cru’s picture

In EntityInlineForm.php Line 295 $entity_form['#save_entity'] is false, so our edit image media won't be saved anyway. Any idea why this isn't TRUE?

if ($entity_form['#save_entity']) {
  $entity->save();
}

When I set it to TRUE updating nested image media works again.

chr.fritsch’s picture

Now we add save_entity true during the form build process. But we are not sure if this is the way to go

IT-Cru’s picture

Status: Active » Needs review
bojanz’s picture

Status: Needs review » Active

No, IEF widgets save the entities themselves, #save_entity should never be TRUE.

Status: Active » Needs work

The last submitted patch, 4: save-entity.patch, failed testing.

chr.fritsch’s picture

In the SimpleWidget we also use #save_entity = True. Maybe thats another problem

tedbow’s picture

@chr.fritsch the problem with the patch is that it saves the entity immediately. So even if you don't save the parent entity the nested entities in IEF will be saved.

All of the entities in ief field widgets should only be saved in the final parent entity form submit. So if never end up saving the top level parent entity(close a tab, click a link) none of ief entities will be saved

tedbow’s picture

I have done a little testing with the test module as described in the issue summary

On /node/add/ief_test_nested1

Adding a new node in method describe in the summary

When saving the top level parent:
The entity in second level is saved in \Drupal\inline_entity_form\Plugin\Field\FieldWidget\InlineEntityFormComplex::extractFormValues
For the field test_ref_nested1

The entity is for test_ref_nested2 in the field entity values for the test_ref_nested1

In extractFormValues

if (!empty($item['needs_save'])) {
            $entity->save();
          }

is actually being called twice for test_ref_nested1

extractFormValues never get called for test_ref_nested2 in the final submit.

When editing the node in the top level submit

extractFormValues is also called twice for test_ref_nested1 and never for test_ref_nested2

But this time only the target_id(entity id) is for test_ref_nested2 in the field entity values for the test_ref_nested1

So because entity for test_ref_nested2 doesn't have it's new values available to test_ref_nested1. It doesn't get updated.

IT-Cru’s picture

With D8.0.1 and IEF commit e039a1ea7bd976b2951a83f14c5ff7e7eae40782 our szenario works correct. And now with D8.0.3 and IEF 1.0-alpha4 it doesn't work any more.

IT-Cru’s picture

Perhaps following line breaks it down between e039a1ea7bd976b2951a83f14c5ff7e7eae40782 and 1.0-alpha4 in InlineEntityFormComplex.php?

if (!empty($trigger['#ief_submit_all'])) {
  ...
  $entity->save();
  ...
}
tedbow’s picture

@IT-Cru e039a1ea7bd976b2951a83f14c5ff7e7eae40782 = alpha3

As far as trying figure what might have changed this functionality there have still been a lot changes since alpha4 also.

To create a patch we need to do against 8.x-1.x-dev

I don't think that code you mentioned in #12 is still there.

bojanz’s picture

IEF used to save entities in validation by mistake (and it was being triggered in AJAX and on form validation fails), that got fixed, exposing this problem.

tedbow’s picture

Status: Needs work » Needs review
FileSize
2.49 KB

Heres a patch:

Currently \Drupal\inline_entity_form\ElementSubmit::trigger tries to handle all nested IEF forms. The problem is that forms that were nested and have been submitted via ajax don't exist on the form as elements. For top level IEFs this is not a problem because they still exist on the form as widgets but not nested ones.

So I added \Drupal\inline_entity_form\ElementSubmit::handleFormStateEntities

That loops through all IEF entities in the $form_state and saves them.

This probably saves ones that have already been saved because 'needs_save' is not removed on $entity->save() in \Drupal\inline_entity_form\Plugin\Field\FieldWidget\InlineEntityFormComplex::extractFormValues

I am trying this approach after @bojanz told me how inline_entity_form_field_attach_submit works in Drupal 7

tedbow’s picture

Here is patch with tests and TEST_ONLY patch.

It tests editing the nested node with and without ajax submitting the IEF widgets for required and non-required forms.

The last submitted patch, 16: ief-nested_edit_save-2674102-16-TEST_ONLY.patch, failed testing.

slashrsm’s picture

Status: Needs review » Needs work

Thank you @tedbow. Tested it and I can confirm that it fixes the problem. Few nitpiks:

  1. +++ b/src/ElementSubmit.php
    @@ -111,4 +113,30 @@ class ElementSubmit {
    +   * @todo This will currently probably save entities that have already been
    +   *   saved. 'needs_save' is not removed on $entity->save() in
    ...
    +   *
    

    Could we set "needs_save" to FALSE in InlineEntityFormComplex::extractFormValues()?

  2. +++ b/src/ElementSubmit.php
    @@ -111,4 +113,30 @@ class ElementSubmit {
    +      if (!empty($inline_form_state['entities'])) {
    +        $entities = $inline_form_state['entities'];
    +        foreach ($entities as $entity_info) {
    +          if ($entity_info['needs_save'] == TRUE && $entity_info['entity']) {
    +            /** @var \Drupal\Core\Entity\ContentEntityInterface $entity */
    +            $entity = $entity_info['entity'];
    +            $entity->save();
    +            unset($entity_info['needs_save']);
    +          }
    

    $inline_form_state['entities'] and $inline_form_state['needs_save'] are field widget specific things. While this should not break any other usages because of empty check we should still note that in a comment.

    I suspect that unset() won't affect the value in $form_state, which is what we want here.

  3. +++ b/src/Tests/InlineEntityFormComplexWebTest.php
    @@ -1,6 +1,7 @@
    +use Drupal\node\Entity\Node;
    

    Missing newline between namespace and use.

  4. +++ b/src/Tests/InlineEntityFormComplexWebTest.php
    @@ -42,6 +43,12 @@ class InlineEntityFormComplexWebTest extends InlineEntityFormTestBase {
    +   * Node storage.
    +   * @var \Drupal\Core\Entity\ContentEntityStorageInterface;
    +   */
    +  protected $nodeStorage;
    

    Missing empty line before @var.

  5. +++ b/src/Tests/InlineEntityFormComplexWebTest.php
    @@ -218,12 +229,50 @@ class InlineEntityFormComplexWebTest extends InlineEntityFormTestBase {
    +   *
    +   * @param \Drupal\node\Entity\Node $node
    +   *  Top level node of type ief_test_nested1 to check.
    +   * @param bool $ajax_submit
    +   *
    +
    +   */
    

    Missing description for second param and extra newline at the end.

bojanz’s picture

handleFormStateEntities() is misplaced. ElementSubmit provides form-level functionality, this code is IEF widget specific.

chr.fritsch’s picture

Fixed most of the nitpicks and moved handleFormStateEntities to the InlineEntityFormBase

chr.fritsch’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 20: ief-nested_edit_save-2674102-20.patch, failed testing.

tedbow’s picture

@chr.fritsch the reason this fails is because handleFormStateEntities should only run when the top level form(not ief form) submits.

I am looking at another way to attach it to the complete form.

tedbow’s picture

It seems that attaching handleFormStateEntities to submit of the parent form runs into the same problems that ElementSubmit is solving.
Namely:
Making sure it is only attached once no matter how many widgets are attached
Making sure it is attached to all top level elements that could submit the parent form.

I am working on patch that would add 2 new classes
SubmitBase
WidgetBase

ElementBase would extend SubmitBase

Submit base would handle
Attaching callback to "trigger" of extending classes once and only once to top level elements.

WidgetBase would only take effect if widget are also on the form.

Here is where i am working https://github.com/tedbow/inline_entity_form/tree/2674102-nested_save-su...

It currently passes all tests including the new ones but is not "patch ready" because I need to do clean up.

bojanz’s picture

Could we just attach handleFormStateEntities via $form['#ief_element_submit'] instead of extending the ElementSubmit class?

tedbow’s picture

@bojanz we could but we need to make sure that handleFormStateEntities is attached to top level buttons and form for the parent form and that is attached only once. handleFormStateEntities should not be called once per widget but once when the parent form is finally submitted. It seems like those same problems is already what ElementSubmit is solving.

tedbow’s picture

Status: Needs work » Needs review
FileSize
9.67 KB

ok here is patch with a new class WidgetSave.

I also add fixes suggested in #18

bojanz’s picture

Title: Nested IEF elements are not saved correctly in some cases » extractFormValues() fails to delete entities, save nested ones
Priority: Normal » Major
Status: Needs review » Needs work

tedbow and I agreed that this patch should also cover deleting entities.
Turns out deleting is broken right now, referenced entities are removed but never deleted. And yet tests pass.

  • bojanz committed b3caaf2 on 8.x-1.x authored by tedbow
    Issue #2674102 by tedbow, chr.fritsch, bojanz: extractFormValues() fails...
bojanz’s picture

Status: Fixed » Closed (fixed)

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