Problem/Motivation

When a entity revision is deleted, the data from {node_field_revision} is not deleted.

Proposed resolution

Remaining tasks

None.

User interface changes

None

API changes

None

Data model changes

None

Original report by generalredneck

It is my expectation that when I remove a revision that all information of that revision is removed... In this case, I've done the following

  $updated_revisions = $connection->select('node_revision', 'nr')
    ->fields('nr', array('nid', 'vid'))
    ->condition('nr.revision_timestamp', '1465595835', '>=')
    ->orderBy('vid')
    ->execute()
    ->fetchAll();
  $node_storage = \Drupal::entityTypeManager()->getStorage('node');
  $node_list = array();
  foreach ($updated_revisions as $row) {
    if (!in_array($row->nid, $node_list)) {
      $node_list[] = $row->nid;
    }
    $revision = $node_storage->loadRevision($row->vid);
    $revision->setNewRevision();
    $revision->save();
    $node_storage->deleteRevision($row->vid);
  }

After doing so I have the following

mysql> select nid, vid, langcode, title, uid, created, changed from node_field_revision where nid >= 60000;
+-------+--------+-----+-------------------------+-------+------------+------------+
| nid   | vid    | lan | title                   | uid   | created    | changed    |
+-------+--------+-----+-------------------------+-------+------------+------------+
| 60001 | 101045 | en  | LIVING IN NEW YORK CITY | 27552 | 1465835307 | 1465835429 |
| 60001 | 101046 | en  | LIVING IN NEW YORK CITY | 27552 | 1465835307 | 1465836518 |
| 60001 | 101047 | en  | Where to Live           | 27552 | 1465835307 | 1465836603 |
| 60001 | 200006 | en  | LIVING IN NEW YORK CITY | 27552 | 1465835307 | 1465835429 |
| 60001 | 200007 | en  | LIVING IN NEW YORK CITY | 27552 | 1465835307 | 1465836518 |
| 60001 | 200008 | en  | Where to Live           | 27552 | 1465835307 | 1466540679 |
| 60002 | 101048 | en  | Classroom               | 27552 | 1465836841 | 1465838185 |
| 60002 | 101060 | en  | Classroom               | 27552 | 1465836841 | 1466179518 |
| 60002 | 101061 | en  | Classroom               | 27552 | 1465836841 | 1466179704 |
| 60002 | 200009 | en  | Classroom               | 27552 | 1465836841 | 1465838185 |
| 60002 | 200021 | en  | Classroom               | 27552 | 1465836841 | 1466179518 |
| 60002 | 200022 | en  | Classroom               | 27552 | 1465836841 | 1466540679 |
| 60003 | 101049 | en  | Classroom               | 27552 | 1465836841 | 1465838208 |
| 60003 | 200010 | en  | Classroom               | 27552 | 1465836841 | 1466540679 |
| 60004 | 101050 | en  | Post a position         | 27552 | 1465838562 | 1465838869 |
| 60004 | 200011 | en  | Post a position         | 27552 | 1465838562 | 1466540679 |
| 60005 | 101051 | en  | /student/life           |     1 | 1465926342 | 1465926388 |
| 60005 | 101052 | en  | /student/life           |     1 | 1465926342 | 1465928313 |
| 60005 | 200012 | en  | /student/life           |     1 | 1465926342 | 1465926388 |
| 60005 | 200013 | en  | /student/life           |     1 | 1465926342 | 1466540679 |
| 60006 | 101053 | en  | /student/orgs           |     1 | 1465928403 | 1465928425 |
| 60006 | 200014 | en  | /student/orgs           |     1 | 1465928403 | 1466540679 |
| 60007 | 101054 | en  | /student/orgs           |     1 | 1465928403 | 1465928445 |
| 60007 | 101055 | en  | /student/orgs           |     1 | 1465928403 | 1465931587 |
| 60007 | 200015 | en  | /student/orgs           |     1 | 1465928403 | 1465928445 |
| 60007 | 200016 | en  | /student/orgs           |     1 | 1465928403 | 1466540679 |
| 60008 | 101056 | en  | VICTOR RODWIN RESEARCH  | 27552 | 1466001552 | 1466007126 |
| 60008 | 200017 | en  | VICTOR RODWIN RESEARCH  | 27552 | 1466001552 | 1466540679 |
| 60009 | 101057 | en  | WORLD CITIES PROJECT    | 27552 | 1466007157 | 1466007184 |
| 60009 | 200018 | en  | WORLD CITIES PROJECT    | 27552 | 1466007157 | 1466540679 |
+-------+--------+-----+-------------------------+-------+------------+------------+
30 rows in set (0.00 sec)
mysql> select * from node_revision where nid >= 60000;
+-------+--------+----------+--------------------+--------------+--------------+
| nid   | vid    | langcode | revision_timestamp | revision_uid | revision_log |
+-------+--------+----------+--------------------+--------------+--------------+
| 60001 | 200006 | en       |         1465835429 |        27552 | NULL         |
| 60001 | 200007 | en       |         1465836518 |            1 | NULL         |
| 60001 | 200008 | en       |         1465836603 |            1 | NULL         |
| 60002 | 200009 | en       |         1465838185 |        27552 | NULL         |
| 60002 | 200021 | en       |         1466179518 |         1077 | NULL         |
| 60002 | 200022 | en       |         1466179704 |         1077 | NULL         |
| 60003 | 200010 | en       |         1465838208 |        27552 | NULL         |
| 60004 | 200011 | en       |         1465838869 |        27552 | NULL         |
| 60005 | 200012 | en       |         1465926388 |            1 | NULL         |
| 60005 | 200013 | en       |         1465928313 |            1 | NULL         |
| 60006 | 200014 | en       |         1465928425 |            1 | NULL         |
| 60007 | 200015 | en       |         1465928445 |            1 | NULL         |
| 60007 | 200016 | en       |         1465931587 |            1 | NULL         |
| 60008 | 200017 | en       |         1466007126 |        27552 | NULL         |
| 60009 | 200018 | en       |         1466007184 |        27552 | NULL         |
+-------+--------+----------+--------------------+--------------+--------------+
15 rows in set (0.00 sec)

