Reverting to revisions prior to addition of field translations is broken

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The entity API allows changes in one revision for any number of languages at once. When a revision is deleted, it should not just delete one language and leave others around, that would leave the entity in an inconsistent state. The revisions data should be deleted wholesale without consideration for language AFAIS.

field_sql_storage_revisions.patch queued for re-testing.

I honestly don't see why would this fail installation :/

Because the patch still accesses some variables that don't exist anymore :)

The languages condition on the deletes can be dropped, $field_languages on the other hand still seems to be necessary.

Just re-rolling this to see if something breaks now. I think it makes sense to wait for #1497374: Switch from Field-based storage to Entity-based storage should be fairly easy to re-create, unlike the other issue.

It seems failures are only caused by the behavior change, hence I'd say the direction is good :)

I agree we should postpone this on #1497374: Switch from Field-based storage to Entity-based storage.

Yes. I think #1983554: Remove BC-mode from EntityNG already contains the changes here, can you confirm? If so, we can just use this to backport to 7.x once the other issue is resolved..

Needs work rather than active.

@catch:

I checked 8.0.x and we went exactly this way, so this issue should be fixed in D8.

From the test failures it looks like we need to check for $entity->revision (although that's not standardized anywhere) and use the old logic if that's not set.

IMO we should just update the test to remove any assumption on field language, the only problem is that the current solution implies a BC break. Could we use a variable and keep the current behavior on existing sites and enable the patch one only for new installations?

Spoke about this with @catch: we agreed a possible fix could be adding a revision_flag_key that would make the following code possible:

if (!empty($entity->{$revision_flag_key})) {
  // Skip field language fiddling.
}
else {
  // Keep things as now.
}

I'm not sure this is the same issue, see the patch over there.

Actually I think this is fixed in D8 by #2453153: Node revisions cannot be reverted per translation (maybe combined with #2465907: Node revision UI reverts multiple languages when only one language should be reverted).

reposting patch for testbot.

Following and added another related ticket: #2259279: field_sql_storage_field_storage_write does not properly update fields that have additional language values removed
Test-Fails seem odd. Try to re-run.

Ugh, I think this change definitely contradicts the last failing test in FieldSqlStorageTestCase::testFieldAttachSaveMissingData():

// Update: Field translation is missing but field is not empty. Translation
    // data should survive.

To work around this we should be a bit more specific in what to delete. How about the attached approach:
Base Table: Delete everything not related to the current revision OR revision languages.
Revision Table: Basically keep as is.

Any further thoughts on @das-peter's question in #31?

This has gained some importance due to Workbench Moderation's v7.x-3.0 release which uses Drafty which can run into this issue.

Hi,

Just wondering which patch should be applied? I am see #15 (with failed test) and then i see #31 is there.

Is it recommended to just do #31 or will both #15 and #31 need to be applied here?

+1 to make this RTBC for D7, this is quite important IMHO

+1 to RTBC and agree this is quite important. When dealing with environments with 24 languages and content moderation enabled, reverting to previous versions is quite common and this really affects the state of the database.
Tried the approach in #31 and works perfectly fine.

Confirming RTBC. Thank you!

31 works for us

FWIW patch in #31 works well for me.

I think this looks right, but it could really use more reviewers to look at the actual code changes and review them - since this touches a pretty fundamental part of the codebase.

Also, shouldn't we have tests for this? Seems like there are tests in the above Drupal 8 issues for this kind of functionality that could be partially backported (for example in NodeRevisionsTest::testRevisionTranslationRevert()), or if those aren't exactly right then at least a simple little test could be added to FieldSqlStorageTestCase (which has lots of tests already that involve passing different entity/language combinations in and seeing what gets written to the field tables). Then it will be more clear what the bug is and whether or not it's fully fixed.

I'm also moving this back to Major for now (since that's what's suggested in the issue summary). It's not good to have the database incorrect like this, but it's also not clear how it breaks the site in any kind of critical way.

RTBC+ patch #31

Confirming RTBC for patch #31

A quick status update:

• Several people have RTBC'd it.
• It needs tests.
• It needs an update to the issue summary to highlight the potential for data corruption.
• The approach needs signoff from a subsystem maintainer; who'd be the best person/people for this?

Bump.

Back to NW for tests then.

This was already fixed in D8.
For D7 I'm not entirely convinced that the test would belong here. Probably belongs with the entity_translation module since D7 core doesn't have the entity_translation module and this really belongs to that. This is still a core issue, and the patch is needed by entity_translation when using the drafty module.

Here's what the tests might look like (make more sense in entity_translation tests?) :

This is the D8 version of this test, anyone want to take a stab at backporting this test?

+++ b/core/modules/system/src/Tests/Entity/EntityRevisionTranslationTest.php
@@ -57,4 +57,39 @@ public function testNewRevisionAfterTranslation() {
     $this->assertTrue($this->reloadEntity($entity)->getRevisionId() > $old_rev_id, 'The entity from the storage has a newer revision id.');
   }

  /**
   * Tests if the translation object has the right revision id after new revision.
   */
  public function testRevertRevisionAfterTranslation() {
    $user = $this->createUser();
    $storage = $this->entityManager->getStorage('entity_test_mulrev');

    // Create a test entity.
    $entity = EntityTestMulRev::create([
      'name' => $this->randomString(),
      'user_id' => $user->id(),
      'language' => 'en',
    ]);
    $entity->save();
    $old_rev_id = $entity->getRevisionId();

    $translation = $entity->addTranslation('de');
    $translation->setNewRevision();
    $translation->save();

    $entity = $this->reloadEntity($entity);

    $this->assertTrue($entity->hasTranslation('de'));

    $entity = $storage->loadRevision($old_rev_id);

    $entity->setNewRevision();
    $entity->isDefaultRevision(TRUE);
    $entity->save();

    $entity = $this->reloadEntity($entity);

    $this->assertFalse($entity->hasTranslation('de'));
  }

 }

I say, RTBC this patch, commit it in, and add the test to entity_translation where it belongs.

I agree with #48 this should be RTBC and test in entity translation

Bumping to 7.61, this didn't make it into 7.60

Is our test coverage sufficient already for this?

Reset issue status.

Hello, stephencamilo, and welcome! Since Drupal 7 is still supported, setting these issues as won't fix is wrong, even if they are old issues.

This issue is almost 9 years old, it is marked as "critical" and has previously been set to a status of RTBC for the patch in comment #31 that passes automated tests. It was then reverted to "needs work" based on a question, "Is our test coverage sufficient already for this?"

I don't think the status should have been changed until the question was answered. And I think this issue should be on #3259739: [meta] Priorities for 2022-06-01 release of Drupal 7.

Changing status based on my previous comment #55.

Added to #3259739: [meta] Priorities for 2022-06-01 release of Drupal 7 with question "Are tests sufficient?"

On comment #41, David_Rothstein (one of the Drupal 7 maintainers) added the Needs tests tag. Since there haven't been patches after that, tests are still necessary.

Comment #48 provides a possible test that might be back ported and suggests it should be be in entity_translation.

Comment #48 says that, since in Drupal 8 the tests are written for the Entity Translation module, in Drupal 7 the tests should be written for the same module, which is a contributed one.
That's wrong, IMO: Since the patches in this issue are changing Drupal 7 core code, the tests need to be added to Drupal core, to avoid a regression and get the same issue again. There aren't tests for this case, or they would have caused test failures in the past already.