entity_translation_field_attach_delete_revision() ends up deleting current entity's record from {entity_translation}table

This is amazing!
i've been wrestling with this for a while, but couldn't pin down the culprit!

Thanks Will - I'll test this for sure!

Could you clarify what you mean by 'associated content' please?

Ah ok. I've updated the summary. I was wondering if it was some other module or something!

Seems to be working!

I was experiencing the same symptoms, when a revision was deleted, all entries for that content item would be cleared from the entity_translation table (using workbench_moderation).

Patch #4 fixed this problem for me too.

+++ b/includes/translation.handler.inc
@@ -573,6 +573,12 @@ class EntityTranslationDefaultHandler implements EntityTranslationHandlerInterfa
     $langcode = $translations->original;

This needs to be moved to the node translation controller.

Btw, did you try #1707156: Workbench Moderation integration? I suspect this is covered by that.

https://www.drupal.org/files/issues/entity_translation-2734295-4.patch
the above patch in combination with another one of Sylus's patches
https://www.drupal.org/files/issues/static_cache_for-2557429-17.patch
works well for us.
#2557429: Make workbench_moderation_node_edit_page_override() work with Entity Translation
these two patches work for us using workbench_moderation 7.x-3.x as they did when we were previously using workbench_moderation 7.x-1.x

there's quite a few issues open for workbench_moderation integration and it seems more than one way to fix it.

there's also a patch here:
https://www.drupal.org/node/2218133#comment-12176822 but I haven't confirmed this one as working but I did test it enough to confirm that it didn't degrade the behavior of our other two patches above. I was testing it because I saw the various issues but didn't realize that we were already patched with Sylus's patches.

***EDIT*** confirmed that sylus's fix is best for us.
the patch for workbench_moderation was modifying the system table weight and seemed very hackish, whereas Sylus's solution does not require a change to the workbench_moderation module. Confirmed as working with even the latest workbench_moderation 3.x as well as 1.x. ***EDIT***

Sylus, your code seems to be very effective even on latest versions of workbench_moderation , entity_translation and core. I had tried some other patches for this but have come back to yours because even though we're upgraded to workbench_moderation 7.x-3.x with the latest entity_translation dev branch, your patches are holding up and working for us. I will create a summary update after this is complete.

RTBC+ patch #4

Confirming patch #4 RTBC +1

@joseph.olstad: Any updates from you?

While this is labelled a 'hack' , it is a very effective one.
We were previously using this with workbench_moderation 1.x and earlier releases of entity_translation in combination with another patch for entity_translation , now we're using this patch with workbench_moderation 3.x and the latest release of entity_translation and it's working.

Here's our latest entity_translation PATCHES.txt list

#using August 8th build entity_translation dev 7.x-1.x dev branch  http://cgit.drupalcode.org/entity_translation/commit/?id=abdc9b0ab
-https://www.drupal.org/files/issues/entity_translation-pathauto_update-2743685-21.patch
-https://www.drupal.org/files/issues/et-bundle_language_with_test-2877074-7.patch
-https://www.drupal.org/files/issues/static_cache_for-2557429-17.patch
-https://www.drupal.org/files/issues/entity_translation-2734295-4.patch

so these issues:
#2743685-21: Pathauto update for all translations for a single node
#2877074-7: Refactor the entity_translation_language() callback to make it bundle-specific
#2557429-17: Make workbench_moderation_node_edit_page_override() work with Entity Translation
#2734295-4: entity_translation_field_attach_delete_revision() ends up deleting current entity's record from {entity_translation}table

the last one is THIS issue, patch #4
Thanks

The patch from #4 solved the problem, the problem is not however specific to workbench moderation, hence the issue's title change.

