Problem/Motivation

The problem occurs through the API and the GUI.

Steps to reproduce:

  1. $nl = entity_load("node", 3)->addTranslation("nl");
  2. $nl->setNewRevision();
  3. $nl->save();

Now in hook_node_update $entity->getRevisionId() will be equal to $entity->original->getRevisionId() and both will point to the previous revision id.

This is critical, as you might have other entities referencing the revision id of the entity being updated and updated on revision id update in this hook, but as in this hook we do not propery receive the new revision id this might lead to data loss.

This is caused by the entity keys and translatable entity keys not being passed around be reference between translations, so changing it on one translation does update it on others.

Proposed resolution

Ensure that those arrays are stored by reference.

Remaining tasks

discuss

User interface changes

none

API changes

discuss

CommentFileSizeAuthor
#8 hook_entity_type_update-2542762-8-interdiff.txt586 bytessasanikolic
#8 hook_entity_type_update-2542762-8.patch2.53 KBsasanikolic
#6 entity-translation_keys-2542762-6.interdiff.txt1.55 KBplach
#6 entity-translation_keys-2542762-6.patch2.55 KBplach
#4 2542762-test_only.patch1.8 KBhchonov

Comments

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: +Contributed project blocker

This would block an 8.x port of CPS.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Can you clarify "used to work"? How long a go?

This could have been caused by #2478459: FieldItemInterface methods are only invoked for SQL storage and are inconsistent with hooks, or maybe also the *very* recently committed #2513094: ContentEntityBase::getTranslatedField and ContentEntityBase::__clone break field reference to parent entity.

But looking at the code, I can't really see why this would happen.

There is a strange inconsistency, there are two saveRevision() calls, one

$entity->{$this->revisionKey} = $this->saveRevision($entity);

and the other:
$record->{$this->revisionKey} = $this->saveRevision($entity);

But saveRevision() also sets that key, so that return shouldn't even be necessary?

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Assigned: Unassigned » plach

Working on a failing test for now.

hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented
Status: Active » Needs review
StatusFileSize
new 2542762-test_only.patch1.8 KB

@berdir:
I am not really sure when exactly it was working properly but for sure with beta 11 it was ok.
Unfortunatelly I am not able to debug at the moment and switch to previous betas to check when exactly the problem was introduced.

But here a test demonstrating the problem.

Status: Needs review » Needs work

The last submitted patch, 4: 2542762-test_only.patch, failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Assigned: plach » Unassigned
Status: Needs work » Needs review
StatusFileSize
new entity-translation_keys-2542762-6.patch2.55 KB
new entity-translation_keys-2542762-6.interdiff.txt1.55 KB

Let's try this...

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue summary: View changes
Status: Needs review » Needs work 
+++ b/core/modules/system/src/Tests/Entity/EntityRevisionTranslationTest.php
@@ -0,0 +1,60 @@
+  /**
+   * {@inheritdoc}
+   */
+  public static $modules = ['language', 'entity_test'];

Nitpick: entity test is already enabled in the parent test class.

Updated the issue summary to explain the fix. RTBC after that.

sasanikolic’s picture

sasanikolic commented
Status: Needs work » Needs review
StatusFileSize
new hook_entity_type_update-2542762-8.patch2.53 KB
new hook_entity_type_update-2542762-8-interdiff.txt586 bytes

Removed entity_test from the test and rerolled.

Status: Needs review » Needs work

The last submitted patch, 8: hook_entity_type_update-2542762-8.patch, failed testing.

hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented

@berdir:
I thought, that in each class extending we are overwriting the modules to be installed and not extending them?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

No, they are always extended.

hchonov queued 8: hook_entity_type_update-2542762-8.patch for re-testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Needs work » Needs review
xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Entity Field API
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

RTBC + 1

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Needs review » Reviewed & tested by the community

Actually, @Berdir already signed-off this in #7, as #8 addresses his only remark.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Nice test coverage. Committed 3532edb and pushed to 8.0.x. Thanks!

  • alexpott committed 3532edb on 8.0.x 
    Issue #2542762 by plach, sasanikolic, hchonov, Berdir:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.