ContentEntityStorageBase::deleteRevision() function does not remove node_field_revision entries

This is blocking #1863906: [PP-1] Replace content revision table with a view because the revisions view is using node_field_revision as base table:

This should expose the bug.

This is at least major then. Also would be nice to have generic tests, not just node specific. Also needs a title update, this method does not exist :)

Marked #2844537: Revision delete option showing fatal error as a duplicated of this issue.

Working on this for Global Sprint Weekend in Boston...

This patch fixes the issue. As for tests, there is still only the Node test from #4. I attempted to hack up some generic tests to EntityRevisionsTest.php without success (see second patch.txt file for my WIP).

That's a good start. You need to make sure to only execute that query if that table actually exists.

EntityRevisionsTest is a web test, I'd instead look at \Drupal\KernelTests\Core\Entity\EntityApiTest::assertCRUD(), everything is already in place there, you just need to add another conditional check for the revision_data_table table

We possibly also need an update function to clean up not-deleted rows.

Thanks @Berdir. Here is the updated patch.

We possibly also need an update function to clean up not-deleted rows.

Makes sense, but probably we can split the clean-up into another issue to get proper testing coverage.

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityApiTest.php
@@ -101,6 +101,9 @@ protected function assertCRUD($entity_type, UserInterface $user1) {
       $this->assertEqual(0, db_query('SELECT COUNT(*) FROM {' . $revision_table . '}')->fetchField(), 'Data table was emptied');
     }
+    if ($revision_data_table = $definition->getRevisionDataTable()) {
+      $this->assertEqual(0, db_query('SELECT COUNT(*) FROM {' . $revision_data_table . '}')->fetchField(), 'Data table was emptied');
+    }

I fear what I said isn't really correct.

This only tests deleting the entity, not just a revision. So we can keep this, but I'm pretty sure this will not actually fail in a test-only patch.

There don't seem to be generic deleteRevision() entity tests yet, which is bad.

Lets add a testDeleteRevision() method to \Drupal\KernelTests\Core\Entity\EntityRevisionTranslationTest, which seems pretty close to what we are doing. Create an entity_test_mulrev entity, add a second revision, verify you have two records in both revision tables. delete the first revision, verify you only have one record in the revision tables.

I'd also be OK with keeping the line you had in the node specific test, that's additional test coverage that doesn't cost us anything. Don't forget to provide a test-only patch then, so we can be sure that we are actually covering that bug now.

Thanks @Berdir. Here is the new patch and the test only patch.

+++ b/core/tests/Drupal/KernelTests/Core/Entity/RevisionableContentEntityBaseTest.php
@@ -18,39 +18,109 @@ class RevisionableContentEntityBaseTest extends KernelTestBase {
-    $entity->setRevisionLogMessage('This is my log message');
-    $entity->save();
+    $this->user = User::create(['name' => 'test name']);
+    $this->user->save();
+    $entity_type = 'entity_test_mul_revlog';
+    $this->definition = \Drupal::entityManager()->getDefinition($entity_type);
+    /** @var \Drupal\entity_test_revlog\Entity\EntityTestMulWithRevisionLog $entity */

looks like something with the indendation is wrong here, every line shows as changed.

I converted the local variables into properties. What you see as difference is the $this->

#21 sounds like an out-of-scope change, so we should probably not do that in this patch.

@berdir, @xjm I can write a new patch if you ask me, but first, please take some minutes to review what the changes do.

The original test RevisionableContentEntityBaseTest has a single method that was designed to test the creation of a single revision in an entity.

What #19 does is extend this test to be able to create multiple revisions of an entity and do some assertions in the process. By making definition, user and entity attributes of the test class, is easier to access them across the assertion methods. If we don't do this, we have to pass 3 parameters to the new methods. It didn't feel the right approach to me at the time to code the patch.

But again, if this breaks BC or still don't convince you please let me know and I will write another version of the patch.

@Berdir, @amateescu, @tstoeckler, @plach, and I discussed this issue and agreed that it is potentially critical because of the data integrity problem.

We also discussed the uprade upgrade path for deleting the orphaned data, and agreed that it should be done in the regular updates function because we can't use the API.

Discussed this on a triage call with @xjm @alexpott, @effulgentsia @cottser.

We agreed this is critical, for a couple of reasons:

1. The actual stale data by itself is extra data rather than lost data, so that in itself is less of a problem than other data loss issues. Also eventually a Drupal 8 to Drupal 9 or 8-8 migration would automatically clean it up.
2. However, entity queries against allRevisions() might (not verified) return rows for deleted revisions if they match the query, since the join will be on entity ID instead of revision ID.
3. Manual database audits to debug other issues could get very confusing if there are extra rows that shouldn't be there (hard to tell if they're extra rows, or if rows from other tables are missing after the fact for example).