I am not sure why this was implemented in this way historically (see #1046282: Make the module work with revisions) and I cannot think of a case where:

... this behavior is correct in a non workbench model but is wrong with workbench.

as mentioned in the OP. My impression is that the main record(s) related to an entity translation's metadata, should not be removed from the {entity_translation} table, unless the entity itself is deleted. The case we are facing however has to do with removing one of the entity's revisions. Just feels unexpected. If anyone can give an example of where this is expected behaviour, I would be grateful.

I put together a small test that imitates the way the problem occurs, as described in the OP.
This test will fail at the moment, irrespective of whether Workbench Moderation is in play.
And I submit a patch for reviewing that uses a similar idea to the one from @sylus and delivers green tests.

I would appreciate any concrete feedback on both the internal logic of EntityTranslationDefaultHandler::saveTranslations() that deletes the records, as well as the patch.

The test itself is simply a POC and I would like to refactor it and extend it to cover more of the ET interplay with revisions.
It does serve the purpose of proving that the problem is reproducible and the patch fixes it.

Great job on the new tests. I haven't tried the patch yet but it seems that you've got a handle on it. I'll try testing this out tomorrow with a copy of the latest dev and the other patches we're still using (although one less patch now, I'll have to update our PATCHES.txt and entity_translation as one was just committed).

made these codesniffer suggested changes:
https://dispatcher.drupalci.org/job/drupal_d7/34208/artifact/jenkins-dru...

plus, while at it did all of the functions in that same test file.

The change makes sense to me, I believe the current logic of ::saveTranslation() was (incorrectly) assuming to be always dealing with the current revision.

1. +++ b/includes/translation.handler.inc
   @@ -572,10 +572,15 @@ class EntityTranslationDefaultHandler implements EntityTranslationHandlerInterfa
   +    // Update current values, unless we are deleting a revision,
   +    // in which case we don't need to update anything.
   +    $langcode = $translations->original;
   

   Comment doesn't wrap at column 80.

2. +++ b/includes/translation.handler.inc
   @@ -572,10 +572,15 @@ class EntityTranslationDefaultHandler implements EntityTranslationHandlerInterfa
   +    if (empty($hook[$langcode]['hook']) || $hook[$langcode]['hook'] != 'delete_revision') {
   

   Makes sense, my only doubt is whether the entity API allows to remove the current revision, and what happens in that case.

3. +++ b/includes/translation.handler.inc
   @@ -572,10 +572,15 @@ class EntityTranslationDefaultHandler implements EntityTranslationHandlerInterfa
   +    // Update revision values if the entity is isRevisionable.
   

   "is revisionable." :)

4. +++ b/tests/entity_translation.test
   @@ -826,3 +826,89 @@ class EntityTranslationIntegrationTestCase extends EntityTranslationTestCase {
   +    $this->assert($node_current_revision_id == $node->nid + 1, t('Correct vid attached to the node object.'));
   ...
   +    $this->assertTrue($result->rowCount() === 1, t('The entity_translation table contains the reference to the entity at hand.'));
   ...
   +    $this->assertTrue($entity_translation_node_record['revision_id'] === $node_current_revision_id, t('The reference to the entity at hand is keyed correctly by the current revision id.'));
   

   Can we use ::assertEqual()/::assertIdentical() here?

Btw, additional test coverage is welcome, but what we have here should be enough to cover the issue at hand, so removing the tag.

@joseph.olstad: Thanks for this, but I unfortunately need to do away with the changes from #24, need to focus on the current issue. The suggestions from codesniffer aren't always the right choice, for example, in the case of the public scope operator for methods. Btw, this is a better place to work with coding standards and 'grooming' the test file #2878191: Clean up entity_translation.test (standards, cosmetic), will be happy to continue that work there.

@plach: All easy points fixed, the difficult one (2) from #25 still needs to be answered. Will leave this to Needs work, till we get some good answer to that.

*EDIT*
see comment# 32 instead
#2734295-32: entity_translation_field_attach_delete_revision() ends up deleting current entity's record from {entity_translation}table
and/or
#2828635-9: Security + Bug Fixes for Workbench Moderation (3.0)
*EDIT*

#25 no comment for Plach feedback
#26 Stefanos patch, ran empirical real-world tests, your patch 26 in combination with workbench_moderation 7.x-3.x and this patch #2557429-28: Make workbench_moderation_node_edit_page_override() work with Entity Translation is resolving some issues we noticed on go-live day.

I've updated entity_translation to latest 7.x-1.x dev with >only one patch<
#2734295-26: entity_translation_field_attach_delete_revision() ends up deleting current entity's record from {entity_translation}table

and using workbench_moderation 7.x-3.x with 3 patches
#2428371-51: Upgrade from 1.3 to newer (1.4, 1.5, 3.x) with drush fails.
#1285090-40: Panels /node/%/edit forms
(this is the related solution) #2557429-28: Make workbench_moderation_node_edit_page_override() work with Entity Translation

Thanks Stefanos! You've made countless entity_translation improvements lately! I call for a 1.0 release party!

this is working great for us so far, I will report back if I find some opportunities for improvement otherwise going with it as-is.

Thanks

some reported production issues creeping in with our workbench_moderation 3.x , under some situation draft is vanishing, still too early to say what is happening just got wind of some recent reports from our client. I'm not sure how we didn't pick this up in our QA / Testing, and more surprised because the last release we put into production was about 3 weeks ago and the issue is being reported only today.

actually the draft vanishing issue was unrelated/misreported.

however, we did look closer and see an issue in our dev environment with a different patch strategy so we're going back to the sylus patches we were using.

I'm intending on trying the sylus patches with et beta7 and seeing how that works for us.

so, we're going to switch entity translation to beta7 with the sylus patches and one spelling fix patch:

#using entity_translation beta7 with these four patches:
-https://www.drupal.org/files/issues/spelling_mistake_2907275.patch
-https://www.drupal.org/files/issues/static_cache_for-2557429-17.patch #sylus patch good luck
-https://www.drupal.org/files/issues/entity_translation-2734295-4.patch #sylus patch good luck

workbench_moderation version = "7.x-3.0+5-dev" with these patches:
- https://www.drupal.org/files/issues/upgrade_from_1x3_to_3x_fails-2428371-51.patch
- https://www.drupal.org/files/issues/workbench_moderation-playnicewithpanels-40.patch

I'll try to followup and report back if there's any issues to report with this configuration.

Queuing patch 4 against entity_translation 7.x-1.0

patch 4 still being used in our recipe with beta7.
see recipe here:

#2828635: Security + Bug Fixes for Workbench Moderation (3.0)

if this passes, should be good for the same recipe with entity_translation 1.0.

Hello,

When testing patch #4 in my site, I don't understand how this can work :

 $query->condition('revision_id', $this->revisionId);

If I try to remove the revision_id, for example, 7200, when reaching this code I can see that $this->revisionId equals to the latest revision and not the revision that is being deleted.

Can anybody else confirm my findings (just doing a simple xdebug will do).
Thanks in advance.

Hi @unknownguy , this patch is used in combination with other contrib modules patched accordingly.
we're using patch 4 as follows:
#2828635-9: Security + Bug Fixes for Workbench Moderation (3.0)

lots of work has gone into making this all work together.
It is working well for us this way.

Ok I understand, it does not work for my site but I am using workbench_moderation-1.4, maybe is that the cause.

Thx anyway.

FYI: The latest patch (#26) is not a 100% complete following from the review in #25:
Point 2 there needs to be treated.

Hello,

I see two scenarios :

1) When ET revision support is enabled : if we delete an old revision, we should delete the corresponding row in the {entity_translation_revision} table, and we should not touch to the current translation stored in {entity_translation} table. On the other hand, if we delete the current revision, after removing the corresponding row in the ET_revision table, we should also remove the current row from the {entity_translation} table and replace it with the previous translation data (if available).

2) If ET revision support is disabled : when deleting an old revision, we should not do anything. On the other hand, when deleting the current revision, we should remove it also from the {entity_translation} table.

This is my logic but I am not an expert about ET, so I would be glad to hear other ideas.

Picking this up again:

Regarding Point 2. from #25:
I did a search in core's code and I could only find these two relevant pieces of information:

• A comment and an if condition in node_revision_delete() (l1299) that makes sure the current revision cannot be deleted.
• A comment inside the hook_field_storage_delete_revision() docblock (see field.api.php) that says "Deleting the current (most recently written) revision is not allowed as has undefined results". This is however not enforced when invoking such hook implementations, and seems to be only suggestive

Conclusion: My understanding is that deleting the current revision should only be possible when deleting the entire node.

@unknownguy:

> 1) When ET revision support is enabled : if we delete an old revision, we should delete the corresponding row in the {entity_translation_revision} table, and we should not touch to the current translation stored in {entity_translation} table.
This is exactly what is happening now

> 2) If ET revision support is disabled : when deleting an old revision, we should not do anything.
This is exactly what is happening now

====================================

So, here is the recap:

Problem:(from the OP)

This function when not being passed a revision as third parameter will delete all records from the entity_translation table matching the node nid. This behavior is correct in a non workbench model but is wrong with workbench.

Origins of the problem:
Calling saveTranslations() in the context of a 'delete_revision' hook from entity_translation_field_attach_delete_revision() causes an update inside the {entity_translation} table that deletes all entries for a given nid even if the action should be specific to the incoming vid.

Workbench Moderation:
This problem is not specific to WBM, which is what the provided tests try to demonstrate.

Solution:
Catch the special case of 'delete_revision' Entity translation hook invocations and avoid modifying the {entity_translation} table.

For the case of the current revision:
The current revision (and the whole node for that matter) will be both deleted as part of the same action; there is not logic in core that dictates we could (or should) delete the current revision while maintaining the node's data.

The provided patch is a rewrite of (#26) making sure we handle the special case of 'delete_revision' inside the doSaveTranslations() function.

patch 36 has tests, passes tests. (Great work on the tests Stefanos!)

Although I no longer have a complicated WBM setup to test this with.