Unable to keep nested IEF data separate with multivalue fields.

Can you test this to see if this problem still happens with the latest 8.x-1.x-dev development snapshot? There has been a lot of changes since then.

Please report back if it is still broken.

If it is still broken can you give more details about the set for each field use IEF? required, which widget, multiple values?

Thanks

I can confirm having this issue even with the latest dev version of IEF. After some debugging I've come to the conclusion that the bug only happens on PHP 7 and not PHP 5.6. Here is how I reproduced it:

1. Fresh install of Drupal 8.0.3
2. install IEF latest dev version
3. Create a entity reference field on an Article node that references Article nodes and set form display to "Inline entity form - Complex"
4. Add a new Article
5. In the edit form add a new referenced Article node with the title "Test 1" and create it
6. Add another Article, call it "Test 2" and create it.

When using PHP 7 both items will suddenly be called "Test 2" after creating the second node. On PHP 5.6 first item is still called "Test 1" and the other "Test 2".

I tested this using Drupal VM so I could switch between PHP 5.6 and PHP 7 easily.

Not sure what's actually causing this, but maybe this can point to some code in IEF that doesn't work correctly on PHP7?

Just re-tested with the latest dev version (2016-Feb-19) and I still get the issue on PHP7 and not PHP 5.6.

If I create one referenced node, save the parent node and edit it again, I can add another reference without it overwriting the first. But if I then add another one it will overwrite again. This means I can only add one entity reference per node edit using PHP 7.

This happens using the "Inline entity form - Complex" widget.

Tell me if you need any additional info about my setup or how to reproduce it.

Thanks!
There's no point in chasing this bug until we rewrite the entire logic in #2667710: Rewrite the base inline form handling.

I understand, thanks!

I have retested this with the latest Dev now that the issue in #6 has been merged. Unfortunately I still get the same error as before. Is it time to further investigate the issue?

Yes! Thanks for checking.

[Deleted]

Ahhhh, public function formElement(). I call you The Beast.

Happens because $this->setIefId(sha1(implode('-', $parents))); in public function formElement() at inline_entity_form/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php.

Delta is not in #field_parents array. #field_parents is the same for all deltas, therefore all form elements gets same IEF ID - one created for first or latest reference value.

@mansspams
Great debugging!

Here is my version of fix. We add parent delta to IEF ID.

Problem was, as already written, that each child of multi value field has same IEF ID since parents were same. Now we add delta of parent and things seem to work.

I guess it's time update tests.

With adjusted tests. Should we add also cardinality test like on Simple widget test?

@mansspams Thank you so much for looking into this!

I tried applying the the patch in #19 on IEF 8.1.0-alpha5. But I am still getting the error of the second entity overwriting the first.

This problem is really critical since IEF is currently overwriting data when editing which makes it unusable!

We need a failing test first, demonstrating the bug.

Since I got the problem in PHP7 only I tried running the patch through the testbot in PHP7 but it seems that the patch fails to apply at all. Not sure why?

PHP7 tests are passing on Travis: https://travis-ci.org/bojanz/inline_entity_form/builds/112699505

One more reason why we need a failing test here.

@bojanz The travis link you posted seems to link to another issue though? From what I understand mansspams has adjusted tests for this issue in #19.

@reekris Correct, I linked to the build for the latest 8.x-1.x commit.

Sorry for being so stupid but do you mean that the build you linked to includes the changes in #19? So it means that the tests mansspams adjusted in #19 passes in PHP7?

@reekris
No, mansspams added no tests for the actual bug, he only tweaked the existing assertions.
I am saying that as far as our build is concerned, IEF is fully functional on PHP7, which obviously means we are missing coverage.

This patch adds a test which tries adding another reference after the first one in the Complex widget. This test passes for me on PHP5.6 and fails on PHP7. Let's see what the testbot says.

It's the first time I submit a test patch so I'm not really sure what I'm doing, hopefully this works..

Aha, it seemed to have worked! The test fails in PHP7. Problem is, I still have no idea what is causing the issue.. Where do we go from here?

This patch contains mansspams patch from #19 along with the test from #28, to see if the patch fixes the issue.

My patch did change parents for all IEF elements and my changes to tests was not about testing what I fixed, but not to brake previous tests, failing test needs to be created from scratch. I am not very good yet at writing tests.

In order to make failing test we need to test one specific issue - we add first value to entity reference field with unlimited cardinality and while adding another, we need to see if parents are the same. Which it was before patch and is not after patch. Or something like that.

