Problem/Motivation

We pretend that we can configure the UUID key in configuration entity annotations yet we hard code it.

class ConfigStorageController extends EntityStorageControllerBase implements ConfigStorageControllerInterface {

  /**
   * Name of the entity's UUID property.
   *
   * @var string
   */
  protected $uuidKey = 'uuid';

vs

 * @EntityType(
 *   id = "view",
 *   label = @Translation("View"),
 *   controllers = {
 *     "storage" = "Drupal\Core\Config\Entity\ConfigStorageController",
 *     "access" = "Drupal\views\ViewAccessController"
 *   },
 *   admin_permission = "administer views",
 *   config_prefix = "views.view",
 *   entity_keys = {
 *     "id" = "id",
 *     "label" = "label",
 *     "uuid" = "uuid",
 *     "status" = "status"
 *   }
 * )

In #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall we need to be able to work with configuration entities without the entity type actually existing. The only requirement this introduces is that the uuid key is always uuid - which it is. We just make it look like it is not. So this actually does not block that patch.

Proposed resolution

Remove uuid from all config entity annotations.

Remaining tasks

Build consensus
Review patch

User interface changes

None

API changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
15.13 KB

Ho hum.

The only this will break for config entities is

function entity_load_by_uuid($entity_type_id, $uuid, $reset = FALSE) {
  $entity_type = \Drupal::entityManager()->getDefinition($entity_type_id);
  if (!$uuid_key = $entity_type->getKey('uuid')) {
alexpott’s picture

FileSize
1.54 KB
16.89 KB

Easy enough to fix and test entity_load_by_uuid().

The last submitted patch, 1: 2198377.1.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 2: 2198377.2.patch, failed testing.

alexpott’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
692 bytes
17.69 KB

Escalating this to major since if you did declare a different uuid key very interesting things would happen because ConfigEntityBase::createDuplicate and entity_load_by_uuid would use the key name in the annotation but the ConfigStorageController would use the hardcoded value!

Status: Needs review » Needs work

The last submitted patch, 5: 2198377.5.patch, failed testing.

tim.plunkett’s picture

+++ b/core/includes/entity.inc
@@ -267,7 +267,12 @@ function entity_revision_delete($entity_type, $revision_id) {
+  if (is_subclass_of($entity_type->getClass(), 'Drupal\Core\Config\Entity\ConfigEntityBase')) {

We have an interface for this...
Also we're discussing removing it, but #2194783: Rename EntityTypeInterface::isSubclassOf() to ::entityClassImplements() does exist for now.

Why do we still need ConfigStorageController::$uuidKey defined?

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.4 KB
24.14 KB

Good point no need for ConfigStorageController::$uuidKey

Whilst fixing up ConfigSingleImportForm noticed that the xpaths in the ConfigSingleImportExportTest were not quite right.

sun’s picture

Looks good to me.

I'd actually go one step ahead and do the same for 'id', too. (separate issue, of course)

tstoeckler’s picture

Note that in #2143729: Entity definitions miss a language entity key I wanted to make the introduced 'language' key only for content entities, as well, as the config system hardcodes knowledge about the 'langcode' key. So this would be consistent with that. (I received some pushback there, however.)

alexpott’s picture

FileSize
24.11 KB

Rerolled.

sun’s picture

Status: Needs review » Reviewed & tested by the community
Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x.

webchick’s picture

Issue tags: +Needs followup

One thing that stood out while reviewing this was:

+++ b/core/includes/entity.inc
@@ -240,7 +240,12 @@ function entity_revision_delete($entity_type, $revision_id) {
-  if (!$uuid_key = $entity_type->getKey('uuid')) {
+
+  // Configuration entities do not use annotations to set the UUID key.
+  if ($entity_type->isSubclassOf('Drupal\Core\Config\Entity\ConfigEntityInterface')) {
+    $uuid_key = 'uuid';
+  }
+  elseif (!$uuid_key = $entity_type->getKey('uuid')) {

Unfortunately, that "after" code got a lot uglier. :\ When asked about this, Alex explained that it's because we're only doing this hardcoded uuid key name assumption for config entities, not for content entities. It'd be great to open a follow-up to decide if that dichotomy actually makes sense. Because if it makes sense to do it in both entity types, we could remove the if check entirely; and if not, we should move that isSubClassOf() check behind a method (like getUUID() or similar).

Berdir’s picture

I think that's covered by #2194783: Rename EntityTypeInterface::isSubclassOf() to ::entityClassImplements() already, at least partially. Title is wrong, the currently suggested approach is to hide the ugliness behind isConfig()/isContent(), which covers the two most frequently (only, right now) usages).

I don't think we should enforce the uuid column for all entity types, a nice example are feed items, which already have the externally provided guid field, it would be pointless to enforce an additional internal uuid for them.

tstoeckler’s picture

We could, however, and I think that might have been what @webchick suggests, provide a getUuidKey() on EntityTypeInterface instead of getKey('uuid'). That would resolve this dance. ConfigEntityInterface could just return 'uuid' directly.

Actually, writing that out, I like that a lot. I'll open an issue for that.

Berdir’s picture

And what exactly is stopping ConfigEntityType from overriding getKey() for that? :) ?

sun’s picture

And what exactly is stopping ConfigEntityType from overriding getKey() for that?

Typically, the final keyword.

Berdir’s picture

I think you misunderstood. I'm not asking how to prevent it (final is bad for mocking btw), I'm asking why we can not simply override the existing getKey() method, instead of introducing a new one?

tstoeckler’s picture

tstoeckler’s picture

webchick’s picture

Cool, thanks all!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.