EntityReferenceRevisions+Translation+ContentModeration in nested IEF loses Refs

Wanders off into "Let's create a failing test for this one"-land

@geek-merlin:

I think I've gotten all your requirements from the "Steps to reproduce" in a test.
Sadly that test doesn't fail (and yes, that's weird to read...)

This test (In my own words):

- Adds a Content Moderation Workflow on L1.
- Makes all fields of L1 translatable by users, except the entity reference fields from L1->L2 and L2->L3
- Enables revisioning of the entity reference fields from L1->L2 and L2->L3
- Checks that the site has more than 1 language enabled
- Create a L1 in which you create an L2 inline and in which you create a L3 inline, and save (as draft)
- Edit the L1, in which edit the L2, in which edit the L3 in which change the title. Hit Update on L3, Update on L2, and Save (as draft too)
- Again edit the L1, in which edit the L2, in which edit the L3 in which change the title. Hit Update on L3, Update on L2, and Save (as draft too)
- Checks revisions on L1, L2 and L3
- Checks entity reference between L2 and L3 on both edits by asserting the last entered title of L3 is still there.

Expected result according to "Steps to reproduce": Saved revisions on L1, L2 and L3, missing previously created entity reference between L2 and L3.

Result: Saved revisions on L1, L2 and L3, but entity reference between L2 and L3 on both edits is still there.

Now since you copy-pasted values in #6 I'm certain you've replicated it at some point.

Any clue what steps from your "Steps to reproduce" I've missed/misinterpreted/implemented completely wrong?

After talking to Merlin we've decided to re-use the "battle tested-and-proven" Drupal\Tests\inline_entity_form\FunctionalJavascript\ComplexWidgetRevisionsTest::testRevisionsAtDepth()0 for this, which will be fed a lot more possible combinations by the @dataProvider.

As a start I've integrated the test from my first attempt that was in a separate class.
As the mentioned test, this is also green.

@geek-merlin: See MR (New: Now with some (hopefully) explanatory comments)
Is this the way you've envisioned this?

IMHO next step would be to add Workflow and Translatable Fields a TRUE/FALSE option in the @dataProvider and alter the test accordingly.
This way we get a bigger matrix to see what combo breaks the camel's back IEF.

Hmmm,

That last testrun is with any permutation of steps 1 and 1a I can think of and will still not fail.

I will return to the original issue to see if I understand the reproduction steps there in a different way than the steps here.

Right, so I finally got a failing test for this issue.

The main difference between my last attempt: Do not alter L1 and L2, only L3 and stuff will break.

I somehow read step 3 of the "Steps to reproduce" as edit the titles of L1 and L2 as well... :/

Anyway: Revisions seem to have nothing to do with this bug.

Locally I tried running the test with and without revisions on L1, L2 and L3, the link L2->L3 still gets deleted on both cases.
Also tried the deepRevisiong test as in \Drupal\Tests\inline_entity_form\FunctionalJavascript\ComplexWidgetRevisionsTest, in both cases the link L2->L3 still gets deleted.

That's the reason there are no assertions on revisions any more in this new test.

Changed the Steps to reproduce in the IS accordingly.

There's something "weird" going on with the workflow on L1.

Somehow it makes L2 and L3 published on creation, although there's no workflow on those two Content Types and there's no transition in the workflow that automagically publishes content.

My gut-feeling says this has something to do with our problem. I will continue with that tomorrow morning.

EDIT: The above is _exactly_ the reason why I shouldn't attempt to think and do Security Updates at the same time.

Of course, nodes L2 and L3 have no workflow attached and thus follow their normal behaviour, which in this case is:

Publishing Options: Default options:

Published
Promoted to front page
Create new revision

So that's the "mystery" of L2 and L3 being published when created solved.

On the failed test result of MR26:

There was 1 error:

1) Drupal\Tests\inline_entity_form\FunctionalJavascript\ComplexWidgetRevisionsTest::testRevisionsAtDepth with data set "testRevisionsAtDepthAndContentModerationWorkflowAndTranslatableFields" (true, true, true)
Behat\Mink\Exception\ElementNotFoundException: Element matching xpath "(//input[@type='submit' and @value='Edit'])[1]" not found.

Now that _is_ unexpected, I would have predicted data set "testNoRevisionsAtDepthAndContentModerationWorkflowAndTranslatableFields" (false, true, true) to have also failed.

Looks like making revisions on the entity reference from L2->L3 _does_ make a difference in this case.

At least we have now established that both a Content Moderation Workflow and Translatable Fields (expect for the entity reference L1->L2) are needed, since non of the tests fail when only one of them is enabled.

Let's see what happens if I test for that in my test on MR29.

Ok,

