Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#11 | 2198377.11.patch | 24.11 KB | alexpott |
#8 | 2198377.8.patch | 24.14 KB | alexpott |
#8 | 5-8-interdiff.txt | 6.4 KB | alexpott |
#5 | 2198377.5.patch | 17.69 KB | alexpott |
#5 | 2-5-interdiff.txt | 692 bytes | alexpott |
Comments
Comment #1
alexpottHo hum.
The only this will break for config entities is
Comment #2
alexpottEasy enough to fix and test
entity_load_by_uuid()
.Comment #5
alexpottEscalating 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!
Comment #7
tim.plunkettWe 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?
Comment #8
alexpottGood point no need for ConfigStorageController::$uuidKey
Whilst fixing up ConfigSingleImportForm noticed that the xpaths in the ConfigSingleImportExportTest were not quite right.
Comment #9
sunLooks good to me.
I'd actually go one step ahead and do the same for 'id', too. (separate issue, of course)
Comment #10
tstoecklerNote 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.)
Comment #11
alexpottRerolled.
Comment #12
sunComment #13
Dries CreditAttribution: Dries commentedCommitted to 8.x.
Comment #14
webchickOne thing that stood out while reviewing this was:
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).
Comment #15
BerdirI 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.
Comment #16
tstoecklerWe 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.
Comment #17
BerdirAnd what exactly is stopping ConfigEntityType from overriding getKey() for that? :) ?
Comment #18
sunTypically, the final keyword.
Comment #19
BerdirI 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?
Comment #20
tstoecklerSure that's possible as well.
Opened #2208285: Make ConfigEntityType::getKey() enforce the UUID key for that.
Comment #21
tstoecklerThe follow-up is now: #2004336: Default UUID key in ConfigEntityType
Comment #22
webchickCool, thanks all!