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
The entity reference field can not reference configuration entities because it can only store integer IDs and it explicitly prohibits users from configuring the field to reference config entities.
Proposal
Change the target_id property to be either int or string, depending on the configured entity type reference.
Original report
Problem
- UUIDs have been added to all entities, but reference fields (such as taxonomy term fields) still reference by serial ID only.
Goal
- Add a UUID column to all reference fields.
Comment | File | Size | Author |
---|---|---|---|
#81 | 1757452-81.patch | 32.11 KB | amateescu |
#80 | 1757452-80.patch | 32.18 KB | amateescu |
#77 | 1757452-77.patch | 32.36 KB | amateescu |
#77 | interdiff.txt | 6.51 KB | amateescu |
#76 | 1757452_76.patch | 28.88 KB | chx |
Comments
Comment #1
sunComment #2
moshe weitzman CreditAttribution: moshe weitzman commentedAre you suggesting to keep existing ref to integer and add uuid as additional information?
Comment #3
sunYeah, an additional uuid column on reference fields (field values). I thought this was originally suggested in #1637370-14: Add UUID support to core entity types, but can't find it in there, so it probably was in another issue.
Comment #4
fagoDo we really need this? We could map ids when import/exporting entities anyway as deploy does it in d7 now?
Comment #5
moshe weitzman CreditAttribution: moshe weitzman commentedI guess that we don't strictly need this. Please reopen if someone disagrees.
Comment #6
cbeier CreditAttribution: cbeier commentedI was a little confused that the UUIDs are only used in the "main entities" and not even in the references themselves. In the current situation is the entity in their unit uniquely identifiable (even across different systems). But this advantage is lost, when the item is related to other tables/entities (or it must be mapped separately between the uuid from the "main entity" and their internal id).
The biggest advantage of the UUID integration (in my opinion) would be, if the main identifier of all items/entities/nodes is a UUID. And these UUID is also used for the internal references. The result would fewer intermediate steps (e.g. for ID mapping) for use cases like data synchronisation.
Comment #7
mitchell CreditAttribution: mitchell commentedComment #8
larowlangoing to give this a crack early next week
Comment #9
webchickGiven the impact that xjm found at #1735118-217: Convert Field API to CMI, this qualifies as at least major.
Comment #10
webchickOopsie.
Comment #11
yched CreditAttribution: yched commentedSo, the problem found with #1735118: Convert Field API to CMI is that the field instance definition, deployed in a CMI file, can contain field values in the 'default_value' entry. For entity_reference or taxo fields, that currently means a local, numeric id, and the reference breaks when the config is deployed.
There's a more general issue that, even if we had only UUIDs in our CMI files, this question of "how do we deploy cross-references between Config and Content, i.e things that are not deployed (if deployed at all) through the same mechanisms" is AFAIK quite blurry at the moment. What do we do when the referenced UUID is absent on the target environment ? What's the nicest way to break, and what tools do we need on the CMI import flow to implement that nicest way ?
Not sure if there's an existing issue about that.
Then there's the issue of making sure our CMI files contain UUIDs rather than local ids, and that's a question for each individual ConfigEntity to solve. "Highly pluggable" subsystems, like Field API or Views, have one additional tricky bit, though, since they are not responsible for a large part of the config they store.
So, for the specific case of field default values :
Those are currently stored in the exact same "canonical" format that's used for field values loaded and saved in an entity, according to the "field schema" of the field type - the format that's produced by the widget in submitted form values :
array('column_1' => 'some value', 'column_2' => 3, ...)
So if a "reference" field type like taxonomy, entity_reference (image and file handle default values their own custom way, even though they store a numeric file id too) store local numeric ids instead of UUIDs, in the current mechanism, that's what will get stored and deployed in the field instance CMI file.
So this either means:
- that reference field types need to change how they store their reference ids,
- or, that we need an additional step for field types to reshape their "default values" when they get saved to or read from the instance definition.
Comment #12
XanoWhat happens now if we delete an entity that is referenced from an entity reference field? Can we mimic that behavior for when entities are unavailable during install?
Comment #13
fagoYep - that would be the solution which would be inline with the currend handling of UUIDs aka map to UUIDs when needed only.
Comment #14
larowlanThis is seriously involved, we don't really support entity_load_multiple_by_properties by uuid so we need to go right down to the storage controller.
With the time given, I reckon this is beyond me.
Will keep tinkering but anyone else who wants it jump in.
Comment #15
XanoAt this moment, is there anything that blocks entity references from using UUIDs?
Comment #16
larowlanXano, configured instance of blocks that represent custom_block derivatives reference the content entity by UUID. Ie Block instance (configured/placed block - a config entity) references the custom block content entity by UUID. This was the main reason I took on that module. Serial IDS in config entities is bad news for deployability.
Comment #17
XanoWe probably misunderstood one another. What I'd like to know if there are any technical barriers left that prevent us from referencing entities by UUID rather than serial ID, so I can go ahead and roll a patch.
Comment #18
larowlanI started down that path but there is no real ability at the storage level to entity_load_multiple_by_uuid (that I could see), so I guess that's the first barrier.
And db perf issues for using a string key instead of a serial.
Comment #19
XanoPerformance can be addressed by not using UUIDs, but to change the schema depending on what type of entity is referenced: integer serial ID for content entities and varchar name for config entities. This is, however, less uniform, but will not be a regression from the current situation.
Comment #20
larowlanYeah I think we're only (mainly) talking about content entities here, especially for default values in fields where we get serial ids bleeding into config entities, limiting deployability.
Comment #21
XanoI'm not sure what you mean. Is there an approach you are in favor of?
Comment #22
larowlanNevermind me :)
Comment #23
XanoCan someone who knows more than me about database performance and field API recommend either 1) using UUIDs or 2) using IDs for content entities and names for config entities?
Comment #24
XanoWorking on a patch that uses the dynamic schema approach.
Comment #25
XanoThose tags were removed accidentally.
This patch is backwards compatible and simply changes the column type for config entities to a varchar. I'm having PHP 5.4 issues locally (#2025451: Drupal\entity_reference\Tests\EntityReferenceAdminTest fails on PHP 5.4.9 and libxml 2.9.0). I will fix those before writing tests.
Comment #26
Xano#25: drupal_1757452_00.patch queued for re-testing.
Comment #27
Xano#2039607: Change node type field to an entity reference and #2039609: Change terms' VID field to an entity reference depend on this issue.
Comment #28
tstoecklerSince we're already changing this, this should *always* be a varchar. ConfigEntities are not the exception by having string IDs, ContentEntities are in fact the exception in that they have serial/int IDs, while that is being enforced nowhere in the system. The current code is quite simply a bug.
If we're worried about performance, we can document that id() must return integers in ContentEntityInterface and then check for that here, but not the other way around.
Hmm.. This should get a @todo at least to clean this up. Even using the same storage controller, different entity types could be saved in different databases, so adding the foreign keys might not always make sense. On the other hand, I could write a BetterDatabaseStorageController that does not subclass the original one and still save my entity type in the same database. Neither of those are very likely, and it's not terrible to add the foreign keys in any case, I guess, but a @todo would be cool.
Did you actually try this, does this actually work? The amount of code is ridiculously small. I also like adding the options() method to EntityManager
Comment #29
XanoNot yet. I haven't been able to address the PHP 5.4 issues I was experiencing, but I just started to write tests anyway a few minutes ago. I'll just let the testbot run those for me.
What's the use of adding foreign keys if we can't even be sure that they point to an existing table? I'd hate to have code in there that only serves one single use case out of many.
Comment #30
tstoecklerYeah, it's one use-case out of many, but it's the 90% one :-)
I discussed with @Xano that we would suggest dropping the foreign keys entirely, and letting contrib solve that. We weren't aware of anything actually using those anyway, so...
Comment #31
yched CreditAttribution: yched commentedPanels / ctools D7 relies on the foreign keys present in the field schema to build possible "relationships"
Comment #32
XanoThis patch removes foreign key support, and simply uses a VARCHAR(255) for storing referenced entities' IDs.
There are some test failures, but I've been staring at them for too long to see what goes wrong.
Comment #34
XanoRe-rolled.
Comment #36
XanoI cannot reproduce the syntax error locally. Hopefully someone else can take a look at it and the test failures.
Comment #37
amateescu CreditAttribution: amateescu commentedI also maintain D7 custom code that relies on the foreign keys definition to build 'relationships', so I would be pretty much against removing this feature unless we have an alternative.
Looking at the patch, the functionality cannot possibly work right now because
\Drupal\Core\Entity\Plugin\DataType\EntityReferenceItem::getPropertyDefinitions()
is not updated, so we clearly need tests..Comment #38
XanoThe problem is–and this existed long before this issue–that we can never be a 100% sure whether entities are stored in the database or not. As @tstoeckler pointed out on IRC, a database storage controller can be written that does not extend DatabaseStorageController, and there will be no foreign keys. True, we're talking about the 20% here, but that doesn't mean we should maintain wonky edge-case code for the 80%. This will bite us later on.
Well spotted. Thanks for pointing this out. I'll reroll the patch.
Comment #39
XanoFixed the whitespace errors and converted the field item target_id property to a string.
The reason why EntityReferenceItemTest fails is because the target_id is not saved, or at least it is emptied upon saving the entity the field is attached to. When inspecting the property definitions on the entity, the reference target ID is correctly set up as a string field, so I don't know why storage fails.
Comment #40
amateescu CreditAttribution: amateescu commentedThis wasn't really a problem in D7 because the usage of alternative storage engines was very very low, but I agree that it can be a potential issue for D8. However, since the foreign keys information is not 'officially' used anywhere, I feel that the responsabilty of how and when to use it is downstream, in contrib and custom code that actually interact with it.
So.. can we please not affect the 80% by being overprotective? :)
Comment #42
yched CreditAttribution: yched commentedWell, if "reference"-like fields (file, image, taxo) somehow settled on EntityRefItem (or subclasses), I guess there could be a smarter way to deduce relationships than looking for foreign keys in a db schema, which is kind of backwards ?
Comment #43
amateescu CreditAttribution: amateescu commentedYes, I guess we can look into their settings definition..
Comment #44
Xano#39: drupal_1757452_39.patch queued for re-testing.
Comment #46
XanoI don't know why it says that. My localhost runs the file just fine.
Comment #47
tim.plunkett$entity->get($field_name)[0]
That's not valid in PHP 5.3
Comment #48
XanoAh, thanks. Must have been a copypaste error and I'm running 5.4 locally.
Comment #50
Xano#48: drupal_1757452_48.patch queued for re-testing.
Comment #52
klonos...the issue summary needs to be updated (this issue originally started as an effort to add UUIDs to reference fields) and the issue title is too vague to understand what we're doing here.
Comment #53
XanoThe config entity reference test still fails. Haven't been able to figure out why.
Comment #54
amateescu CreditAttribution: amateescu commentedBecause referencing a config entity wasn't actually working :P
As discussed in Prague, we need to vary the target_id property (int or string) by referenced entity type. I think this should do it.
Comment #54.0
amateescu CreditAttribution: amateescu commented.
Comment #55
dawehnerThis should be public. Also the function name is kind of odd. Don't we have some generic entity form controller where this could also live?
Comment #56
tim.plunkettCan't we call this getEntityTypeLabels() or something? options() is not what I expected. Also,
public
needed. Also bonus points for using array_map() insteadOtherwise, this looks great (way easier to review if you diff locally with -w)
Comment #58
amateescu CreditAttribution: amateescu commentedRenamed that method to getEntityTypeLabels(), but I think an array_map() would just make it less readable..
Comment #59
XanoI'm not sure the method should be a getter, as we're not actually getting a value from the object (something that can be set as well), but we retrieve a dynamically assembled list.
Comment #60
amateescu CreditAttribution: amateescu commentedJust
entityTypeLabels()
then?Comment #61
XanoI think that would indeed be a more accurate description of what the method does.
We should probably check for ConfigEntityInterface and ContentEntityInterface instead. I believe this was also discussed and agreed upon earlier this year, as we require entities to implement either of those interfaces, but storage is pluggable and does not say anything about entity IDs' data types.
Comment #62
amateescu CreditAttribution: amateescu commentedYep, I think that makes sense, I guess I was just lazy in the previous patch :/
Comment #64
amateescu CreditAttribution: amateescu commentedRerolled.
Comment #65
dawehnerTested it by choosing views as config entity. Sadly views can't build a view of views, because then you could reference a view using a view of views.
Okay I went with the default selecting method.
While creating a new node I should reference a view, though I tried "content", "Content" but none of those worked even
there is a Content view serving "admin/content".
Note: The autocompletion also failed.
Additional this patch should maybe inject the entity manager.
Comment #66
amateescu CreditAttribution: amateescu commentedThat's an interesting bug you found there, but it's related to config entity query, not ER :) This code should return the 'Content' view.. and it doesn't work. I'll open a separate issue for it.
The problem with this is that the selection plugin manager uses the "non-conventional" ReflectionFactory instead of ContainerFactory. I'll open another issue to decouple selection plugins from field definitions :)
Comment #67
BerdirWould it make more sense to check for a revision table/key in the definition?
What else could it be other than int or string? :) I guess the numeric check was when we still had autocreate in target_id ?
We could just write this like this:
?
Comment #68
fagoShouldn't that be getEntityTypeLabels()? The other methods on the manager use the get.. pattern.
There shouldn't be a requirement for content entities to have numeric IDs?
I'd suggest taking the "id" entity key and looking up the definition of the first field item property (or go with the main property once introduced by #2015687: Convert field type to FieldType plugin for taxonomy module ).
Comment #69
XanoThat would make it look like a real getter, for which a setter is available as well. Instead, it computes the labels, so we opted not to use "get" as part of the method name.
Comment #70
fagohm, generally methods should do something. not sure about assembleEntityTypeLabels() - get would work for me, but I do not want to start bikesheding on that ;-)
Comment #71
fagoA getter does not imply there has to be a setter.
Comment #72
amateescu CreditAttribution: amateescu commentedAfter a quick bikeshed here and in IRC we decided to go with getEntityTypeLabels(). Also agreed to open a separate issue for the false requirement of content entities to have numeric IDs.
Comment #73
amateescu CreditAttribution: amateescu commentedOpened #2107243: Decouple entity reference selection plugins from field definitions and #2107249: Don't assume that content entities have numeric IDs in EntityReferenceItem, and linked the latter in the @todo.
Comment #74
amateescu CreditAttribution: amateescu commentedAlso opened #2107309: Case (in)sensitivity for config entity query for the config entity query problem.
Comment #75
BerdirThere's a problem with the validation of autocomplete inputs.
That defaults to $label ($id) in the output, but the validate then won't accept that because AutocompleteWidget::elementValidate() has a hardcoded assumption about id being integer:
Failing that, it tries to do an EFQ for e.g. "Administrator (administrator) on the label and that doesn't match either.
Comment #76
chx CreditAttribution: chx commentedComment #77
amateescu CreditAttribution: amateescu commentedThanks @chx, I added a @todo so we can get away with assuming that content entity IDs are numeric for now. Also, fixed up $value not being assigned and added an integration test for the whole thing.
Comment #78
BerdirOk, manually tested this and it's working fine. I've created a field that references a field definition and it works fine (Apart from not displaying it because it doesn't have an access controller or admin permission, but that's not the fault of this issue). A role reference was displayed just fine.
This has been reviewed by me and various others. Let's get this in, then we can do cool stuff with e.g. $node->type, $term->vid and so on.
Comment #79
alexpottPatch no longer applies.
Comment #80
amateescu CreditAttribution: amateescu commentedHere you go :)
Comment #81
amateescu CreditAttribution: amateescu commentedAnd another one.
Comment #82
catchCommitted/pushed to 8.x, thanks!
Comment #83
jibranThis deserves change notice IMO.
Comment #84
BerdirChange compared to what? A HEAD to HEAD chang notice is imho not necessary and 7.x users already expect that they can reference all entity types. In a way, this just implemented a feature that users already expect to work :)
And the only change notice about adding entityreference is https://drupal.org/node/1796546.
Comment #85
amateescu CreditAttribution: amateescu commentedI feel the same way. I'd gladly write a notice if there was anything else to write than the title :)
Comment #86
webchick:(
#2116551: Fix the UX of Entity Reference fields
Comment #86.0
webchickUpdated issue summary with the current solution.