So when we don't define a value for the revisioning on the entity_reference L2->L3 the test fails.

If we _do_ define a value for it, the test only fails when we set revisioning on the entity_reference L2->L3 to TRUE. If we set it to FALSE, the test passes.

One more attempt to get this drilled down further:

- Since we know both a Content Moderation Workflow _and_ translatable fields on L1 are needed, we can drop the permutations of those two from the test in MR26. (Which is good because adding another option would make this thing having a huge matrix and run forever)
- I like to see if (not) adding a revision on the Entity Reference from L1->L2 makes any difference.

Let change MR26 accordingly and see what happens.

Result of the last failed test on MR26:

There were 2 errors:

1) Drupal\Tests\inline_entity_form\FunctionalJavascript\ComplexWidgetRevisionsTest::testRevisionsAtDepth with data set "testNoRevisionsAtRootAndRevisionsAtDepth" (false, true)
Behat\Mink\Exception\ElementNotFoundException: Element matching xpath "(//input[@type='submit' and @value='Edit'])[1]" not found.

/var/www/html/vendor/behat/mink/src/WebAssert.php:418
/var/www/html/modules/contrib/inline_entity_form/tests/src/FunctionalJavascript/ComplexWidgetRevisionsTest.php:276

2) Drupal\Tests\inline_entity_form\FunctionalJavascript\ComplexWidgetRevisionsTest::testRevisionsAtDepth with data set "testRevisionsAtRootAndRevisionsAtDepth" (true, true)
Behat\Mink\Exception\ElementNotFoundException: Element matching xpath "(//input[@type='submit' and @value='Edit'])[1]" not found.

/var/www/html/vendor/behat/mink/src/WebAssert.php:418
/var/www/html/modules/contrib/inline_entity_form/tests/src/FunctionalJavascript/ComplexWidgetRevisionsTest.php:276

This shows to me that only the revisioning on L2->L3 is of importance, the L1->L2 revisioning doesn't matter.

Updating "Steps to Reproduce" accordingly.

As far as I can see now, MR29 is the most minimal failing test case.

Can manually recreate the problem without Inline Entity Form being installed, only the Entity Revision Reference module.

Creating a failing test in #3206499: Translation and moderation of another content type seems to make an ERR field behave different (and break IEF).

Whilst stripping this test down to the bare minimum, I've discovered that the condition "with a revision on the entity reference from L2->L3" isn't needed to reproduce this bug.

In fact: I'm pretty sure the code used for setting this revision isn't doing anything and a revision on an entity reference isn't possible.

I'll open up a follow-up issue to see if the ComplexWidgetRevisionsTest::testRevisionsAtDepth, where I got the code from, gets the same results without that piece of code trying to set this revision on a entity reference.

Updated "Steps to Reproduce" in the IS.

Ok, the above failing test is as far as I can get at this moment. Still unsure why and where it all goes wrong, but I'll try to put some observations down so Bigger Brains might jump aboard the ""Fix Me"-train.

Running testNestedToDoubleNestedLinkRetaining as is:

When this test runs, and after the save button is pressed on the Level 1 node, which has a nested Level 2 node, which has a nested Level 3 node:

1. Due to the bottom-up save strategy in IEF, the node at Level 3 gets saved first and gets an initial revision.
2. The node at Level 2 gets saved and gets an initial revision.
3. The node at Level 1 gets saved and gets an initial revision.
4. The ERR from Level 2 -> Level 3 gets saved and gets an initial revision.
5. This triggers both Drupal\entity_reference_revisions\Plugin\Field\FieldType::preSave and ::postSave
6. The ERR from Level 1 -> Level 2 gets saved and gets an initial revision.
7. This triggers both Drupal\entity_reference_revisions\Plugin\Field\FieldType::preSave and ::postSave
8. After the edit of the title of the node at Level 3, the node at Level 3 gets saved and gets a new revision.
9. The node at Level 2 gets saved first and gets NO new revision.
10. The node at Level 1 gets saved and gets a new revision.
11. The ERR from Level 1 -> Level 2 gets saved and gets a new revision.
12. This triggers both Drupal\entity_reference_revisions\Plugin\Field\FieldType::preSave and ::postSave

NOTE: Upon the resave the ERR from Level 2 -> Level 3 does NOT get resaved and thus gets NO new revision

If I only remove the extra language from the test:

1. The node at Level 1 gets 2 revisions (inital and a new revision).
2. The node at Level 1 gets 1 revisions (inital revision).
3. The node at Level 3 gets 2 revisions (inital and a new revision).

If I only remove the content moderation workflow from the test:

