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
I'm in the process of porting the Search API module to D8 and just tried to create a new config entity type, for search indexes (see here).
As you can see, I renamed all entity keys, since they were already named like this in D7 (where I built on the Entity API contrib module). I'd expect the corresponding functions, like id() and status(), to respect these settings. However, this doesn't seem to be the case.
I had to override id() manually to provide the right field, otherwise the storage controller (which also doesn't seem to use the entity_keys setting, at least there) threw an exception. The entity's status() method also uses the status field instead of enabled, and ConfigEntityBase even adds the property public $status = TRUE;, no matter whether you want that field, or are even using it for something completely different, etc.
I couldn't really test uuid(), as the field is called uuid on my entity, but it seems to have the same problem. Only label() seems to work fine, for whatever reason.
Proposed resolution
Rewrite the id(), uuid() and status() methods on Entity and ConfigEntityBase to use the specified entity keys. Also remove ConfigEntityBase::status as long as the object properties are directly used as the entity fields (compared to, e.g., the approach of EntityNG – which seems to have the same problem, though).
Remaining tasks
Problem to be confirmed.
Patch to be written and tested.
Tests to be written.
User interface changes
None.
API changes
I think this would only fix the current API as it is expected to work.
| Comment | File | Size | Author |
|---|---|---|---|
| #19 | 2052083-entity_keys-19-PASS.patch | 48.19 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettIt seems issues like #2182239: Improve ContentEntityBase::id() for better DX and #2004336: Default UUID key in ConfigEntityType are actively working to fix this.
Comment #2
balagan CreditAttribution: balagan commentedThe mentioned issue in #1 is fixed and committed. Did that solve it?
Comment #3
tim.plunkettEven though this issue is way older, #2429169: ConfigEntityBase::sort() ignores the entity_keys configuration is tackling the same problem. Closing as a duplicate.
Comment #4
MattA CreditAttribution: MattA commentedHmm,
#2182239 only fixed this issue for content entities, not config.
#2004336 seems to be a completely different problem.
#2429169 isn't named very well, and only fixes the problem for 'weight' keys. It does not contain an overall solution for other keys like 'id' or 'status'.
Going to set this as postponed until #2429169: ConfigEntityBase::sort() ignores the entity_keys configuration is either committed or the patch is expanded in scope to actually solve this problem.
Comment #5
tim.plunkettClosed #2429169: ConfigEntityBase::sort() ignores the entity_keys configuration as a dupe.
Not fixing uuid because of #2604450: Prevent uuidKey from being overridden in Configuration entities.
Comment #7
tim.plunkettHeh, turns out we use blocks A LOT in tests.
Comment #8
tim.plunkettComment #11
tim.plunkettWorking more on this.
Comment #12
tim.plunkettFixing all the unit tests that bypass getKey.
Comment #14
tim.plunkettComment #15
dawehnerInteresting #1826602-50: Allow all configuration entities to be enabled/disabled argued against using keys for everything.
If we need that this is an API change, so if we want to do that, we need to adapt
\Drupal\Core\Config\Entity\ConfigEntityType::__constructto deal with it.Comment #16
tim.plunkettIt's interesting, because some entity types already had status in their annotation.
I too would just prefer people override the methods if they're going to change the property they use. That would make more sense for status, unfortunately we didn't standardized on id/label for every one earlier.
Agreed on fixing the constructor for status.
Comment #17
tim.plunkettComment #18
dawehnerSo for test coverage purposes at least one entity type should not specify it.
While I see usecases for id(), its hard to see usecases for status(), given that its a concept in the Drupal domain, unlike id() which is a very generic concept.
Comment #19
tim.plunkettThanks for asking for tests, if you look at my last interdiff you'll see I added it to ContentEntityType not ConfigEntityType, whoops!
Comment #20
tim.plunkettPIFT is having issues today, it can't get results from Drupal CI. but if you click all the way through, you'll see that the FAIL failed and the PASS passed :)
Comment #23
tim.plunkettComment #24
tim.plunkettComment #31
tim.plunkettGet it together DrupalCI. And I uploaded the pass/fail in the other order, why did you switch them...
Comment #32
dawehnerIs there any way to optimize this maybe? Sorting could potentially call that method really often.
In general it feels quite harsh to rewrite all that tests to use prophecy. it makes the patch much bigger than it has to be.
Comment #33
tim.plunkett1) I'm not sure how to optimize this.
We need the getEntityType() call to find the keys...
2) In order to fix this issue, we have to switch from this:
to either this
or
I consider the latter more readable and maintainable. I only switched the mocks when they had to be rewritten anyway.
Comment #34
dawehnerThe other scary bit about this, is that the container is accessed in the constructor, according to tim.
Comment #35
tim.plunkett\Drupal\Core\Entity\Entity::id() with this patch:
Entity::getEntityType() in HEAD and unchanged here:
And of course, entityManager():
So yeah, this change results in the container being consulted in entity constructors.
Comment #47
smustgrave CreditAttribution: smustgrave at Mobomo commentedAfter 6 years wonder if this is still valid.
Imagine it could use a issue summary update.