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
The comment entity has the entity_id field which is of type integer. This results i.e. in module default_content not being able to correctly import/export comments.
Proposed resolution
Make the entity_id field to be a reference field via bundleFieldDefinitions() where it would be possible to specify the target_type based on the bundle.
Remaining tasks
-
User interface changes
-
API changes
-
Comment | File | Size | Author |
---|---|---|---|
#30 | drupal8-comment_target_reffield-2233157-30.patch | 5.46 KB | Jalandhar |
Comments
Comment #1
blueminds CreditAttribution: blueminds commentedHere's the patch, let's see how tests are passing.
Comment #2
blueminds CreditAttribution: blueminds commentedComment #4
blueminds CreditAttribution: blueminds commentedComment #6
blueminds CreditAttribution: blueminds commented4: comment_target_reffield-2233157-2.patch queued for re-testing.
Comment #7
larowlanComment #8
larowlanThe field is still required? Or are all ER fields required by default.
Awesome!
If the answer to (1) is 'all ER fields are required', then this is RTBC.
Great work!
Comment #9
BerdirYeah, I think it should still be required, not sure why @blueminds removed it :)
Comment #10
blueminds CreditAttribution: blueminds commentedRequired readded.
Comment #11
aspilicious CreditAttribution: aspilicious commented+ list($target_type, ) = explode('__', $bundle);
My gut feeling tells me there should be a function somewhere to extract the target type from a bundle. No?
Comment #12
BerdirYes, but that's not a problem that we are introducing here, this is something for #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form.
Comment #13
larowlanthis needs the third argument (2) as per CommentBundleEnhancer::enhance in case user names field field__snazzy via the UI
Comment #14
blueminds CreditAttribution: blueminds commentedPlease see the patch.
Comment #15
amateescu CreditAttribution: amateescu commentedFrom an ER point of view, this looks very nice. I'm just wondering if/how we can find other places which might try to access the value of the field with
->value
instead of->target_id
.Also, would this be a good time to update the field name?
get('entity_id')->target_id
looks... unfortunate :)Comment #16
BerdirYeah, that's a common problem that we have elsewhere, like $node->uid, not sure about changing that here, would mean a considerable bigger change.
I'm pretty sure that there is no direct access to entity_id left, should all use the methods.
Also, welcome back :)
Comment #17
amateescu CreditAttribution: amateescu commentedThanks :)
Ok then, let's move forward with what we can for now.
Comment #18
andypostOverall great!!!
any reason for this default value?
Comment #19
tstoecklerNot marking needs work for that, but in case this gets a re-roll this should be moved to setSetting(). setSettings() should never be used as it removes the default settings by the field item class. See #2236903: setSettings() on field definitions can unintentionally clear default values.
Super minor, but omitting the ", " would work as well, i.e. :
list($target_type) = ...
Comment #20
andypost#18 needs work 'entity_id' could be string!
Comment #21
BerdirNo, right now the schema is hardcoded to an integer, so it can't be a string. That's not related to this issue. Not sure why we do need a default_value though?
Comment #22
blueminds CreditAttribution: blueminds commentedUpdated, please see the patch.
Comment #23
tstoecklerAwesome, thanks for the quick turnaround!
There's still a default value and a setSettings() here. Can you quickly fix that as well? That would be great, thanks.
Comment #24
Jalandhar CreditAttribution: Jalandhar commentedUpdated patch with change said in #23. Please review.
Comment #25
tstoecklerI don't think 'comment' makes sense here. I.e. comments on comments. I think that line can just be removed.
Comment #26
BerdirCorrect, there is only a target type once we know the bundle.
@tstockler: Is schema() of entity references already capable to handle that? This might break the schema generation?
Comment #27
tstoecklerHeh, I thought about that as well. No, it currently can't. should be an easy fix, though. It only gets the entity type as it uses the 'integer' schema type for content entities.
So should we fix that here?
Comment #28
Jalandhar CreditAttribution: Jalandhar commented@Berdir, @tstoeckler:
btw, I have made the change that you said in #25 by looking at the existing code just above than it in same file(pasting below).
Comment #29
BerdirYes, but that has nothing to do with this field, so it shouldn't be added for this.
We decided that we won't touch schema() here, so just needs an update to remove that line. All patches were green, so that's good.
Comment #30
Jalandhar CreditAttribution: Jalandhar commentedOk. Removed that line now.
Comment #31
tstoecklerYup, looks great. Thanks!
Comment #32
andypost+1 to RTBC
Comment #33
webchickHm. I might be wrong, but this smells like something that could do with some profiling.
Comment #34
BerdirNo need, as long as you don't actually access the referenced entity, there is only a very minor overhead and this is in line with every other similar field (like the node author and node type), we've just been holding out on this because we weren't able to do the by-bundle definition earlier.
And in return, this gives us proper validation of this field and better integration with REST or similar things that rely on UUID's for referenced entities.
Comment #35
webchickOk, right on. Thanks for the explanation. The patch surprisingly involves far fewer changes than I would've guessed. :)
Committed and pushed to 8.x. Thanks!
Comment #37
andypostFiled follow-up #2245001: Remove unneeded CommentManagerInterface::getParentEntityUri()