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
    1. Specify the entity key in hook_entity_info():
            'entity keys' => array(
        'id' => 'name',
        'label' => 'label',
        'uuid' => 'uuid',
      ),
    2. 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

  1. 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

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

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';

}
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

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.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Active » Closed (works as designed)

I think with the move to annotations, this has become less confusing. And the performance gain is important.