src/Tests/InlineEntityFormSimpleWebTest.php has protected function testSimpleCardinalityOptions() to test cardinality, maybe we need similar for src/Tests/InlineEntityFormComplexWebTest.php?

@mansspams In #28 I attached a test that does test this issue. It adds a second reference after the first and as you can see it passes in PHP 5.6 and fails in PHP 7.

#31 is a combination of your patch and my addes test.

So I guess that means that the test is alreafy in place and we can move on to try to fix the issue. Unless you have any comments on the test I created?

@reekis I cannot comment on test. What is usual procedure? First test should be committed and then fix in #16?

Not sure what the proper procedure is, im quite new to contributing :)

The problem for me is that I still have the same issue after applying the fix in #16. To make sure I created a patch including both the fix from #16 and my added test from #28. It is the one in #31, and as you can see there it still fails the test.

Maybe we actually have two different issues and your patch fixes yours but not mine?

Just spent another couple of hours trying to find the source of this issue without success.

Saw another thing which may give some hint to what the problem could be:

When adding a second reference it overwrites the first one.
But if I add a third reference it wiull also overwrite the first one but keep the second one intact.

So the IEF table of entities looks like this:

After adding the first reference:

1. Reference 1

After adding the second reference:

1. Reference 2
2. Reference 2

After adding the third reference:

1. Reference 3
2. Reference 2
3. Reference 3

This also only happens when adding multiple entities at once. If I save the parent node between creating each reference it works as espected.

I think problem is described above, maybe in not enough detail - each value in multivalue IEF form element

IEF builds nested forms by encoding parents of form in IEF ID. Delta was not part of form parents. You create first value, it gets saved in form state with ID like "parent-childX-childY". You add another value to childX and find that childY form element already has value. Because both childY parents and therefore IEF IDs are same.

Patched code builds ID differently (see #16) - "parent-childX-delta-childY". Now you add first value of childX and first value of childY and for that childY ID will be made from "parent-childX-0-childY". Add another childX value and clean form element will be created, because for this ID will be made from "parent-childX-1-childY".

Before:
parent-childX-childY
parent-childX-childY

Now:
parent-childX-0-childY
parent-childX-1-childY

@mansspams thanks for the explanation, i think I understand it better now. The weird thing for me is it seems when using PHP 5.6 I don't even have this issue, it only happens in PHP 7. Applying the patch in #16 does not help.

Which version of PHP are you testing on?

I tried to debugging your added getParentDelta method. It seems that it always returns 0 since $element['#field_parents'] is always empty when passed in to that method.

@reekris I am testing with PHP 5.6 and #16 helps. Make sure we are tackling same issue :)

Lets see if I can make description clearer.

Setup:

• Node X with field_reference_y targeting Node Y (unlimited values, complex widget)
• Node Y with field_reference_z targeting Node Z (single value, complex widget)
• Node Z

Steps:

1. Open Node X add form
2. Add new Node Y as first value of field_reference_y
3. Add new or existing Node Z as value of field_reference_z
4. Save Z
5. Save Y
6. Add new Node Y as second value of field_reference_y
7. Form for new Node Y will open

Expected

New (empty) Node Y form opens.

Result

Node Y form opens, but field_reference_z already has value - Node Z created or referenced in step 3.

@mansspams It seems we are actually tackling two different issues. Reading the original issue description again I see now that my issue is something else and your patch is actually solving the problem described.

Thanks for clearing that up and sorry for ranting about my unrelated PHP 7 issue here :)

I have created a new one for the PHP 7 problem and I've currently moved my project back to 5.6 until I have more time to look at the problem, if no one else finds a solution before me.

For this issue I guess it could be set to needs review to get your patch merged?

I have exactly the same issue for version 7.x-1.6
Is there thread for 7?

Hi guys,

I tried patching this against the latest dev and the patch fails to apply. What should I be patching against?

Latest dev. Needs re-roll. Will do.

Re-roll.

Well...

