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).
Comment | File | Size | Author |
---|---|---|---|
#27 | ief-nested_edit_save-2674102-27.patch | 9.67 KB | tedbow |
| |||
#20 | ief-nested_edit_save-2674102-20.patch | 9.34 KB | chr.fritsch |
#16 | ief-nested_edit_save-2674102-16.patch | 8.51 KB | tedbow |
| |||
#16 | ief-nested_edit_save-2674102-16-TEST_ONLY.patch | 6.01 KB | tedbow |
#15 | ief-nested_edit_save-2674102-15.patch | 2.49 KB | tedbow |
|
Comments
Comment #2
IT-CruSame 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)
Comment #3
IT-CruIn 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?
When I set it to TRUE updating nested image media works again.
Comment #4
chr.fritschNow we add save_entity true during the form build process. But we are not sure if this is the way to go
Comment #5
IT-CruComment #6
bojanz CreditAttribution: bojanz at Centarro commentedNo, IEF widgets save the entities themselves, #save_entity should never be TRUE.
Comment #8
chr.fritschIn the SimpleWidget we also use #save_entity = True. Maybe thats another problem
Comment #9
tedbow@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
Comment #10
tedbowI 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
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.
Comment #11
IT-CruWith 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.
Comment #12
IT-CruPerhaps following line breaks it down between e039a1ea7bd976b2951a83f14c5ff7e7eae40782 and 1.0-alpha4 in InlineEntityFormComplex.php?
Comment #13
tedbow@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.
Comment #14
bojanz CreditAttribution: bojanz at Centarro commentedIEF 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.
Comment #15
tedbowHeres 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
Comment #16
tedbowHere 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.
Comment #18
slashrsm CreditAttribution: slashrsm at Examiner.com commentedThank you @tedbow. Tested it and I can confirm that it fixes the problem. Few nitpiks:
Could we set "needs_save" to FALSE in InlineEntityFormComplex::extractFormValues()?
$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.
Missing newline between namespace and use.
Missing empty line before @var.
Missing description for second param and extra newline at the end.
Comment #19
bojanz CreditAttribution: bojanz at Centarro commentedhandleFormStateEntities() is misplaced. ElementSubmit provides form-level functionality, this code is IEF widget specific.
Comment #20
chr.fritschFixed most of the nitpicks and moved handleFormStateEntities to the InlineEntityFormBase
Comment #21
chr.fritschComment #23
tedbow@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.
Comment #24
tedbowIt 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.
Comment #25
bojanz CreditAttribution: bojanz at Centarro commentedCould we just attach handleFormStateEntities via $form['#ief_element_submit'] instead of extending the ElementSubmit class?
Comment #26
tedbow@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.
Comment #27
tedbowok here is patch with a new class WidgetSave.
I also add fixes suggested in #18
Comment #28
bojanz CreditAttribution: bojanz at Centarro commentedtedbow 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.
Comment #30
bojanz CreditAttribution: bojanz at Centarro commentedThis was a tricky one. Committed in https://github.com/bojanz/inline_entity_form/commit/b3caaf2433741849d553...
Followup: #2678166: Expand InlineEntityFormComplexWebTest to cover deleting entities.