Why do we have $entity_info['entity keys']['id'] *and* MyEntity::id() method overrides? [#1822176]

Problem

$entity_info['entity keys']['id'] & Co are duplicating MyEntity::id() & Co method overrides.

Goal

Simplify the entity system by removing duplicate abstraction facilities.

Details

In order to implement an entity that does not use the standard ::$id property (e.g., 'nid', 'name', etc), you have to

Specify the entity key in hook_entity_info():

      'entity keys' => array(
        'id' => 'name',
        'label' => 'label',
        'uuid' => 'uuid',
      ),

AND override the corresponding methods on the entity class:

  /**
   * Overrides Drupal\Core\Entity\Entity::id().
   */
  public function id() {
    return $this->name;
  }

The entity info is not used by the entity class itself. Only by the entity storage controller.
The entity class itself heavily relies on the overridden method.
Even the new EntityNG does not use the entity info; the id(), label(), and uuid() methods are still accessing the hard-coded 'id', 'label', and 'uuid' property names directly.

Proposed solution

Remove one of both.

Notes

Is there any reason for why entity storage controllers are not able to use the entity class methods instead of entity info?

Comments

Comment #1

sun

German

Karlsruhe

CreditAttribution: sun commented 24 October 2012 at 19:13

I now see that DatabaseStorageController uses the entity info like this:

      if (!empty($this->entityInfo['entity class'])) {
        // We provide the necessary arguments for PDO to create objects of the
        // specified entity class.
        // @see Drupal\Core\Entity\EntityInterface::__construct()
        $query_result->setFetchMode(PDO::FETCH_CLASS, $this->entityInfo['entity class'], array(array(), $this->entityType));
      }
      $queried_entities = $query_result->fetchAllAssoc($this->idKey);

      $query->condition("base.{$this->idKey}", $ids, 'IN');

So those use-cases cannot work with an ::id() method, because there is no entity object yet.

However. The entity class is known.

And how the keys are used/implemented on the entity class is actually specific to a particular entity class. I.e., if you override the entity class, then the entity keys definition in entity info may no longer be valid.

Thus, the entity keys definition is actually bound to a specific class implementation.

I wonder whether we could do this:

class Node extends Entity {

  public static $keys = array(
    'id' => 'nid',
    'uuid' => 'uuid',
    'label' => 'title',
  );

}

Or possibly also this:

class Entity {

  protected static $entityKeys = array(
    'id' => 'id',
    'uuid' => 'uuid',
    'label' => 'label',
  );

  public static function getEntityKey($key) {
    return isset(static::$entityKeys[$key]) ? static::$entityKeys[$key] : NULL;
  }

}
class Node extends Entity {

  protected static $entityKeys = array(
    'id' => 'nid',
    'uuid' => 'uuid',
    'label' => 'title',
  );

}

Or perhaps even this?

class Entity {

  protected static $idKey = 'id';

  protected static $uuidKey = 'uuid';

  protected static $labelKey = 'label';

}
class Node extends Entity {

  protected static $idKey = 'nid';

  protected static $labelKey = 'title';

}

Comment #2

Berdir

German

Switzerland

CreditAttribution: Berdir commented 24 October 2012 at 21:23

The explicit id() overrides exist for performance reasons. In the issue where we removed $this->entityInfo from the entity classes, it was decided that we do this.

The plan for entity id's is to standardize on id (and e.g. use user_id when referencing a user). All entities which follow that convention won't need to override id().

There is no way to mandate/document static properties/methods using EntityInterface, so I'm not sure if that's a good idea or not.