Issue #2182239 by Berdir, andypost: Improve ContentEntityBase::id() for better DX.

core/lib/Drupal/Core/Entity/ContentEntityBase.php

@@ -34,13 +34,6 @@ abstract class ContentEntityBase extends Entity implements \IteratorAggregate, C
    */
   const TRANSLATION_CREATED = 2;
 
-  /**
-   * Local cache holding the value of the bundle field.
-   *
-   * @var string
-   */
-  protected $bundle;
-
   /**
    * The plain data values of the contained fields.
    *
@@ -128,21 +121,44 @@ abstract class ContentEntityBase extends Entity implements \IteratorAggregate, C
    */
   protected $isDefaultRevision = TRUE;
 
+  /**
+   * Holds entity keys like the ID, bundle and revision ID.
+   *
+   * @var array
+   */
+  protected $entityKeys = array();
+
+  /**
+   * The instantiated entity data definition.
+   *
+   * @var \Drupal\Core\Entity\TypedData\EntityDataDefinition
+   */
+  protected $dataDefinition;
+
   /**
    * Overrides Entity::__construct().
    */
   public function __construct(array $values, $entity_type, $bundle = FALSE, $translations = array()) {
     $this->entityTypeId = $entity_type;
-    $this->bundle = $bundle ? $bundle : $this->entityTypeId;
+    $this->entityKeys['bundle'] = $bundle ? $bundle : $this->entityTypeId;
     $this->languages = $this->languageManager()->getLanguages(Language::STATE_ALL);
 
     foreach ($values as $key => $value) {
       // If the key matches an existing property set the value to the property
-      // to ensure non converted properties have the correct value.
+      // to set properties like isDefaultRevision.
+      // @todo: Should this be converted somehow?
       if (property_exists($this, $key) && isset($value[Language::LANGCODE_DEFAULT])) {
         $this->$key = $value[Language::LANGCODE_DEFAULT];
       }
-      $this->values[$key] = $value;
+    }
+
+    $this->values = $values;
+    foreach ($this->getEntityType()->getKeys() as $key => $field_name) {
+      if (isset($this->values[$field_name])) {
+        if (is_array($this->values[$field_name]) && isset($this->values[$field_name][Language::LANGCODE_DEFAULT])) {
+          $this->entityKeys[$key] = $this->values[$field_name][Language::LANGCODE_DEFAULT];
+        }
+      }
     }
 
     // Initialize translations. Ensure we have at least an entry for the default
@@ -214,7 +230,7 @@ public function isDefaultRevision($new_value = NULL) {
    * {@inheritdoc}
    */
   public function getRevisionId() {
-    return NULL;
+    return $this->getEntityKey('revision');
   }
 
   /**
@@ -235,9 +251,11 @@ public function preSaveRevision(EntityStorageInterface $storage, \stdClass $reco
    * {@inheritdoc}
    */
   public function getDataDefinition() {
-    $definition = EntityDataDefinition::create($this->getEntityTypeId());
-    $definition->setBundles(array($this->bundle()));
-    return $definition;
+    if (!$this->dataDefinition) {
+      $this->dataDefinition = EntityDataDefinition::create($this->getEntityTypeId());
+      $this->dataDefinition->setBundles(array($this->bundle()));
+    }
+    return $this->dataDefinition;
   }
 
   /**
@@ -347,6 +365,7 @@ public function __sleep() {
     }
     $this->fields = array();
     $this->fieldDefinitions = NULL;
+    $this->dataDefinition = NULL;
     $this->clearTranslationCache();
 
     return parent::__sleep();
@@ -356,21 +375,21 @@ public function __sleep() {
    * {@inheritdoc}
    */
   public function id() {
-    return $this->id->value;
+    return $this->getEntityKey('id');
   }
 
   /**
    * {@inheritdoc}
    */
   public function bundle() {
-    return $this->bundle;
+    return $this->getEntityKey('bundle');
   }
 
   /**
-   * Overrides Entity::uuid().
+   * {inheritdoc}
    */
   public function uuid() {
-    return $this->get('uuid')->value;
+    return $this->getEntityKey('uuid');
   }
 
   /**
@@ -443,8 +462,8 @@ protected function getTranslatedField($name, $langcode) {
    * {@inheritdoc}
    */
   public function set($name, $value, $notify = TRUE) {
-    // If default language changes we need to react to that.
-    $notify = $name == 'langcode';
+    // If default language or an entity key changes we need to react to that.
+    $notify = $name == 'langcode' || in_array($name, $this->getEntityType()->getKeys());
     $this->get($name)->setValue($value, $notify);
   }
 
@@ -592,6 +611,15 @@ public function onChange($name) {
       }
       $this->updateFieldLangcodes($this->defaultLangcode);
     }
+    // Check if the changed name is the value of an entity key and if the value
+    // of that is currently cached, if so, reset it. Exclude the bundle from
+    // that check, as it ready only and must not change, unsetting it could
+    // lead to recursions.
+    if ($key = array_search($name, $this->getEntityType()->getKeys())) {
+      if (isset($this->entityKeys[$key]) && $key != 'bundle') {
+        unset($this->entityKeys[$key]);
+      }
+    }
   }
 
   /**
@@ -681,6 +709,9 @@ protected function initializeTranslation($langcode) {
     $translation->translations = &$this->translations;
     $translation->enforceIsNew = &$this->enforceIsNew;
     $translation->translationInitialize = FALSE;
+    // The label is the only entity key that can change based on the language,
+    // so unset that in case it is currently set.
+    unset($translation->entityKeys['label']);
 
     return $translation;
   }
@@ -707,7 +738,7 @@ public function addTranslation($langcode, array $values = array()) {
     // Instantiate a new empty entity so default values will be populated in the
     // specified language.
     $entity_type = $this->getEntityType();
-    $default_values = array($entity_type->getKey('bundle') => $this->bundle, 'langcode' => $langcode);
+    $default_values = array($entity_type->getKey('bundle') => $this->bundle(), 'langcode' => $langcode);
     $entity = $this->entityManager()
       ->getStorage($this->getEntityTypeId())
       ->create($default_values);
@@ -905,6 +936,8 @@ public function createDuplicate() {
       $duplicate->{$entity_type->getKey('revision')}->value = NULL;
     }
 
+    $duplicate->entityKeys = array();
+
     return $duplicate;
   }
 
@@ -948,8 +981,8 @@ public function label() {
     if (($label_callback = $entity_type->getLabelCallback()) && is_callable($label_callback)) {
       $label = call_user_func($label_callback, $this);
     }
-    elseif (($label_key = $entity_type->getKey('label')) && isset($this->{$label_key})) {
-      $label = $this->{$label_key}->value;
+    elseif (($label_key = $entity_type->getKey('label'))) {
+      $label = $this->getEntityKey('label');
     }
     return $label;
   }
@@ -975,6 +1008,30 @@ public function referencedEntities() {
     return $referenced_entities;
   }
 
+  /**
+   * Returns the value of the given entity key, if defined.
+   *
+   * @param string $key
+   *   Name of the entity key, for example id, revision or bundle.
+   *
+   * @return mixed
+   *   The value of the entity key, NULL if not defined.
+   */
+  protected function getEntityKey($key) {
+    if (!isset($this->entityKeys[$key]) || !array_key_exists($key, $this->entityKeys)) {
+      if ($this->getEntityType()->hasKey($key)) {
+        $field_name = $this->getEntityType()->getKey($key);
+        $property = $this->getFieldDefinition($field_name)->getMainPropertyName();
+        $this->entityKeys[$key] = $this->get($field_name)->$property;
+      }
+      else {
+        $this->entityKeys[$key] = NULL;
+      }
+
+    }
+    return $this->entityKeys[$key];
+  }
+
   /**
    * {@inheritdoc}
    */

