Updated: Comment #0
Problem/Motivation
Currently some entity bundle fields are defined as string. Example: the taxonomy term "vid" field is just a string field, but it should rather be an entity reference field that points to a vocabulary configuration entity.
Proposed resolution
Convert all entity bundle fields to entity reference to be consistent across all entity types.
Remaining tasks
Create a patch and fix potential test fails.
User interface changes
none.
API changes
The field type of the bundle base field of an entity would change, which could break assumptions about the bundle field in (contributed) modules.
Comment | File | Size | Author |
---|---|---|---|
#22 | 2181593-22.patch | 4.48 KB | andypost |
#22 | interdiff.txt | 765 bytes | andypost |
#19 | interdiff-2181593-15-19.txt | 926 bytes | chakrapani |
#19 | convert_entity_bundle_fields_to_reference-2181593-19.patch | 3.73 KB | chakrapani |
#15 | convert_entity_bundle_fields_to_reference-2181593-15.patch | 3.55 KB | chakrapani |
Comments
Comment #1
Github sync CreditAttribution: Github sync commentedklausi opened a new pull request for this issue.
Comment #2
klausiHere is a start. I saw that many entity types are using entity_reference for their bundle already, so I just had to convert the remaining ones.
Comment #3
amateescu CreditAttribution: amateescu commentedLooks good to me.
Comment #4
BerdirThis currently just works because EntityReferenceItem still has the magic methods overridden and redirects ->value to ->target_id.
I'd rather not rely on that and prefer to update existing code references, at least comment should have tons of ->field_id->value calls all over the place.
It would be easier if #2016679: Expand Entity Type interfaces to provide methods, protect the properties would have been committed already for comments... :(
Comment #5
amateescu CreditAttribution: amateescu commentedDiscussed a bit with @Berdir on IRC and he convinced me that the subtle API change introduced here is not really ok. So we can either wait for #2028025: Expand CommentInterface to provide methods and do the switch properly or just leave Comment out of this patch.
Comment #6
BerdirYep, I was in a bit in a hurry, let me explain my thoughts a bit for my previous post.
Thanks to the following and similar methods in EntityReferenceItem, you might think that this is no API change when it in fact is:
That's why no changes are necessary and it still works.
However, this code was added before we introduced
getMainPropertyName
and generic code (e.g. the database storage controller) needed a way to get the value of a field in a generic way. We have that now, and I'd prefer to remove those hacks, in order to have less unexpected behavior (If you'd call getPropertyDefinitions(), it will tell you that there is ->target_id and ->entity, but not ->value, so why does it work?) and it might give you the (wrong) idea that there is always a ->value, which there is not.So, what about this:
1. I'd like to avoid a huge conflict with #2028025: Expand CommentInterface to provide methods and that will also minimize the necessary changes (another reason why those methods are useful), so let's postpone changing comment on that issue. If that issue gets in first, we'll update it here, otherwise, we update the code there once this is in.
2. Open a follow-up issue to try and remove those magic magic methods from EntityReferenceItem, make sure that calling code uses target_id.
3. Look at the remaining two cases and check if there are cases that we need to update. I think I already changed most $term->vid to $term->bundle(), I found a single use with a short search), I can't find a single case for custom_block, I think that already uses ->bundle() everywhere. Which shows that the API change impact exists but is minimal as code should either use the generic bundle() method when working with a generic entity or the entity type specific method if available, like $node->getType() and they have nothing to be worried about.
Comment #7
amateescu CreditAttribution: amateescu commentedI agree with this plan :)
Comment #8
Github sync CreditAttribution: Github sync commentedklausi pushed some commits to the pull request.
Comment #9
klausiInterdiff see https://github.com/klausi/drupal/pull/15/commits
Converted one instance of $term->vid->value to $term->bundle(), could not find any other instance.
Created follow-up: #2188075: Remove magic getter of EntityReferenceItem
Comment #10
BerdirThanks, yes, looks like there's fewer things to update than I expected. Back to RTBC then, @amateescu agreed in IRC.
Comment #11
xjmThis looks like a fairly minor API change; are there any existing change records we'd update?
Edit: I guess probably the one for #2112239: Convert base field and property definitions for a start.
Comment #12
xjmAh, based on @berdir's comment on that issue, no change record action needed until #2002134: Move TypedData metadata introspection from data objects to definition objects lands?
Comment #13
BerdirThe API change is that the changed fields are now an entity reference, which means their actual properties that you should be using are ->target_id and ->entity (yes, $term->vid->entity == $vocabulary). This mostly affects code that acts in a generic way with entities, for example the serialization stuff changes, a term exposed over REST now has different properties and there's an additional explicit reference from the term to the vocabulary.
Due to the the mapping of ->value to ->target_id, this is not yet visible, but the referenced issue will make it so. (That is #2188075: Remove magic getter of EntityReferenceItem, you referenced the wrong one)
Comment #14
alexpott2181593_0.patch no longer applies.
Comment #15
chakrapani CreditAttribution: chakrapani commentedHere we go, Re-rolling the patch from #8.
Comment #16
klausiLooks good, back to RTBC.
Comment #17
andypostI think comment needs more work because #2149859: Convert the 'field_id' base field on comment entities to an entity reference field was closed as duplicate of the issue
The related issue does not supposed to change this!
The actual issue was closed as duplicate of that one.
Comment #18
andypostComment #19
chakrapani CreditAttribution: chakrapani commentedHere is a patch with suggestions from #17.
@andypost thank you :)
Comment #22
andypostFix broken test, now vocabulary should exist to create term!
+1 to RTBC
Comment #23
klausiExcellent, back to RTBC.
Comment #24
xjmBased on @Berdir's comment in #13 above, this is good to go change record-wise.
Comment #25
alexpottCommitted 0b2c622 and pushed to 8.x. Thanks!
Comment #26
andypostReopened #2149859: Convert the 'field_id' base field on comment entities to an entity reference field because #2002158: Convert form validation of comments to entity validation reverts following hunk
Comment 'field_id' is 'node__comment' but actual
field->id()
is 'node.comment'Comment #27
Berdir