Problem/Motivation

This issue is similar to #2772979: Enforcing a cloned entity translation to be new propagates to the original entity and #2828073: Cloning an entity with initialized translations leaves $entityKeys and $translatableEntityKeys pointing to the old entity object and the problem has been initially mentioned in #2828073: Cloning an entity with initialized translations leaves $entityKeys and $translatableEntityKeys pointing to the old entity object, but in order to keep each problem separately there is an isolated issue for the $values entity property.

What happens is that if an entity with at least one translation initialized is cloned then the $values property will be shared between the clone and the original entity and under circumstances the values of both entity will get mixed up. An example use is:

  1. Create an entity with at least one translation.
  2. Clone the entity or its translation.
  3. Alter a field value on the cloned entity object.
  4. Serialize the entity and the clone.
  5. Read out the value of the field that has been changed from the original entity.
  6. The returned value now is not the one from the original entity, but the altered one from the cloned entity object.

Proposed resolution

Break the reference of the property $values in CEB::__clone().

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hchonov created an issue. See original summary.

hchonov’s picture

hchonov’s picture

And here the fix for the cloning problem.

hchonov’s picture

Issue tags: +DevDaysSeville

The last submitted patch, 2: 2862463-2-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2862463-3.patch, failed testing.

hchonov’s picture

Status: Needs work » Needs review
FileSize
2.66 KB
2.63 KB

Ouch, the problem was that we later use the $values variable and so overwrite the reference.. We have to either unset the $values variable or use different name for it. I think unsetting it should be fine.

hchonov’s picture

FileSize
2.66 KB
417 bytes

Ouhh.. somehow I've uploaded the previous patch by mistake instead the interdiff.. Here again.

tstoeckler’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -1107,6 +1107,10 @@ public function __clone() {
+    $values = $this->values;
...
+    unset($values);
...
     foreach ($this->fields as $name => $values) {

Sorry, but in my opinion this is pretty confusing because $values actually refers to different things both times.

I'm not exactly sure what would be better naming, but I think we should avoid the unset(). Maybe foreach ($this->fields as $field) instead? Not sure...

hchonov’s picture

What about $fields_by_langcode?

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

I like it, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
jofitz’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.67 KB

Re-rolled.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, thank you!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 1ef413b to 8.4.x and cc1c6b6 to 8.3.x. Thanks!

  • alexpott committed 1ef413b on 8.4.x
    Issue #2862463 by hchonov, Jo Fitzgerald, tstoeckler: Cloning an entity...

  • alexpott committed cc1c6b6 on 8.3.x
    Issue #2862463 by hchonov, Jo Fitzgerald, tstoeckler: Cloning an entity...

Status: Fixed » Closed (fixed)

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