We talked about this today and agreed to just implement the UUID on the ConfigEntityBase class, and enforce this key.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, d8-uuid-move.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
22.05 KB

Hmmm, we would maybe have to do something like this if we really want to move this... or maybe a step too far? It seems if we make the assumption that everything extending ConfigEntityBase (not implementing ConfigEntityInterface necessarily) and then actually add the uuid key back programmatically for all entities extending ConfigEntityBase. Discuss.

Status: Needs review » Needs work

The last submitted patch, 2004336-2.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
21.82 KB

errrhhmmm

Status: Needs review » Needs work

The last submitted patch, 2004336-4.patch, failed testing.

dawehner’s picture

Just a rerole.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -78,6 +78,10 @@ public function __construct($entity_type, array $entity_info, ConfigFactory $con
+    if (isset($this->entityInfo['entity_keys']['uuid'])) {
+      $this->uuidKey = $this->entityInfo['entity_keys']['uuid'];
+    }

+++ b/core/modules/config/config.moduleundefined
@@ -90,3 +90,19 @@ function config_menu() {
+
+/**
+ * Implements hook_entity_info().
+ */
+function config_entity_info(&$info) {
+  $config_entities = array_filter($info, function($entity_info) {
+    // Use the ConfigEntityBase class, and not ConfigEntityInterface. As the
+    // uuid key assumption is made in ConfigEntityBase.
+    return is_subclass_of($entity_info['class'], 'Drupal\Core\Config\Entity\ConfigEntityBase');
+  });
+
+  // Merge the 'uuid' key into all entity_keys data.
+  foreach (array_keys($config_entities) as $type) {
+    $info[$type]['entity_keys'] += array('uuid' => 'uuid');
+  }

I thought config module just provides the UI so we can't rely on that functionality here?

tim.plunkett’s picture

andypost’s picture

Related #2182239: Improve ContentEntityBase::id() for better DX supposes that 'id' key works wrong for some config entities

damiankloip’s picture

Issue summary: View changes

This has basically been done along the way somewhere. We can just clean up the classes now.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
15.07 KB

We should really default the uuid key in the ConfigEntityType imo. and remove all the duplicate property declarations for uuid ?

damiankloip’s picture

Title: Move uuid property to ConfigEntityBase and remove from annotation » Default UUID key in ConfigEntityType

Maybe that is a more accurate title now.

Status: Needs review » Needs work

The last submitted patch, 10: 2004336-10.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
16.55 KB
1.48 KB

That's not fragile at all...no ;)

tstoeckler’s picture

This is certainly a better approach than the one I had in #2208285: Make ConfigEntityType::getKey() enforce the UUID key. However, this should also revert the relevant hunks that were introduced in #2198377: Enforce UUID key name in configuration entities, namely:

diff --git a/core/includes/entity.inc b/core/includes/entity.inc
index 0ade970..226d82a 100644
--- a/core/includes/entity.inc
+++ b/core/includes/entity.inc
@@ -240,7 +240,12 @@ function entity_revision_delete($entity_type, $revision_id) {
  */
 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')) {
+
+  // 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')) {
     throw new EntityStorageException("Entity type $entity_type_id does not support UUIDs.");
   }
 
diff --git a/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
index 42ad63e..87b43f7 100644
--- a/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -174,7 +174,12 @@ public function isSyncing() {
    * {@inheritdoc}
    */
   public function createDuplicate() {
-    $duplicate = parent::createDuplicate();
+    $duplicate = clone $this;
+    $duplicate->set($this->getEntityType()->getKey('id'), NULL);
+
+    // @todo Inject the UUID service into the Entity class once possible.
+    $duplicate->set('uuid', \Drupal::service('uuid')->generate());
+
     // Prevent the new duplicate from being misinterpreted as a rename.
     $duplicate->setOriginalId(NULL);
     return $duplicate;
tstoeckler’s picture

damiankloip’s picture

Related issues: -#2198377: Enforce UUID key name in configuration entities
FileSize
18.1 KB
1.55 KB

Yes, that indeed makes a lot of sense!

damiankloip’s picture

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, looks great!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Less code++

Committed 0f35ae9 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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