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
Discovered by @kevin.dutra in #2644088-20: DefaultTableMapping::getFieldTableName does not report table for fields with dedicated storage:
SqlContentEntityStorage contains a $tableMapping property which is populated on the first call to getTableMapping(). Subsequent calls use this locally cached version rather than recreating it. However, nothing forces this to be rebuilt after a field storage definition is inserted, so the table mapping has stale data for the remainder of the request.
Proposed resolution
Figure out a way to clear the SqlContentEntityStorage::$tableMapping static property when a new field storage definition is added.
Remaining tasks
Write a patch.
User interface changes
Nope.
API changes
I hope none.
Data model changes
Nope.
| Comment | File | Size | Author |
|---|---|---|---|
| #19 | 2705205-19.patch | 2.52 KB | amateescu |
| #17 | interdiff-17.txt | 3.99 KB | amateescu |
| #17 | 2705205-17.patch | 3.86 KB | amateescu |
| #13 | interdiff-12.txt | 2.05 KB | amateescu |
| #13 | 2705205-12.patch | 4.16 KB | amateescu |
Comments
Comment #4
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI was poking around in this area lately and it seems that simply clearing the statically cached $tableMapping at the end of
\Drupal\Core\Entity\Sql\SqlContentEntityStorage::onFieldStorageDefinitionCreate()will fix the test from #2644088-20: DefaultTableMapping::getFieldTableName does not report table for fields with dedicated storage, but it does not allow us to remove the @todo from\Drupal\Core\Entity\Sql\SqlContentEntityStorage::countFieldData().Comment #10
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedLooking at this issue again after working on various places related to the table mapping, I think the current code for
\Drupal\Core\Entity\Sql\SqlContentEntityStorage::countFieldData()in HEAD is actually correct and doesn't need the@todoanymore. Updated the comment to make it clear why we need to use a fresh table mapping instead of the cached one.Comment #11
tstoecklerYes, this makes sense to me, as well. Not sure if we want to handle field storage update and delete here, as well, otherwise this is RTBC.
Comment #13
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedSince CRUD operations on field storage definitions usually go hand-in-hand, I think it's good to be consistent and save our future selves some unwanted headaches :)
Comment #15
tstoecklerOK, perfect! Looks good to me and I agree fully with #13. Just not sure whether we need dedicated test coverage for the update and delete cases, so not yet setting to RTBC to let someone else chime in, maybe.
Comment #17
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe difference between the code path taken in #4 and #10/#13 is that
\Drupal\Core\Entity\Sql\SqlContentEntityStorage::onFieldStorageDefinitionCreate()is operating on a cloned storage object (see\Drupal\Core\Field\FieldStorageDefinitionListener::onFieldStorageDefinitionCreate()), so setting the table mapping to NULL on the cloned storage object has no effect on the storage object cached in\Drupal\Core\Entity\EntityTypeManager::$handlers.The only other option that I see is to reset the entity type manager cache directly in
FieldStorageDefinitionListener, just like we do for the field manager cache.Comment #19
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis problem was fixed in #2554235: Make the content entity storage and entity query use the last installed definitions instead of the ones living in code, but it would be nice to commit the extra test coverage from this issue.
This patch is basically the test-only one from #4.
Comment #20
plachComment #21
plachLooks good!
Comment #22
plachComment #23
alexpottComment #24
alexpottCommitted and pushed 7c9ecd6c71 to 8.8.x and ead2914b5c to 8.7.x. Thanks!
As a documentation and test only patch this is eligible for backport to 8.7.x during the beta.