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
For DX ContentEntityDatabaseStorage provides defaults for the base and data table if it is not specified. Actually doing this, however, i.e. specifying an entity type as translatable but not specifying a data table leads to a fatal error upon module installation.
The problem is that, due to the default value inside of ContentEntityDatabaseStorage $this->getDataTable() returns something different (i.e. the default value) than $this->entitytype->getDataTable() (which returns an empty string).
When building the schema, the schema handler asks the storage whether there is a data table when initializing the tables but when building the actual table mapping the storage asks the entity type directly. Therefore the resulting schema has the scaffolding for a data table, but the fields key is empty. Thus an SQL exception is thrown as a table is trying to be created without any fields/columns.
Proposed resolution
Replace $this->entityType->getDataTable() with $this->getDataTable() in ContentEntityDatabaseStorage::getTableMapping().
Remaining tasks
User interface changes
API changes
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | 2328565-8-ceds.patch | 12.76 KB | tstoeckler |
| #5 | 2328565-5-ceds-fail.patch | 7.86 KB | tstoeckler |
Comments
Comment #1
tstoecklerJust for fun including a patch to see whether the data table check is still need at all. The second patch should be green and is the actual bugfix here.
We should also provide a test for this.
Comment #4
tstoecklerYeah, I had the slight suspicion that this was no longer needed.
Still needs tests, though me thinks.
Comment #5
tstoecklerHere's a test for this behavior. Instead of testing that the table names can be omitted, I added a test that the defaults can be overridden. The amount of coverage is the same but we want to move away from specifying the table names everywhere so this is the more modern approach. The test tests all four table types, so provides more coverage than just for this bug, but that shouldn't hurt.
I uploaded the interdiff as the "fail" patch as only a very small non-test fix is contained there, so that should work.
If this is red/green as expected I think it is RTBC material.
Comment #8
tstoecklerAhh, I had feared that this would uncover more issues.
Comment #11
kristiaanvandeneyndeI just ran into this as well.
I think the right approach here would be to make sure that both
$this->entitytype->getDataTable()and$this->getDataTable()return the same result. Trying to replace calls with their counterpart just seems like we will still have places where this may bug out.Seeing as core relies on the data table being BASETABLE_field_data all over the place, why not have EntityType make sure it has the right defaults set when the keys are omitted and read those values in the storage handler instead of figuring out the defaults there (and in views)?
Comment #12
kristiaanvandeneyndeOr, alternatively, seeing as getBaseTable(), getDataTable(), etc. all mention they are only used in the SQL storage: Why don't we have all the logic in the SQL storage and let the EntityType do something like this?
Comment #13
tstoecklerRe #11: As you mention in #12 the idea was that at some point (i.e. 9.x) we could remove EntityType::getBaseTable() so that's why we moved more calls to the storage directly. I'm not sure if I still agree with that, but that was the idea at the time.
You idea in #12 actually sounds fairly awesome in principle, but I'm not sure how we would get the
$entity_type_managerfrom within an entity type. We could say, that this is code that's going to me dropped in 9.x anyway, and we don't care about elegance and slam a\Drupal::service('entity_type.manager')in there, but (even though I would be fine with that) I don't think that's going to fly.Comment #14
kristiaanvandeneyndeLooking at EntityType::getBundleConfigDependency(), core seems to be fine with using \Drupal in there.
Comment #15
tstoecklerWow, that's true. Well I guess why not, then?
Comment #20
bradjones1Comment #21
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI wonder how was the patch from #8 generated on September 3 when (partly) the same fix was committed on August 18: #2322097: Enforce data tables for translatable entity types in the SQL entity storage.
Does that mean that this is no longer a problem and we can close it?
Comment #22
tstoecklerA) The other fix was in a different place in the same file. So while it won't apply anymore, it did at the time.
B) No, I think it's still needed.
SqlContentEntityStoragestill pretends to hold it's own reference to the table names independent of the entity type. So we should fix those instances where we are still introspecting the entity type directly. Of course, the whole premise of this might change now with #2232465: Deprecate table names from entity definitions or some of the other related issues, but I don't think we can just close it.Comment #23
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedOh, I didn't realize that the fix was in a different place, sorry :) And I agree that we should see how #2232465: Deprecate table names from entity definitions pans out before doing anything here.