Default revision flag doesn't propagate to all entity translation objects

Awesome, thank you!

Is there any way to confirm the problem and fix in UI?

Assigning by mistake, sorry.

Just thinking out aloud - but is there anyway we can add a definite test for cloning an entity and ensuring that the clone doesn't have any references back to the original object?

Given the size of the test changes to #11 I think it is worth getting another community review before committing.

1. It is really really great to see this generic test. hchonov++

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityCloneTest.php
   @@ -251,4 +251,82 @@ public function testFieldValuesAfterSerialize() {
   +    // Retrieve the entity properties.
   

   Retrieve the cloned entity's properties.

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityCloneTest.php
   @@ -251,4 +251,82 @@ public function testFieldValuesAfterSerialize() {
   +    $properties_filter = \ReflectionProperty::IS_PUBLIC | \ReflectionProperty::IS_PROTECTED | \ReflectionProperty::IS_PRIVATE;
   +    $properties_clone = $reflection->getProperties($properties_filter);
   

   Just a point of style. I wouldn't bother creating the $properties_filter it doesn't make the code any more readable and is not re-used. Also $properties_clone make it sound like a clone of the properties which this is not. It's the properties of the $clone entity. So I think $clone_properties makes more sense. I realise that these thought might be a language thing as the implications of different word orders is different in different languages. So it might be simpler just to call it $properties

4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityCloneTest.php
   @@ -251,4 +251,82 @@ public function testFieldValuesAfterSerialize() {
   +      $translation_unique_properties = ['activeLangcode', 'translationInitialize', 'fieldDefinitions', 'languages', 'langcodeKey', 'defaultLangcode', 'defaultLangcodeKey', 'validated', 'validationRequired', 'entityTypeId', 'typedData', 'cacheContexts', 'cacheTags', 'cacheMaxAge', '_serviceIds'];
   

   This can be set outside the loop no?

I think the test looks great. Just some minor notes:

1. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityCloneTest.php
   @@ -251,4 +251,81 @@ public function testFieldValuesAfterSerialize() {
   +    $properties = $reflection->getProperties(\ReflectionProperty::IS_PUBLIC | \ReflectionProperty::IS_PROTECTED | \ReflectionProperty::IS_PRIVATE);
   

   So per http://php.net/manual/de/class.reflectionproperty.php#reflectionproperty... this is equivalent to ~\ReflectionProperty::IS_STATIC, no? That would be both shorter and clearer IMO.

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityCloneTest.php
   @@ -251,4 +251,81 @@ public function testFieldValuesAfterSerialize() {
   +        $this->assertFalse($property->getValue($entity) === $property->getValue($translation), (string) new FormattableMarkup('Entity property %property_name is not cloned properly.', ['%property_name' => $property->getName()]));
   ...
   +        $this->assertTrue($property->getValue($entity) === $property->getValue($translation), (string) new FormattableMarkup('Entity property %property_name is not cloned properly.', ['%property_name' => $property->getName()]));
   

   Can we not instead test for the actual values? I.e.:

   $this->assertEquals('test-translation-cloning', $property->getValue($translation);
   

   etc. We could do an assertion on both the $entity and the $translation in both cases. What do you think?

Thank you. You know how much I like to nitpick, but as hard as I tried, I couldn't find anything ;-)

Committed and pushed ce2aae0 to 8.4.x and a2d2c19 to 8.3.x. Thanks!

Backported to 8.3.x as this change does not have BC implications and is a straight up bug fix.