ContentEntityBase::__set() messes with values that happen to be TypedData

Patch look good to me but can't RTBC this is way over my head.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -802,25 +802,27 @@ public function &__get($name) {
   +    if ($this->hasField($name)) {
   

   I'm not sure on much this magic method is really called, but this adds at least two method calls to every invocation. Should we check this isn't adding a performance regression?

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -802,25 +802,27 @@ public function &__get($name) {
   +      if ($value instanceof TypedDataInterface && !$value instanceof EntityInterface) {
   

   Alternatively, could we just support down-casting objects of FieldItemListInterface ? I don't think we have to support something else, this just makes sure we can do:
   $node->title = $node2->title;

   I've no idea while there is the check for EntityInterface here in the first place.

If #7 is true, raising this priority to match that one.

So… not sure what's left to be done here? fago needs to review/RTBC?

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -802,25 +802,31 @@ public function &__get($name) {
   +      // If a FieldItemList object already exists, set its value.
   +      if (isset($this->fields[$name][$this->activeLangcode])) {
   +        $this->fields[$name][$this->activeLangcode]->setValue($value);
   +      }
   

   would putting this before the

   if (!isset($this->fieldDefinitions)) {
   		+ $this->getFieldDefinitions();
   		}
   

   result in any speed up? Would that mirror the existing code more and save the lookup?

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -802,25 +802,31 @@ public function &__get($name) {
        // The translations array is unset when cloning the entity object, we just
        // need to restore it.
        elseif ($name == 'translations') {
          $this->translations = $value;
        }
   

   out of scope but this could go first in the function and then we could exit early

+1 rtbc then

Per #10 & #13.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -780,7 +780,7 @@ public function &__get($name) {
   -    // Inline getFieldDefinition() to speed up things.
   +    // Inline getFieldDefinition() to speed things up.
   

   lol

2. +++ b/core/modules/system/src/Tests/Entity/EntityFieldTest.php
   @@ -264,17 +264,22 @@ protected function doTestReadWrite($entity_type) {
   +    $this->assertFalse($entity->name === $entity2->name, format_string('%entity_type: Copying properties results in a different field object.', array('%entity_type' => $entity_type)));
   ...
   +    $this->assertTrue($entity2->_foo === $entity->name, format_string('%entity_type: Copying properties results in a different field object.', array('%entity_type' => $entity_type)));
   

   Isn't the point here that it results in the the same field object?

+++ b/core/modules/system/src/Tests/Entity/EntityFieldTest.php
@@ -264,17 +264,22 @@ protected function doTestReadWrite($entity_type) {
+    $this->assertFalse($entity->name === $entity2->name, format_string('%entity_type: Copying properties results in a different field object.', array('%entity_type' => $entity_type)));
...
+    $this->assertTrue($entity2->_foo === $entity->name, format_string('%entity_type: Copying properties results in a different field object.', array('%entity_type' => $entity_type)));

Testing for difference using === is super weird to me.

Back to RTBC to get feedback from Alex RE: #18.

+++ b/core/modules/system/src/Tests/Entity/EntityFieldTest.php
@@ -264,17 +264,22 @@ protected function doTestReadWrite($entity_type) {
+    $this->assertTrue($entity2->_foo === $entity->name, format_string('%entity_type: Copying properties results in a different field object.', array('%entity_type' => $entity_type)));

I think what's puzzling about the line is that the message assertion is wrong. It's actually checking that a non-field property is not touched, thus is identical. Adjusted it.

if ($value instanceof TypedDataInterface && !$value instanceof EntityInterface) {

Whatever reason introduced this, it does not make sense any more as EntityInterface does not extend TypedDataInterface any more. Thus removed the needless double-check.

Else, patch looks good to me also.

ad #6.2: Yeah, this sounds like a separate issue to me as we should ensure it stays consistent across all levels.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 1420b77 and pushed to 8.0.x. Thanks!