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.Updated: Comment #0
@plach came up with this idea, so kudos go to him.
Problem/Motivation
The names of the entity tables for entity types are exposed in the entity type annotation even though, they really are an implementation detail of the storage.
Proposed resolution
Once #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities goes in we are technically able to provide sane defaults for the table names, so that they no longer need to live in the annotation, but an entity type does not need to override ContentEntityDatabaseStorage either in order to provide the table names. It can still do that, though, if it does wants to use non-default names.
Remaining tasks
- Commit #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities
User interface changes
None.
API changes
- Entity type no longer define their table names in the annotation
EntityTypeInterface::getBaseTable(),EntityTypeInterface::getRevisionTable(),EntityTypeInterface::getDataTable()andEntityTypeInterface::getRevisionDataTable()are removed without replacement. This is a good thing, as they are an implementation detail.
| Comment | File | Size | Author |
|---|---|---|---|
| #27 | 2232465-27.patch | 3.93 KB | andypost |
| #21 | interdiff-21.txt | 1.85 KB | amateescu |
Comments
Comment #1
plachComment #2
plachComment #8
BerdirMaybe as a start we should just have defaults instead of removing the ability to set them, which honestly seems very unlikely to get to any time soon, simply because this would break anything that would not follow the default/enforced table name pattern already.
Comment #9
BerdirThat would look like this.
Just getting rid of those weird *property* table names would be worth doing this :p
Comment #10
BerdirI guess this will break contact_message not having a table at all, lets see.
Comment #12
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedDo we still want to do this part of the issue summary:
I imagine we'll need to deprecate those instead of removing them?
Comment #13
BerdirNo, my suggestion would neither remove or deprecate those.
Even deprecating them at this time seems unrealistic IMHO, the current views integration is not going away any time soon and that works on tables and needs those methods.
I'll have a look at the test fails later, looks like at least one is because we have an entity type that is not using lowercase ID... :)
Comment #14
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedYep, we won't be able to remove them until D9, but this issue should still be about deprecating them.
Discussed this with @Berdir and agreed to move #9 to a separate issue, so we can keep this one with its initial scope/purpose.
Comment #15
tstoecklerSee also #2328565: Omitting the "base_table" or "data_table" for entity types is broken
Comment #16
plachI agree we are not allowed to remove the keys from the entity type annotation until D9.
The only viable solution for D8 would be to deprecate them (and the related getters) but still make sure they have priority over the defaults provided when instantiating the storage (or soon the table mapping), so we keep BC but we basically make them optional. I recently defined a custom entity type and I realized atm they are not optional at all, there is core code that (silently) breaks if they are missing.
In fact, we can only deprecate them once the Views code relies on the table mapping, at very least to retrieve those values. We probably don't need for #2079019: Make Views use SqlEntityStorageInterface to happen, though.
Comment #17
plachWhich is what #15 was subtly trying to tell me, I guess :D
Comment #18
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@Berdir opened #2958963: Provide default values for the table name annotations for content entities for his patch from #9 and I also opened #2960037: Add a way to get the primary table of an entity type from TableMappingInterface to provide a replacement for the
$entity_type->get(Base|Data|Revision|RevisionData)Table()methods that will be deprecated here.Comment #19
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedHere's a start for this. It changes both the entity type definition and the default SQL storage to get the table names from the table mapping, while keeping the ability to specify custom table names in the entity definition.
Comment #21
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis is actually a BC break since code that was relying on NULL output from these methods on
EntityTypewill now receive the auto-generated table name.A lot of unit tests will need to be refactored here, so let's see first if we're ok with the general approach of this patch.
Comment #23
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI split a part of this patch into #2981220: SqlContentEntityStorage should use the table names provided by its table mapping, because that is needed for #2960136: Add the ability to use a prefix for all the tables in a table mapping and it can go in as a small separate change.
Comment #24
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI'm curios how this fares applied on top of #2981220: SqlContentEntityStorage should use the table names provided by its table mapping.
Comment #27
andypostReroll
Now it needs CR and test for deprecation which could be common for #2958963: Provide default values for the table name annotations for content entities