In EntityFieldManager::buildBundleFieldDefinitions(), we have some supporting code for automatically setting a BaseFieldDefinition's name, entity type ID and bundle name. However, we don't do the same in EntityFieldManager::buildFieldStorageDefinitions().
This lead to an annoying "bug" on my end where I tried to declare a bundle field using FieldableEntityInterface::bundleFieldDefinitions() and its storage using hook_entity_field_storage_info(), providing both of them with the same BaseFieldDefinition. The dedicated table that got created for an unlimited cardinality field was named __FIELDNAME instead of ENTITYTYPEID__FIELDNAME because it couldn't find an entity type ID.
When Drupal tried to store data in that dedicated table, it correctly tried to store it in ENTITYTYPEID__FIELDNAME because it was able to properly get the table name from the field definition (not storage definition!) because we properly inform it of the entity type it's attached to in EntityFieldManager::buildBundleFieldDefinitions().
Proposed solution:
Also set the name and entity type ID in EntityFieldManager::buildFieldStorageDefinitions().
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 2927500-14.patch | 2.34 KB | quietone |
| #16 | 2927500-14-fail.patch | 1.62 KB | quietone |
| #14 | 2927500-14.patch | 2.34 KB | phenaproxima |
| #8 | interdiff_2-8.txt | 1.71 KB | ridhimaabrol24 |
| #8 | 2927500-8.patch | 1.56 KB | ridhimaabrol24 |
Comments
Comment #2
kristiaanvandeneyndeNot sure if feature request or bug. Also changing the IS because we obviously want to set the entity type ID to the one we're currently retrieving storage definitions for. The only thing we might want to do is put the setName() call in an if-statement checking for any value in getName() and, when there is a name set, leave it like that.
Comment #7
kristen polI tested the patch on 9.1.x and it still applies cleanly. I see the similar code in
EntityFieldManager::buildBundleFieldDefinitions. It seems like the only way to test this is with code so marking for tests.Comment #8
ridhimaabrol24 commentedAdding the required test. Please review! Thanks
Comment #10
kristiaanvandeneynde@ridhimaabrol24 What is wrong with that interdiff? It seems like you moved the fix into the wrong place?
Comment #11
ridhimaabrol24 commented@kristiaanvandeneynde patch #8 is a reroll to the latest head of 9.1.x branch. The code has changed since drupal 8.4, hence you are seeing that difference.
Comment #12
kristiaanvandeneyndeOh wow, never seen an interdiff for a reroll before. Thanks for clearing that up.
Comment #13
ridhimaabrol24 commentedYeah my mistake, shouldn't have added an interdiff for a reroll. Apology there.
Comment #14
phenaproximaThis should fix the broken test. I had to mock a base field definition specifically, rather than a generic FieldStorageDefinitionInterface, since the calls we're adding here are only done to base fields (and, indeed, only can be done to base fields).
Comment #16
quietone commentedI reviewed the patch and it look good to me. I am uploading a fail patch and re-uploading the successful patch just to double check against 9.2.x
edit: s/reviews/reviewed/
Comment #18
quietone commentedTests are still passing, so off we go.
Comment #21
catchCommitted/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!