1. The node at Level 1 gets 3 revisions (inital and 2 new revisions).
2. The node at Level 1 gets 1 revisions (inital revision).
3. The node at Level 3 gets 3 revisions (inital and 2 new revisions).

Possibly related? #3088790: Creating a new translation draft is not populating node_field_data leading to inconsistent data display.

But this sounds like it needs a published node, which we afaicr don't.

We actually do, the node at Level 1 is under Content Moderation Workflow and stays unpublished, however the nodes at Level 2 and 3 are "normal" and get published when created and saved.

See also the MR review comment above. Starting to look into this test and trying to understand what's going on.

My knowledge of the inner workings of IEF are practically zero, I will need to look into that.

Based on some debug statements, the node__field_level2_level3 field table is empty after the second save. My assumption right now is that the reason is that \Drupal\entity_reference_revisions\Plugin\Field\FieldType\EntityReferenceRevisionsItem::isEmpty() decides the reference is empty due to either target id (or much more likely) target revision id being empty. Maybe the node is set in a state that doesn't have a revision id on the parent field and that confuses it. \Drupal\Core\Entity\RevisionableInterface::setNewRevision() is a bit weird in that it unsets the current revision id. But that's just a first guess, might be something else.

What would be helpful is to have a bit more information on the real-world use case that you have, what kind of references, entity types and moderation/translation configuration that you have. I have little experience with ERR for anything else than what I call composite entities which are exclusively used and edited within a single parent entity (meaning mostly just paragraphs). Did you use nodes in the test just to keep it simple or does that match your real site? Feel free to send me that privately through mail/slack or something.

In the ERR issue, you mentioned you have a way to reproduce it with just ERR too? What does that mean exactly, UI or kernel test, using what widget, if UI? there are existing issues in ERR about the provided default widgets not working properly either.

Yes, I'm now fairly certain that this is at the core of the problem, either an entity or just a target_id being set on that field. Actually more likely to be just the target_id, because the entity should still allow it to get past the isEmpty() call. Starting to look through IEF, I see that it attempts to fix that with that entity upgrader service that is called in \Drupal\inline_entity_form\WidgetSubmit::doSubmit(), the problem is that at this point, the empty values have already been filtered out and there's nothing left that it could upgrade.

I don't know yet why this only happens with translations/moderation, but looking into that now. An extra filterItems() call I suppose. Just as I wrote that, the debugger stopped at the place and I can confirm that it is \Drupal\Core\Entity\ContentEntityBase::hasTranslationChanges() that is calling the filter function, through \Drupal\Core\Entity\Plugin\Validation\Constraint\EntityUntranslatableFieldsConstraintValidator and the \Drupal\entity_reference_revisions\EntityReferenceRevisionsFieldItemList::hasAffectingChanges().

Adding the following to ERRItem::isEmpty() "fixes" it, but it's not the right fix. IMHO IEF must deal with ERR fields correctly and set a revision ID on them, even if it will later on update it to the correct value

if ($this->target_id !== NULL) {
  return FALSE;
}

Tracking down where and how exactly values are set on the field item, because I also see the needs_save flag pushed on there.

Ok, I think I have more or less the full picture now.

My first assumption was correct, the problem is that an entity object is set on the field that was marked to be saved as a new revision and no longer has a revision_id, then that updates the target_id and target_revision_id in onChange(), the second to NULL. That is however not immediately a problem, because isEmpty() is happy as long as there is at least an entity object.

The fun part is that eventuallly, that entity object is serialized, in which case it is simplified down to raw values by calling getValue(), then, because the entity is not new, we drop it. Later on, the translation validation stuff re-initializes the field values on a new field object, but now the only thing left is the target_id. filterItems() is called and then the value is gone.

Technically, this is IMHO a problem in IEF, setting an entity object without a current revision_id on an ERR field does not make much sense. But I also see that it would be pretty hard to change that within IEF. I'm testing with a solution where we patch ERRItem::getValue() to return the entity object value if set but no target_revision_id is set. Does not make sense on its own and only works when IEF then later on fixes the value by setting the saved entity on it again, but I can live with that change in ERR. Will post a patch later.

Posted a patch on #3206499: Translation and moderation of another content type seems to make an ERR field behave different (and break IEF). Let me know how manual testing of that goes. That should fix the test here, but see also the remarks above on how I'd recommend updating the assertions to work around the current, strange behaviors of the revision UI/affected revision flag.

First of all: Sorry for the (massive) delay on getting back to this issue. Life kinda happened there...

Second first of all: Thanks very much @berdir for lending your Big Brain!

I (and more importantly d.o. CI) can confirm the patch you've created on EER does indeed "solve" this issue, or, at the very least, makes the failing test now pass without any other changes to it.

I'll look at your comments now to see what I can to address those.