We also agreed to split the upgrade path to a follow-up, since we can stop this from happening more quicker if we put the clean-up in another issue.

I need to open the upgrade issue still before removing that tag, but adding the triaged tag for now.

Follow up created here: #2869568: Add upgrade path to clean up deleted revisions from node_field_revision

Still not quite sure about the properties. IMHO, it would be more readable to pass entity and user to the save helper and the definition to the other. If it would be more code and methods, then I could see how this might be worth it.

Also, one nitpick:

+++ b/core/tests/Drupal/KernelTests/Core/Entity/RevisionableContentEntityBaseTest.php
@@ -18,39 +18,109 @@ class RevisionableContentEntityBaseTest extends KernelTestBase {
+   * @param int $count
+   */

Missing description.

Thanks @Berdir and @xjm. Here is the new patch without the properties introduced in #19.

Almost there.

+++ b/core/tests/Drupal/KernelTests/Core/Entity/RevisionableContentEntityBaseTest.php
@@ -23,34 +26,98 @@ class RevisionableContentEntityBaseTest extends KernelTestBase {
+   * @param Drupal\Core\Entity\EntityTypeInterface $definition
...
+   * @param Drupal\Core\Entity\EntityInterface $entity
...
+   * @param Drupal\user\UserInterface $user

needs a leading \

Also, https://www.drupal.org/pift-ci-job/647781 shows one coding standard issue.

Thanks @Berdir

Thanks, lets get this fixed :)

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -598,6 +598,12 @@ protected function doDeleteRevisionFieldItems(ContentEntityInterface $revision)
       ->condition($this->revisionKey, $revision->getRevisionId())
       ->execute();
     $this->deleteRevisionFromDedicatedTables($revision);
+
+    if ($this->revisionDataTable) {
+      $this->database->delete($this->revisionDataTable)
+        ->condition($this->revisionKey, $revision->getRevisionId())
+        ->execute();
+    }

I wonder if we should try to be consistent about the order the queries fire... it seems odd to do revision table, dedicated revision tables, and then revision table table. I would have though the dedicated tables would come last. See \Drupal\Core\Entity\Sql\SqlContentEntityStorage::doDeleteFieldItems().

Also I think the issue summary needs work to explain the resolution and the fact the update path is going to be fix in a follow-up.

Changed the order of execution to be consistent with SqlContentEntityStorage::doDeleteFieldItems.

Updated issue summary.

That's a good change, moving this back to RTBC. Issue summary update is brief but accurate.

Committed b8ed298 and pushed to 8.4.x. Thanks!

I credited @generalredneck for discovery the issue and everyone for contributions in either triaging the issue or review feedback points.

We should backport the fix to 8.3.x but it'll need a reroll.

The backport is not simple because the test using an entity type only 8.4.x

@alexpott: Would it be acceptable to skip the generic test for 8.3.x and only have the node specific test and EntityApiTest additions?

@Berdir - sure I think that that is acceptable - it's a lot of work for only a little gain in test coverage. And once 8.4.0 is out 8.3.x is obsolete so we'll great the generic test in the future. We should try to keep backports simple and not add to the divergence.

Actually I tried to do the backport so I had a branch lying around where doing this is simple...

Thanks!

Same patch as for 8.4 minus the additional test, so I think that's OK.

Committed 3e3eab6 and pushed to 8.3.x. Thanks!

Also agreed on skipping the extra test coverage here.