core/modules/aggregator/lib/Drupal/aggregator/Entity/Feed.php

@@ -45,13 +45,6 @@
  */
 class Feed extends ContentEntityBase implements FeedInterface {
 
-  /**
-   * Implements Drupal\Core\Entity\EntityInterface::id().
-   */
-  public function id() {
-    return $this->get('fid')->value;
-  }
-
   /**
    * Implements Drupal\Core\Entity\EntityInterface::label().
    */

core/modules/aggregator/lib/Drupal/aggregator/Entity/Item.php

@@ -33,13 +33,6 @@
  */
 class Item extends ContentEntityBase implements ItemInterface {
 
-  /**
-   * Implements Drupal\Core\Entity\EntityInterface::id().
-   */
-  public function id() {
-    return $this->get('iid')->value;
-  }
-
   /**
    * Implements Drupal\Core\Entity\EntityInterface::label().
    */

core/modules/block/custom_block/lib/Drupal/custom_block/Entity/CustomBlock.php

@@ -76,13 +76,6 @@ public function createDuplicate() {
     return $duplicate;
   }
 
-  /**
-   * {@inheritdoc}
-   */
-  public function getRevisionId() {
-    return $this->get('revision_id')->value;
-  }
-
   /**
    * {@inheritdoc}
    */

core/modules/comment/lib/Drupal/comment/Entity/Comment.php

@@ -59,13 +59,6 @@ class Comment extends ContentEntityBase implements CommentInterface {
    */
   protected $threadLock = '';
 
-  /**
-   * {@inheritdoc}
-   */
-  public function id() {
-    return $this->get('cid')->value;
-  }
-
   /**
    * {@inheritdoc}
    */

core/modules/contact/lib/Drupal/contact/Entity/Message.php

@@ -38,7 +38,7 @@
 class Message extends ContentEntityBase implements MessageInterface {
 
   /**
-   * Overrides Drupal\Core\Entity\Entity::id().
+   * {@inheritdoc}
    */
   public function id() {
     return NULL;

core/modules/file/lib/Drupal/file/Entity/File.php

@@ -46,13 +46,6 @@ class File extends ContentEntityBase implements FileInterface {
     'langcode' => array(Language::LANGCODE_DEFAULT => array(0 => array('value' => Language::LANGCODE_NOT_SPECIFIED))),
   );
 
-  /**
-   * {@inheritdoc}
-   */
-  public function id() {
-    return $this->get('fid')->value;
-  }
-
   /**
    * {@inheritdoc}
    */

core/modules/node/lib/Drupal/node/Entity/Node.php

@@ -61,20 +61,6 @@
  */
 class Node extends ContentEntityBase implements NodeInterface {
 
-  /**
-   * Implements Drupal\Core\Entity\EntityInterface::id().
-   */
-  public function id() {
-    return $this->get('nid')->value;
-  }
-
-  /**
-   * Overrides Drupal\Core\Entity\Entity::getRevisionId().
-   */
-  public function getRevisionId() {
-    return $this->get('vid')->value;
-  }
-
   /**
    * {@inheritdoc}
    */

core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Entity/EntityTestRev.php

@@ -42,13 +42,6 @@
  */
 class EntityTestRev extends EntityTest {
 
-  /**
-   * Implements Drupal\Core\Entity\EntityInterface::getRevisionId().
-   */
-  public function getRevisionId() {
-    return $this->get('revision_id')->value;
-  }
-
   /**
    * {@inheritdoc}
    */

core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Term.php

@@ -54,13 +54,6 @@
  */
 class Term extends ContentEntityBase implements TermInterface {
 
-  /**
-   * {@inheritdoc}
-   */
-  public function id() {
-    return $this->get('tid')->value;
-  }
-
   /**
    * {@inheritdoc}
    */

core/modules/user/lib/Drupal/user/Entity/User.php

@@ -60,13 +60,6 @@ class User extends ContentEntityBase implements UserInterface {
    */
   protected $hostname;
 
-  /**
-   * {@inheritdoc}
-   */
-  public function id() {
-    return $this->get('uid')->value;
-  }
-
   /**
    * {@inheritdoc}
    */

core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php

@@ -89,6 +89,13 @@ class ContentEntityBaseUnitTest extends UnitTestCase {
    */
   protected $id;
 
+  /**
+   * Field definitions.
+   *
+   * @var \Drupal\Core\Field\FieldDefinition[]
+   */
+  protected $fieldDefinitions;
+
   /**
    * {@inheritdoc}
    */
@@ -114,6 +121,12 @@ public function setUp() {
     $this->bundle = $this->randomName();
 
     $this->entityType = $this->getMock('\Drupal\Core\Entity\EntityTypeInterface');
+    $this->entityType->expects($this->any())
+      ->method('getKeys')
+      ->will($this->returnValue(array(
+        'id' => 'id',
+        'uuid' => 'uuid',
+    )));
 
     $this->entityManager = $this->getMock('\Drupal\Core\Entity\EntityManagerInterface');
     $this->entityManager->expects($this->any())
@@ -155,7 +168,7 @@ public function setUp() {
     $container->set('plugin.manager.field.field_type', $this->fieldTypePluginManager);
     \Drupal::setContainer($container);
 
-    $field_definitions = array(
+    $this->fieldDefinitions = array(
       'id' => FieldDefinition::create('integer'),
       'revision_id' => FieldDefinition::create('integer'),
     );
@@ -163,10 +176,7 @@ public function setUp() {
     $this->entityManager->expects($this->any())
       ->method('getFieldDefinitions')
       ->with($this->entityTypeId, $this->bundle)
-      ->will($this->returnValue(array(
-        'id' => FieldDefinition::create('integer'),
-        'revision_id' => FieldDefinition::create('integer'),
-      )));
+      ->will($this->returnValue($this->fieldDefinitions));
     $this->entity = $this->getMockForAbstractClass('\Drupal\Core\Entity\ContentEntityBase', array($values, $this->entityTypeId, $this->bundle));
   }
 
@@ -193,15 +203,32 @@ public function testIsNewRevision() {
       ->method('getKey')
       ->with('revision')
       ->will($this->returnValue('revision_id'));
+    $this->entityType->expects($this->at(4))
+      ->method('hasKey')
+      ->with('revision')
+      ->will($this->returnValue(TRUE));
+    $this->entityType->expects($this->at(5))
+      ->method('getKey')
+      ->with('revision')
+      ->will($this->returnValue('revision_id'));
 
     $field_item_list = $this->getMockBuilder('\Drupal\Core\Field\FieldItemList')
       ->disableOriginalConstructor()
       ->getMock();
+    $field_item = $this->getMockBuilder('\Drupal\Core\Field\FieldItemBase')
+      ->disableOriginalConstructor()
+      ->getMock();
+    $field_item->staticExpects($this->once())
+      ->method('mainPropertyName')
+      ->will($this->returnValue('value'));
+
     $this->typedDataManager->expects($this->any())
       ->method('getPropertyInstance')
       ->with($this->entity, 'revision_id', NULL)
       ->will($this->returnValue($field_item_list));
 
+    $this->fieldDefinitions['revision_id']->getItemDefinition()->setClass(get_class($field_item));
+
     $this->assertFalse($this->entity->isNewRevision());
     $this->assertTrue($this->entity->isNewRevision());
     $this->entity->setNewRevision(TRUE);