Note there are twice as many node_field_revision records.

Comments

generalredneck created an issue. See original summary.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dagmar’s picture

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

dagmar’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Active » Needs review
FileSize
1.08 KB

This should expose the bug.

Status: Needs review » Needs work

The last submitted patch, 4: 2753971-3-test-only.patch, failed testing.

Berdir’s picture

Priority: Normal » Major

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 :)

generalredneck’s picture

Title: NodeStorage::deleteRevision() function does not remove node_field_revision entries » ContentEntityStorageBase::deleteRevision() function does not remove node_field_revision entries

Updated the title with the technicality. Navigating to the NodeStorage Members function list clearly shows the correct parent class this lives in. See https://api.drupal.org/api/drupal/core%21modules%21node%21src%21NodeStor...

dagmar’s picture

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

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

hampercm’s picture

Assigned: Unassigned » hampercm

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

hampercm’s picture

Assigned: hampercm » Unassigned
Status: Needs work » Needs review
Issue tags: +SprintWeekend2017
FileSize
1.78 KB
5.01 KB

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).

Status: Needs review » Needs work

The last submitted patch, 11: 2753971-11.patch, failed testing.

Berdir’s picture

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

Berdir’s picture

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

dagmar’s picture

Status: Needs work » Needs review
FileSize
2.82 KB
1.6 KB

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.

Status: Needs review » Needs work

The last submitted patch, 15: 2753971-15.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Needs work
+++ 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.

dagmar’s picture

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

Berdir’s picture

+++ 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.

dagmar’s picture

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

The last submitted patch, 19: 2753971-19-test-only.patch, failed testing.

xjm’s picture

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

dagmar’s picture

@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.

xjm’s picture

Priority: Major » Critical

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

xjm’s picture

Issue tags: +Needs upgrade path

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.

catch’s picture

Issue tags: +Triaged D8 critical

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.

dagmar’s picture

Berdir’s picture

Status: Needs review » Needs work

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.

dagmar’s picture

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

Status: Needs review » Needs work

The last submitted patch, 30: 2753971-30-test-only.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Needs work

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.

dagmar’s picture

Status: Needs work » Needs review
FileSize
8.68 KB
1.88 KB

Thanks @Berdir

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, lets get this fixed :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update
+++ 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.

dagmar’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
8.73 KB
863 bytes

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

Updated issue summary.

catch’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

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

dagmar’s picture

Issue summary: View changes
alexpott’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

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.

  • alexpott committed b8ed298 on 8.4.x
    Issue #2753971 by dagmar, hampercm, Berdir, xjm, generalredneck, catch,...
alexpott’s picture

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

Berdir’s picture

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

alexpott’s picture

@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.

alexpott’s picture

Status: Patch (to be ported) » Needs review
FileSize
5.48 KB
3.23 KB

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

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

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

  • catch committed 3e3eab6 on 8.3.x
    Issue #2753971 by dagmar, alexpott, hampercm, Berdir, generalredneck,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

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

Also agreed on skipping the extra test coverage here.