My hands are not very straight. Ignore this :(

FTW

;(

;,,(

Not my day I guess.

Can you add some interdiffs? That would make easier to review your patches.

Checking if my day. Re-roll of #19.

Hi guys,

I tried patching this against the latest dev and the patch fails to apply.

Re-test was a pass, so seem to apply to latest dev.

Here is a re-roll against the latest dev. I was also noticing the latest patch did not apply cleanly.

Humm apparently failing somewhere in TranslationTest now...

There's something more wrong here. Try this:

My setup is similar to the one in the description, just simplified down to the essentials.

Content type "Level 1" with entity reference to content type "Level 2" using IEF Complex
Content type "Level 2" with entity reference to content type "Level 3" using IEF Complex
Create a node of type "Level 1"
Using IEF Complex create a "Level 2" node
Using IEF Complex create a "Level 3" node
At this stage looks like this: http://take.ms/3lAic
Save both "Level 2" and "Level 3" nodes with IEF Complex widgets
Edit the existing "Level 2" node using the button in the table row (http://take.ms/XN4sN)
At this stage looks like this: http://take.ms/le37r
Save the "Level 1" node
Error is thrown and the node fails to save :(

Error shown on screen: "The website encountered an unexpected error. Please try again later."
Errors in watchdog:

Drupal\Core\Database\IntegrityConstraintViolationException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '256a2b9e-2cb2-455a-9ca4-64e363b9d01e' for key 'node_field__uuid__value': INSERT INTO {node} (vid, type, uuid, langcode) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3); Array ( [:db_insert_placeholder_0] => [:db_insert_placeholder_1] => level_3 [:db_insert_placeholder_2] => 256a2b9e-2cb2-455a-9ca4-64e363b9d01e [:db_insert_placeholder_3] => en ) in Drupal\Core\Database\Connection->handleQueryException() (line 668 of /var/www/docroot/core/lib/Drupal/Core/Database/Connection.php).

Drupal\Core\Entity\EntityStorageException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '256a2b9e-2cb2-455a-9ca4-64e363b9d01e' for key 'node_field__uuid__value': INSERT INTO {node} (vid, type, uuid, langcode) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3); Array ( [:db_insert_placeholder_0] => [:db_insert_placeholder_1] => level_3 [:db_insert_placeholder_2] => 256a2b9e-2cb2-455a-9ca4-64e363b9d01e [:db_insert_placeholder_3] => en ) in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 756 of /var/www/docroot/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).

I think it stems from the fact that as soon as you click Edit on an entity that was Created, the child entities exist in more than one place in the form_state.

This is a problem with and without the patch here btw. Not sure it separate issue. It all seems related though.

Let's try this. Been working on it awhile. Hoping this will knock out this and a number of related issues. Explanation to follow, just want to make sure I fixed all the tests..

Excellent. OK so what I found through hours and hours of debugging was that there were two fundamental issues:

* As mansspams and others have found, the parent delta was missing as part of the #parents key's, and needed to be included somehow.
* The #field_parents and thus #parents can be different between an 'add' form and an 'edit' form. I found this inconsistency was producing the strange results that were first reported in this ticket and also the other issues reported in #68. What was happening is b/c of the inconsistency in #field_parents from when an entity was first created via and 'add' form, and then was edited via an 'edit' form, it's values would end up stored in $form_state under two different $ief_id's, thus throwing the DB errors I reported.

The patch I posted does a couple things:

* Breaks up the task of figuring out a base #parents array for formElement() to work from into it's own method.
* Breaks up the task of coming up with a hash for an $ief_id into it's own method (handy for debugging, where looking at sha1 hashes is not helpful at all).
* Trims back irrelevant values from the composition of an $ief_id (also easier for debugging).
* Set's the 'add' and 'edit' forms up with a correct delta. The previous patch did not work correctly for setting a delta in the 'edit' form case, b/c it was always setting the delta to count($entities). Existing entities needed to use their own established delta.
* Updates tests since the #parents array structure changed. This was the most annoying part.

I probably should add more tests to demonstrate the break in my last comment. I'm just really happy I got this to work so far.

Well done!

Patch worked for me.

Not putting it RTBC right away but this should be on the right path !

I was testing this over the weekend. I'm marking this RTBC, @m4olivei awesome work, hope you're well my friend!

The only nitpick I could possibly see would be:

@@ -656,16 +664,18 @@ class ComplexWidgetWebTest extends InlineEntityFormTestBase {
    *  The text to enter into the existing node text field.
    * @param $expected_error
    *  The error message that is expected to be shown.
+   * @param $delta
+   *  The delta of the multi node to be edited.
    */

We may want a @param int $delta - but that seems really minor!

If people don't feel that is required, I'd call this RTBC, thoughts?

The patch file in comment #69 doesn't work for me. My issue with inline entity form is impossible to add new field collection item of block type field in content type. It's also impossible to delete the last item of field collection.

@tamnv - That sounds like something that might be coming from Field Collection itself—whenever I see the words "Field Collection" near "IEF" near anything else dealing with different entities-inside-entities... there be dragons!

We're going to test this on our project too, because we found we were having the exact issue outlined in #2838635: Nested IEF filled automaticaly., and this looks like it may solve that nicely.

The patch provided #69 works for me. Therefore marking this RTBC.

The issue brought up in #75 sounds like a separate issue.

Patch not good.

Problem:
After deleting first reference and create similar - reference fields are prefilled.

@divined - I can't reproduce the problem you're describing after applying the patch—see the attached gif/screencast:

Still RTBC, imo—but if you can reproduce that error, can you give the exact steps outlined? I've tested now the following two scenarios:

1. Add new nested IEF and confirm it's not automatically added in multiple IEF forms during new content creation.

1. New node
2. Add new IEF item
3. Add new IEF item inside first IEF item
4. Save IEF items (nested and outer).
5. Add another new IEF item
6. Pass: item doesn't have the first nested item prepopulated

2. Edit existing content and confirm IEF still working correctly.

1. Edit node
2. Add new IEF item
3. Pass: item doesn't have the first nested item prepopulated

I was also jumping around, editing and rearranging all the items within each of my test nodes and with this patch, everything works correctly in all test scenarios I can come up with...

One thing to note here. This patch apparently changes the way the form field's name attribute. Which if you had written tests to walk through the form's dom structure using the form field name as a selector *cough*Lightning*cough* you will have issues.

@geerlingguy,

1. create parent entity with ONE reference and save
2. edit parent entity and remove this reference without saving
3. try to add similar reference again - inner reference will be pre-filled as in removed previously

@divined - I still can't reproduce the issue you mention following the steps you outlined after applying this patch...

(Apologies for the somewhat large Gifs).

Same issue here, but rather than having nested complex widgets, I have a simple ief widget nested within a complex ief widget.

Applying the solution in #69 to the InlineEntityFormSimple class ( and refactoring so common functions are moved to InlineEntityFormBase ) solves the issue for me.

Thanks #69!

The patch #83 work for me, this issue need to be merged.

What a great patch, nothing beats isolating an issue and then finding a patch that sorts it.

#83 seems to have sorted it out for me, attached is an interdiff between #69 and #83

Oh, and @geerlingguy, sorry for the off-topic, but how do you create those nice GIFs?

@hanoii - http://www.cockos.com/licecap/ — it's an awesome little cross-platform gif screenshot generator.

Just adding a ++ to this patch, without it, adding orders via the backend in Commerce 2.x is a hot mess since it's IEF's all the way down. With patch though everything works as expected.

Hi,

#83 worked for me.

The patch in #83 solved the issue i was having with this as well. Would love for this to be moved into an actual release if possible.

Patch applies to current HEAD and works as expected. +1 to RTBC and get it committed.

This issue has a long history.

re-queued #83 because it fails on the current development release (20 Apr 2018 at 12:43 UTC)

Looks like it's a simple fix in the constructor of InlineEntityFormComplex, but not 100% sure on this since the patch is rather to completely roll by hand.

Suspicion kind of confirmed

{"buildLabel":"Patch Failed to Apply","buildDetails":"error: patch failed: src\/Plugin\/Field\/FieldWidget\/InlineEntityFormComplex.php:275\nerror: src\/Plugin\/Field\/FieldWidget\/InlineEntityFormComplex.php: patch does not apply\nerror: patch failed: src\/Tests\/ComplexWidgetWebTest.php:228\nerror: src\/Tests\/ComplexWidgetWebTest.php: patch does not apply"}

Now to figure out the 'what' part of the patch.

Uploading the patch from #83 without the tests, just to see if that really is the issue. I'm not sure if I have the time to fix the tests and feel a little insecure in actually fixing them.

Updating patch from Rene Bakx. No tests.

Updated tests.

Updated with some code standards improvements in the new stuff and more importantly, adjusted an extra test that was created since #83.

Thanks ! It worked for me.

Nice! works like a charm!

#100 FTW!

Without the patch, I still got the issue.

With the patch I can create new entities without the paragraphs being copied in the new entity using the IEF complex.

Still need an issue summary update.

@jonathanshaw Ready to be committed now?

I can confirm the #100 is working on my project, great job!

I don't understand this issue well, but was able to make an attempt at a full IS based on @manspams comments.

The #100 is working when you add more item but when you modify the item added, it's still keep the error.
Failed on PHP 7.0, MySQL 5.7.23, D8.3.9.
Video report IEF bug

My previous comment (#110) is right with IEF version 8.x-1.0-beta1.
But is not right when I upgraded to version 8.x-1.0-rc1 and applied path from #100.
So I confirm the #100 is working for IEF version 8.x-1.0-rc1.

Hello,
The #100 patch applied correctly and working as expected.
+1 for RTBC.
Tested on dev branch.

The patch #2 (multivalue-complex-and-simple-ief-fix-2653574-83-no-tests-2) worked for me, thanks.

patch #100 worked for me.

Thanks a lot! Made my day

Thanks...!!! the path #100 worked for me.

I ended up here when working with Entity Reference with Layout and Paragraphs with a paragraph type that uses inline_entity_form_complex widget.

I can confirm #100 fixes the original problem. Thanks to all who contributed! 👏

AFAICS, #100 adjusts the existing tests to make them pass with a new change. I assume we would need additional test coverage and a test only patch similar to #28 in order to commit this? On the other hand, since this is a major issue for years confirmed by many people could we perhaps add additional tests in a follow-up?

Per @joachim's comment in #2576445: Inline Entity Form stable release plan #20, this RTBC issue is blocked until #2974544: Convert tests from Simpletest to FunctionalJavascript is fixed.

+1 for RTBC - #100 worked! Thanks for the hard work.

Using version 8.x-1.0-rc2 is working for me too.

Thanks for the patch!

@UriDrupal
Does the patch work for 8.x-1.0-rc2, or has the patch already been integrated into the rc2?

@UriDrupal
Does the patch work for 8.x-1.0-rc2, or has the patch already been integrated into the rc2?

#2974544: Convert tests from Simpletest to FunctionalJavascript is in, this is unblocked. Unfortunately the tests need to be revised due to the tests all moving and being updated. Adding the 'Needs tests' tag until the tests are updated. Anyone up for the challenge?

Lets see what breaks if we apply a reroll of patch #100 without any changes in the new tests.

(This is bound to end in tears, but will give an indication on what to do where)

Lets see if I understand what needs to be altered.
ComplexSimpleWidgetTest was chosen as victim for this test.

Right, I think I've got the gist of it.

Will try to fix remaining failures and will add an interdiff with #100 from now on.

Quick check if my JS-test setup is crazy, or if I am.

Ploughing on, I need to see what TestBot thinks every once in a while, sorry for the noise.

Almost there.
Will finish this tomorrow.

Sanity check against latest 8.x-1.x-dev

This _should_ be all green...

No stuff in the actual non-test code was changed, except for the multi-line breaking of a long array.

Removed "Needs Tests"-tag, unassign and setting status to "Needs Review"

@Spokje: So valuable you help drive that major issue home.

If i see it right, the patch itself was RTBC'ed several times since #102. Now tests have been rerolled for 9.x fitness.

@Spokje: Can you do the following (which is a usual pattern):
* In one comment, upload first the test-only as patch
* ...then the fix-plus-test patch
* ...and set needs-review

When maintainers see test-only red we know that tests "work". (Having fix-plus-test second prevents the infrastructure from setting this needs-work.)

So if test-only is red and test-plus-fix is green i see this RTBC finally.

Thanks! Glad when we have this in!

@geek-merlin Sure, here you go.

(I opted not to do this initially since the test-only will _always_ fail, since this patch is fundamentally changing the structure of the form-array, but you're absolutely right: This is the way it should be done)

OK so this is red/green as it should.

> I opted not to do this initially since the test-only will _always_ fail, since this patch is fundamentally changing the structure of the form-array

Ah yes OK.

Whe looking over the code, this removal of sub-arrays concerns me as it may lead to key clashes. Found no comment in the code why this is safe, and we quite some hands on this which does not feel assuring too (i did not grok the issue comment by comment tbh). Any ideas about this?

OK, now i spent some time with the patch.
Notes:
* Interesting patches in #19, #31, #69.
* Appreciated comment #14, #71

All in all, we have a quite convoluted codebase which contains a lot of magic that is often not clear what purpose if any it serves, which makes it verrry hard to maintain.
So all in all, we must not add complexity and weed complexity out whereever we can.
And document what is there. Which means:

1. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormBase.php
   @@ -564,4 +564,73 @@ abstract class InlineEntityFormBase extends WidgetBase implements ContainerFacto
   +  protected function iefIdFromParents(array $parents) {
   +    return sha1(implode('-', $parents));
   

   While i'm all for factoring this out, this is unrelated here and should be done in a separate commit.
   So please remove.
   
   EDIT: Maybe, maybe not. Wrote too quickly.

   (MEMO: Later add @todo This hashing is only used as key in form state, so looks like obsolete magic to be removed.)

   EDIT: Maybe we should remove the sha1-hashing as part of fixing this. As noted in a previous comment, if this does not break anything, it simplifies debugging a lot.

2. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormBase.php
   @@ -564,4 +564,73 @@ abstract class InlineEntityFormBase extends WidgetBase implements ContainerFacto
   +  protected function iefIdParents($field_name, array $parents) {
   +    if (empty($parents)) {
   

   Same as above: While i'm all for factoring this out, this is unrelated here and should be done in a separate commit.
   So please remove.
   
   EDIT: Maybe, maybe not. Wrote too quickly.

   (MEMO: Later Add @todo Why do we need #parents diverging from the form structure in the first place? Let's document or get rid of this.)

3. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormBase.php
   @@ -564,4 +564,73 @@ abstract class InlineEntityFormBase extends WidgetBase implements ContainerFacto
   +  protected function trimIefIdParents(array $parents) {
   +    // Remove 'form' and 'inline_entity_form' values in the array, as they are
   

   This magic may be added with best intentions (#39: "easier to debug"), but adds unnecessary complexity.
   Also and even more important, this is unrelated and requires changes to the tests that obscures what happens here.
   So please remove.

4. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
   @@ -307,7 +304,13 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
   -              array_merge($parents, ['inline_entity_form', 'entities', $key, 'form']),
   +              array_merge($parents, [
   +                $key,
   +                'inline_entity_form',
   +                'entities',
   +                $key,
   +                'form',
   +              ]),
                  $value['form'] == 'edit' ? $entity : $entity->createDuplicate()
   

   Changing code style here is unrelated.

   Adding $key twice to the parents should not be necessary here.
   Please fix or explain.
   (I suppose this is the most interesting change needed.)
   (And in any subsequent changes, please document whatever you find in comments.)

   EDIT: It looks as the parents structure of the complex widget needs fixes.

5. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
   @@ -324,7 +327,7 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
   -            '#parents' => array_merge($parents, ['entities', $key, 'form']),
   +            '#parents' => array_merge($parents, [$key, 'entities', $key, 'form']),
                // Store the entity on the form, later modified in the controller.
   

   Same as above.

6. +++ b/tests/src/FunctionalJavascript/ComplexSimpleWidgetTest.php
   @@ -67,8 +67,8 @@ class ComplexSimpleWidgetTest extends InlineEntityFormTestBase {
   -        $outer_title_field = 'ief_complex_outer[form][inline_entity_form][title][0][value]';
   -        $inner_title_field = 'ief_complex_outer[form][inline_entity_form][single][0][inline_entity_form][title][0][value]';
   +        $outer_title_field = 'ief_complex_outer[0][title][0][value]';
   +        $inner_title_field = 'ief_complex_outer[0][single][0][inline_entity_form][title][0][value]';
            if (!$outer_required_option) {
   

   OMG these selector changes surely made an awful lot of work. What a pity that the review had not been done earlier.
   But it looks like many of the changes will not be obsoleted and we get a clean diff of the test changes of this fix.
   So please adjust after the other changes.

   EDIT: Ah i read how this came.

Working a bit on this.

Played a bit with this... And read through the tests line by line. I don't see a test that is specific to this (it should contain an assertEmpty()).

That's a quite challenging issue, man.
* We first need a failing test that passes with the fix
* On the way we need (at least for complex widget) some adjustments of parents (see #71)
* That in turn will change the selectors for the tests

One approach i see for this is in the test class to add a helper method for the selectors, which will make the parents structure adjustments a one line patch and help with further fixes.

@geek-merlin Thanks for your work on this.

Just to make this clear: I have not written any of the patch, I just adjusted the tests to work with the "magic", that's indeed (at very least) interesting...

OMG these selector changes surely made an awful lot of work. What a pity that the review had not been done earlier.

I fully agree with that!

That's a quite challenging issue, man.
* We first need a failing test that passes with the fix
* [..,]

I fully agree, as you said in your review, it looks like there isn't a test (yet) that actually does the "Steps to reproduce" as described in the description of this issue. So we don't have the rudimentary basics in place to get this one across the line in a way that makes sense.

Let me see if I can get that rudimentary test in first, before somebody has a look at your (very much appreciated) review notes in #137

OK in the meantime i did some debugging and found a 4-liner patch that makes sense code-wise and fixes the issue for me. I'm far from groking all code paths so let's be sceptical and let some more people try break this.

@geek-merlin I like the simple elegance of your patch and for the few code-paths I've tried it works.

Back to the tests: Basically _any_ test for this issue will be void for the fix without changing any of the explicitly coded input-field IDs.
($third_title_field = 'test_ref_nested1[form][inline_entity_form][test_ref_nested2][form][inline_entity_form][title][0][value]'; as a nice example there).
Which would make it kind of a "self-fulfilling prophecy" in my eyes: You change to test to match the output of the fix in the patch.

Wouldn't it be wiser/better/something to make the current tests a bit more HTML-ID agnostic?
So instead of the above ID, something like a xpath selector for the third text input field?
//input[contains(@class, 'js-text-full text-full form-text')])[3])

If we tackle that first and make sure the current tests pass, and then add a test for this issue (I have one of them ready, I need to go through all the possibilities), we won't have to change the tests since the indexes on the HTML form changed.
Since every patch that will (attempt to) fix this issue will inevitably change the form-indexes in some way IMHO.

Yes, it's a bit(?) of extra work (and will postpone this issue for a bit more), but I personally think it's worth it and I'm willing to take that on.

Your input (as always) is much appreciated on this. Don't want to do that work when it's not needed/wanted/doesn't make any sense whatsoever.

> Wouldn't it be wiser/better/something to make the current tests a bit more HTML-ID agnostic?

Yes i agree that's the only thing to make the module maintainable.
Moved this to #3103609: Make tests (more) HTML ID agnostic and postponing this.
Thanks a lot you help with this!

To make it explicit, that commit was by mistake and reverted. This is still postponed on test refactoring.

Now that #3069423: Replace weak hashing algorithms is in, this may need a rebase.

Moving back to needs review because #3069423: Replace weak hashing algorithms has been committed.

@joelpittet I think this one is still postponed until (the now RTBC-ed) #3103609 is in and tests can be made for the patch.
(See Comment by geek-merlin here)

Thanks for summoning me, so i woke up and finally committed that one! And yes, needs test refactoring.

So I think I finally got a test showing that geek-merlin's patch from #141 is indeed as good as it looks.
Let's see if TestBot agrees.

test_only-2653574.patch is the test that does what is so thoroughly described in the Issue Description and should fail.
inline_entity_form-2653574-151.patch is Axel's patch combined with the above test and should pass without failure.

Right....

So the patch passes the new test, but fails a lot of others.
That's what you get for only locally testing the new test with the patch :/

Let's see if I can fix this all pronto!

test_only_should_fail-2653574.patch now should have no Coding Standards issue for the Doc comment short description.

The extra warning in D9.0:

1) Drupal\Tests\inline_entity_form\FunctionalJavascript\SimpleWidgetTest::testSimpleCardinalityOptions
Using assertContains() with string haystacks is deprecated and will not be supported in PHPUnit 9. Refactor your test to use assertStringContainsString() or assertStringContainsStringIgnoringCase() instead.

is unrelated to this patch and deserves its own issue IMHO.

So the failures on the patch with the new test are all coming from lines containing:

$assert_session->waitForField('multi[form][entity_id]')

So we're basically back to making the tests more HTML ID agnostic :/

We need to refactor that line into something that doesn't directly reference exact HTML IDs in a separate issue.
After that we should be fine with the current patch, but we really shouldn't do that in this issue. Changing two things in one issue is a self-fulfilling prophecy IMHO (Sadly we've been there before: #142)

I'll create a new issue and get cracking on it straight away.
Putting this issue on Postponed once more.

Turns out the patch from #151 changes 'multi[form][entity_id] into multi[form][0][entity_id] breaking a lot of tests.

I'm however sure the patch does solve the issue, since it passes the test made for this issue.

@geek-merlin: Could you have a look at Make Tests 'multi[form][entity_id]' agnostic? As soon as that one is in, we need to reroll this patch and I'm pretty sure it will (finally) pass and can be (again finally) committed.

@Spokje Excellent work, rock on!

Reroll of #158

@geek-merlin

The patch in Issue 3129703 should fix the test-failures in class Drupal\Tests\inline_entity_form\FunctionalJavascript\ComplexWidgetRevisionsTest.

That leaves us with 2 more Test-classes to fix.
Oh boy, we were seriously bitten in the behind by the way all the test are very much riddled with very specific HTML-IDs.
And I was seriously wrong in thinking we didn't have to replace them all with more ID-agnostic tests.

Anyway: I'm planning to get this issue in to Green TestBot Land during this weekend. It isn't that much work, but running Nightwatch tests locally just takes a looong time, even on a beefy machine.

The patch in Issue Make ComplexSimpleWidgetTest HTML ID agnostic should fix the test-failures in class Drupal\Tests\inline_entity_form\FunctionalJavascript\ComplexSimpleWidgetTest.

OK committed the test improvements and re-queued.

@geek-merlin Thanks Axel!

So Make ComplexWidgetTest HTML ID agnostic should fix the last remaining test failure.

After committing that one, I don't think a reroll of this one is needed, a simple retest should pass.
#FamousLastWords

Fixing coding standard issue.

Well, that was "fun". but I think we've finally got green TestBot on all the tests.

Great work!

+++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
@@ -526,6 +526,7 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
+      $new_key = count($entities);

Alas, inspired by #3129760-7: Compatibility with multiple_fields_remove_button, i realize that my below code assumes that $entities is a numeric array with no "holes", and i don't grok that 380-lines monster method well enough to ensure that.

Let's use some more robust code like (untested):

    // Make a numeric key bigger than all existing ones.
    $new_key = $entities ? max(array_keys($entities))+1 : 0;

which should do the same in sane cases but not break on "holes".

Let's see how this behaves.

@geek-merlin Like the approach, it did however, introduce 2 new coding standards violations (spaces around the plus sign).

Besides that: Good enough for TestBot = Good enough for me.
However, since I've contributed patches, I can't put it on RTBC any more.

> Good enough for TestBot = Good enough for me.

Yes tests show this fixes (at least one instance of) this bug so let's get this in.

> coding standards violations (spaces around the plus sign).

Thanks! Fixed on commit.

Added attributions, thank you all! 🎉🐳💪

Woohoo! Thanks @geek-merlin for ploughing through the massive amount of sub-issues before we got here in the end (And to all in this thread!)

EDIT: I think this is big enough to rectify a new release soon? (Our agency has this patch on 29 out of 36 supported D8-sites, where the only reason 7 don't have the patch is that inline_entity_form isn't installed on those)

Wooow! can't thank you guys enough for such an amazing work and dedication on this issue! Congrats!!!

Wooot: inline_entity_form 8.x-1.0-rc5 | Drupal.org - a.k.a. the @Spokje release ;-) (not forgetting the 40 other people involved in that release - 30 of which in this very issue 💪)

EDIT: replaced RC4 with RC5.

Ups i screwed something up in the last commit.

It looks like this may have broken the Drupal Comemrce tests due to the key rename

1) Drupal\Tests\commerce_order\FunctionalJavascript\OrderAdminTest::testCreateOrder
Behat\Mink\Exception\ElementNotFoundException: Form field with id|name|label|value|placeholder "order_items[form][inline_entity_form][purchased_entity][0][target_id]" not found.

It's now order_items[form][0][purchased_entity][0][target_id].

Testing in #3135949: Inline Entity Form rc4+ causing test regressions.

EDIT:

+++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
@@ -535,8 +538,8 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
             $parent_langcode,
-            NULL,
-            array_merge($parents, ['inline_entity_form'])
+            $new_key,
+            array_merge($parents, [$new_key])
           )

Yep, this change.

@mglaman: In #3129703: Make ComplexWidgetRevisionsTest HTML ID agnostic @Spokje made an excellent job to make tests robust, you might want to steal that.

This also broke some behat tests in our projects, for example:

    And I fill in "field_related_locations[form][inline_form_entity][title][0][value]" with "Test location"

became:

    And I fill in "field_related_locations[form][0][title][0][value]" with "Test location"

@geek-merlin I had a look at the issue you referenced but it's not clear to me how the xpath selectors would apply to a Behat test?

@mpp Make a custom step definition using the xpath selector instead of cutting the corner and putting the literal name from the current implementation into the step text.

The patch #83 worked for 1.0-beta1 version but this is only d8 compatible.
Please find the D9 compatible patch and this need to be merged : multivalue-complex-and-simple-ief-fix-with-d9-compatible-2653574_01.patch

The issue persists with the latest version of IEF 8.x-1.0-rc14 on Drupal 10.x To resolve the issue tried to apply the patch #184 unfortunately not applied.