New host revisions do not always create new composite entity revisions

Can we get a test-only patch?

Promoting.

As a side effect, you touch this area:

We discussed once that we are not properly doing the default revision handling now. But we also realised that we are not relying on the default revision until now.

Maintaining the default revision also really makes sense from a performance perspective. It is faster to load it as the entity cache works.
But the problem is, we currently always load by revision, which circumvents it as far as i know.

So as a result, when loading the paragraphs, we could check if the host isDefaultRevision and then load the default revision instead of a specific revision. This would get us the entity cache back.

Passing to Berdir to get more qualified feedback.

Yes, I think that makes sense.

We have a separate issue for that already, we can discuss that there: https://www.drupal.org/node/2673076

now that we no longer save too many new revisions, there shouldn't be a blocker for trying to do that. Or we can also just try to push the core issues that are trying add static and persistent caching for revision loading. That would be even better.

Committing. Plus updating the other issue.

People say, this issue commit broke Paragraphs.

If we revert this, we need another release to make Paragraphs pass again.

So i reverted. No release needed as the commit was after the last release.
We can discuss now how we can fix this cleanly.

Added an if/else for the parent::preSave.
The parent was also saving the entity, that is why there was an extra revision in paragraphs. With this patch, paragraphs tests don't fail.

We should have an explicit test for this here as well. We already have a composite test, we should be able to extend it so that the initial save doesn't result in more than one revision.

I don't think we have to do all in ERR, we are in this issue moving almost all the revision management to this module thus there might be errors in Paragraphs (which should work based on this revision management in ERR).

So we have to remove the revision management in paragraphs to rely on ERR only.

Added an assertion for the revisions count.

+++ b/err-new_revisions-2801321-5.patch
@@ -0,0 +1,26 @@
+diff --git a/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php b/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php
+index 8b25f4d..62a574f 100644
+--- a/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php
++++ b/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php
+@@ -248,7 +248,20 @@ class EntityReferenceRevisionsItem extends EntityReferenceItem implements Option
+    */
+   public function preSave() {
+     parent::preSave();
+-    if ($this->entity instanceof EntityNeedsSaveInterface && $this->entity->needsSave()) {

patch in patch, don't forget to remove that.

+++ b/src/Plugin/Field/FieldType/EntityReferenceRevisionsItem.php
@@ -247,9 +247,25 @@ class EntityReferenceRevisionsItem extends EntityReferenceItem implements Option
+    if ($this->hasNewEntity()) {
+      parent::preSave();
...
+    else {

parent::preSave should always be called except clearly expressed..
Berdir pointed out that hasNewEntity() should be called before though.

I changed, passing local. Committing.

I appreciate the original motivation behind this issue, but I think there are also good reasons to allow composite entities to not have a new revision generated when they have not been edited. I opened #3267490: Allow composite entities to opt out of creating duplicate revisions to explore if and how we want to support that.